Closed
Bug 1301856
Opened 4 years ago
Closed 4 years ago
Spurious valgrind warning in in CSSParserImpl::ParseGridLine() with gcc 5.3
Categories
(Core :: CSS Parsing and Computation, defect)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla51
Tracking | Status | |
---|---|---|
firefox51 | --- | fixed |
People
(Reporter: jseward, Unassigned)
Details
Attachments
(1 file)
1.08 KB,
patch
|
mats
:
review+
|
Details | Diff | Splinter Review |
This is a followup to bug 1056864 and in particular to the line of discussion that begins at bug 1056864 comment 15. The problem is that gcc 5.4 and possibly later inline Maybe::{isSome,ref} here if (integer.isSome() && integer.ref() < 0) { and then proceed to evaluate the expression right-to-left, as if it had been written integer.ref() < 0 && integer.isSome() This is a legitimate transformation because gcc presumably can prove via dataflow analysis that integer.isSome() is false whenever integer.ref() is undefined, so the overall expression result is still defined. Valgrind/Memcheck assumes that all conditional branches in the program are important, and that assumption is deeply wired in, so there is no easy way to avoid the warning apart from to force-initialise |integer|.
Reporter | ||
Comment 1•4 years ago
|
||
I actually meant gcc 5.3. Duh.
Summary: Spurious valgrind warning in in CSSParserImpl::ParseGridLine() with gcc 5.4 → Spurious valgrind warning in in CSSParserImpl::ParseGridLine() with gcc 5.3
Comment 2•4 years ago
|
||
(CC'ing mats, who's the main person working on grid, so he's in the loop.) (Based on an email exchange, I think jseward is planning taking this & moving forward with a version of his "possible fix" from bug 1056864, which is a valgrind-specific #ifdef.)
Comment 3•4 years ago
|
||
A similar #ifdef sounds good to me. Ideal would be if we could inform gcc not to do that kind of optimization for MOZ_VALGRIND builds.
Reporter | ||
Comment 4•4 years ago
|
||
Reporter | ||
Updated•4 years ago
|
Attachment #8790307 -
Flags: review?(mats)
Comment 5•4 years ago
|
||
Comment on attachment 8790307 [details] [diff] [review] Proposed fix nit: please use // for the comment to follow code style
Attachment #8790307 -
Flags: review?(mats) → review+
Pushed by jseward@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/87423892cc1c Spurious valgrind warning in in CSSParserImpl::ParseGridLine() with gcc 5.3. r=mats@mozilla.com.
Comment 7•4 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/87423892cc1c
Status: NEW → RESOLVED
Closed: 4 years ago
status-firefox51:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in
before you can comment on or make changes to this bug.
Description
•