Closed
Bug 1409382
Opened 7 years ago
Closed 7 years ago
EnumSet.h - mVersion can be uninitialized
Categories
(Core :: MFBT, defect)
Core
MFBT
Tracking
()
RESOLVED
FIXED
mozilla58
People
(Reporter: Sylvestre, Assigned: Sylvestre)
References
Details
Attachments
(1 file)
(deleted),
text/x-review-board-request
|
froydnj
:
review+
ritu
:
approval-mozilla-beta+
|
Details |
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 3•7 years ago
|
||
mozreview-review |
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•7 years ago
|
||
mozreview-review |
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.
Attachment #8919289 -
Flags: review?(nfroyd) → review+
Comment 5•7 years ago
|
||
mozreview-review |
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 hidden (mozreview-request) |
Assignee | ||
Comment 7•7 years ago
|
||
mozreview-review-reply |
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?
Pushed by sledru@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/967a16acd18c
EnumSet.h - Initialize mVersion to silence a warning with gcc 8 r=froydnj
Comment 9•7 years ago
|
||
mozreview-review-reply |
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.
Assignee | ||
Comment 10•7 years ago
|
||
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
Attachment #8919289 -
Flags: approval-mozilla-beta?
Comment 11•7 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
status-firefox57:
--- → affected
Comment on attachment 8919289 [details]
Bug 1409382 - EnumSet.h - Initialize mVersion to silence a warning with gcc 8
low risk, Beta57+
Attachment #8919289 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 13•7 years ago
|
||
bugherder uplift |
You need to log in
before you can comment on or make changes to this bug.
Description
•