939464 - warning: ‘*((void*)(& c)+16).js::CompartmentsInZoneIter::it’ may be used uninitialized in this function [-Wmaybe-uninitialized]

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[:Benjamin Peterson]

Attachment: uiv-zone.patch

Every since 442211bce621 from bug 928050, GCC has been spewing crap like
In file included from ../jscompartment.h:13:0,
                 from /home/benjamin/dev/mozilla/inbound/js/src/gc/Iteration.cpp:7:
../gc/Zone.h: In function ‘void JS_IterateCompartments(JSRuntime*, void*, JSIterateCompartmentCallback)’:
../gc/Zone.h:421:9: warning: ‘*((void*)(& c)+16).js::CompartmentsInZoneIter::end’ may be used uninitialized in this function [-Wmaybe-uninitialized]
/home/benjamin/dev/mozilla/inbound/js/src/gc/Iteration.cpp:137:27: note: ‘*((void*)(& c)+16).js::CompartmentsInZoneIter::end’ was declared here
In file included from ../jscompartment.h:13:0,
                 from /home/benjamin/dev/mozilla/inbound/js/src/gc/Iteration.cpp:7:
../gc/Zone.h:380:13: warning: ‘*((void*)(& c)+16).js::CompartmentsInZoneIter::it’ may be used uninitialized in this function [-Wmaybe-uninitialized]
/home/benjamin/dev/mozilla/inbound/js/src/gc/Iteration.cpp:137:27: note: ‘*((void*)(& c)+16).js::CompartmentsInZoneIter::it’ was declared here

Here is a patch, which rewrites CompartmentsIterT to not use Maybe. It's not exactly pretty, since I have to introduce the concept of a empty CompartmentsInZoneIter.

Comment 1

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 833374 [details] [diff] [review]
uiv-zone.patch

Review of attachment 833374 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks. I suspect we could eliminate the placement new by keeping the friended nullptr constructor you added and calling it in the CompartmentsIterT constructor if zone.done() is true. Then we'd always be initializing and we could continue to use Maybe<T>.

However, it's up to you what to do. I'm not sure using Maybe<T> is any cleaner than directly doing placement new.

Comment 2

[:Benjamin Peterson]

I retained the use of Maybe. It's still a bit awkward, especially Maybe now has to be friended.

https://hg.mozilla.org/integration/mozilla-inbound/rev/b4c82e9838d5

Comment 3

[:Benjamin Peterson]

Now with less bugs:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8a40011ec7cb

Comment 4

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/8a40011ec7cb

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 833374 [details] [diff] [review]
uiv-zone.patch

Review of attachment 833374 [details] [diff] [review]:
-----------------------------------------------------------------

Somehow bugzilla seems to have lost my previous review.