Closed Bug 1301856 Opened 3 years ago Closed 3 years ago

Spurious valgrind warning in in CSSParserImpl::ParseGridLine() with gcc 5.3

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: jseward, Unassigned)

Details

Attachments

(1 file)

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|.
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
(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.)
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.
Attached patch Proposed fixSplinter Review
Attachment #8790307 - Flags: review?(mats)
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.
https://hg.mozilla.org/mozilla-central/rev/87423892cc1c
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.