Fixes to avoid Valgrind false positives with gcc-5.4 -O2 builds

RESOLVED FIXED in Firefox 51

Status

()

Core
General
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: jseward, Unassigned)

Tracking

Trunk
mozilla51
Points:
---

Firefox Tracking Flags

(firefox50 affected, firefox51 fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

a year ago
This is similar to bug 1122375, which does the same for gcc-4.9.X
builds, except that 5.4 is a bit more aggressive in optimising and so
there are a couple more places that need to be patched.  I had hoped
to do this for gcc-6.0 builds, but at least as of my last testing a
couple of weeks ago, we can't currently build a working Fx with
gcc-6.0 at -O or above.
(Reporter)

Comment 1

a year ago
To summarise, for all of these, the problem is that recent gccs
(and, AFAIK, clangs) will do this transformation

  A && B  -->  B && A    if A is false whenever B is undefined

It's a bit mind-bending, but if you stare at it long enough, you
can see it is correct.  The difficulty is that Valgrind/Memcheck
assumes that all conditional branches have an effect on the outcome
of the computation, and so it complains about branching on B when it
is undefined.

As gcc/clang gradually become more aggressive in optimisation, and in
particular, as they analyse ever larger sections of code, they will
presumably be able to find ever more places where the "if A is false .."
condition holds, and the transformation is desirable.  I assume that's
why we are gradually seeing more of these occurrences.
(Reporter)

Comment 2

a year ago
Created attachment 8775918 [details] [diff] [review]
bug1289098-1.diff

Initialisations for nsStyleTextReset::CalcDifference and 
nsStyleContext::CalcStyleDifference.
(Reporter)

Comment 3

a year ago
Comment on attachment 8775918 [details] [diff] [review]
bug1289098-1.diff

This is an ultra-trivial patch that gives two additional initialisations
needed to keep Valgrind/Memcheck quiet on gcc-5.3 -O2 generated code.
Currently these are the only changes of this kind needed in Gecko for
the gcc-4.9 to 5.3 transition, that I am aware of.
Attachment #8775918 - Flags: review?(dbaron)
Comment on attachment 8775918 [details] [diff] [review]
bug1289098-1.diff

Not happy about it, but r=dbaron.
Attachment #8775918 - Flags: review?(dbaron) → review+

Comment 5

a year ago
Pushed by jseward@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6041c4a602ea
Fixes to avoid Valgrind false positives with gcc-5.4 -O2 builds.  r=dbaron.

Comment 6

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6041c4a602ea
Status: NEW → RESOLVED
Last Resolved: a year 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.