1409382 - EnumSet.h - mVersion can be uninitialized

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Description

[Sylvestre Ledru [:Sylvestre]]

In file included from /root/firefox-gcc-last/js/src/gc/GCRuntime.h:11:0,
                  from /root/firefox-gcc-last/js/src/vm/Runtime.h:33,
                  from /root/firefox-gcc-last/js/src/jscntxt.h:21,
                  from /root/firefox-gcc-last/js/src/jsexn.h:15,
                  from /root/firefox-gcc-last/js/src/jsexn.cpp:11,
                  from /root/firefox-gcc-last/obj-x86_64-pc-linux-gnu/js/src/Unified_cpp_js_src25.cpp:2:
 /root/firefox-gcc-last/obj-x86_64-pc-linux-gnu/dist/include/mozilla/EnumSet.h: In function '(static initializers for /root/firefox-gcc-last/js/src/jsnativestack.cpp)':
 /root/firefox-gcc-last/obj-x86_64-pc-linux-gnu/dist/include/mozilla/EnumSet.h:330:5: error: '*((void*)& IncrementalSliceZealModes +8)' is used uninitialized in this function [-Werror=uninitialized]
      mVersion++;
      ^~~~~~~~

Comment 1

[Sylvestre Ledru [:Sylvestre]]

Attachment: Bug 1409382 - EnumSet.h - Initialize mVersion to silence a warning with gcc 8

Review commit: https://reviewboard.mozilla.org/r/190168/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/190168/

Comment 2

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8919289 [details]
Bug 1409382 - EnumSet.h - Initialize mVersion to silence a warning with gcc 8

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/190168/diff/1-2/

Comment 3

[Nathan Froyd [:froydnj]]

Comment on attachment 8919289 [details]
Bug 1409382 - EnumSet.h - Initialize mVersion to silence a warning with gcc 8

https://reviewboard.mozilla.org/r/190168/#review195428

Please nominate this for uplift; touching uninitialized memory seems like a bad thing, and this patch is pretty low risk.

::: mfbt/EnumSet.h:322
(Diff revision 2)
>    void initVersion() {
>  #ifdef DEBUG
>      mVersion = 0;
>  #endif
>    }

Shall we just remove this method and all the calls to it as well?  It looks like forgetting to call this method in the `EnumSet(T)` constructor was the source of the problem...but adding an initializer as is done below means that this method is redundant now.

Comment 4

[Nathan Froyd [:froydnj]]

Comment on attachment 8919289 [details]
Bug 1409382 - EnumSet.h - Initialize mVersion to silence a warning with gcc 8

https://reviewboard.mozilla.org/r/190168/#review195432

Argh, flip the flag too.

Comment 5

[Nathan Froyd [:froydnj]]

Comment on attachment 8919289 [details]
Bug 1409382 - EnumSet.h - Initialize mVersion to silence a warning with gcc 8

https://reviewboard.mozilla.org/r/190168/#review195434

::: commit-message-7619a:1
(Diff revision 2)
> +Bug 1409382 - EnumSet.h - Initialize mVersion to silent a warning with gcc 8 r?froydnj

Nit: "to silence a warning"

Comment 6

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8919289 [details]
Bug 1409382 - EnumSet.h - Initialize mVersion to silence a warning with gcc 8

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/190168/diff/2-3/

Comment 7

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8919289 [details]
Bug 1409382 - EnumSet.h - Initialize mVersion to silence a warning with gcc 8

https://reviewboard.mozilla.org/r/190168/#review195428

> Shall we just remove this method and all the calls to it as well?  It looks like forgetting to call this method in the `EnumSet(T)` constructor was the source of the problem...but adding an initializer as is done below means that this method is redundant now.

Happy to open a follow up bug for that, should I do it?

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/967a16acd18c
EnumSet.h - Initialize mVersion to silence a warning with gcc 8 r=froydnj

Comment 9

[Nathan Froyd [:froydnj]]

Comment on attachment 8919289 [details]
Bug 1409382 - EnumSet.h - Initialize mVersion to silence a warning with gcc 8

https://reviewboard.mozilla.org/r/190168/#review195428

> Happy to open a follow up bug for that, should I do it?

Yes please.

Comment 10

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8919289 [details]
Bug 1409382 - EnumSet.h - Initialize mVersion to silence a warning with gcc 8

Approval Request Comment
[Feature/Bug causing the regression]: bug 1266402
[User impact if declined]: Crashes, memory issues when built in DEBUG
[Is this code covered by automated tests?]: Yeah, gcc 8! :) and it seems that the coverage is great: https://codecov.io/gh/marco-c/gecko-dev/src/master/mfbt/EnumSet.h#L330
[Has the fix been verified in Nightly?]: Not yet but I have been able to build firefox in debug with that
[Needs manual test from QE? If yes, steps to reproduce]: None 
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]:  just init a var to 0 in DEBUG mode
[String changes made/needed]:no

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/967a16acd18c

Comment 12

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8919289 [details]
Bug 1409382 - EnumSet.h - Initialize mVersion to silence a warning with gcc 8

low risk, Beta57+

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/d4e212a2b1e1