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

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Julian Seward [:jseward]]

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|.

Comment 1

[Julian Seward [:jseward]]

I actually meant gcc 5.3.  Duh.

Comment 2

[Daniel Holbert [:dholbert]]

(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

[Mats Palmgren (inactive)]

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.

Comment 4

[Julian Seward [:jseward]]

Attachment: Proposed fix

Comment 5

[Mats Palmgren (inactive)]

Comment on attachment 8790307 [details] [diff] [review]
Proposed fix

nit: please use // for the comment to follow code style

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/87423892cc1c
Spurious valgrind warning in in CSSParserImpl::ParseGridLine() with gcc 5.3.  r=[e-mail].

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/87423892cc1c