1122375 - Fixes to avoid Valgrind false positives with gcc-4.9.x -O2 builds

Status: RESOLVED FIXED

Description

[Julian Seward [:jseward]]

gcc-4.9.x on x86_64 (at least, maybe other targets) performs apparently
new optimisations at -O2 that confuse Valgrind/Memcheck's uninitialised
value tracking instrumentation and result in false error reports.  As far
as I can tell, this only happens in two places in the tree.  On examination
of the machine code, in both cases the nature of the transformation makes
it difficult and/or a big performance hit to fix Valgrind, so a preferable
approach is to provide minimal dummy initialisations.

Comment 1

[Julian Seward [:jseward]]

In the first of the two problems, in layout/base/nsDisplayList.cpp,
gcc-4.9 appears to transform

  if (e1 && e2) ...

into

  if (e2 && e1) ...

in the case where it can show that e1 is |false| whenever |e2| is
undefined.  This causes Memcheck to complain because it has the
(deeply wired in) assumption that every conditional branch is
important and therefore must be based on defined values.

Comment 2

[Julian Seward [:jseward]]

Created attachment 8550092 [details]
Valgrind error report for the problem in comment 1

Comment 3

[Julian Seward [:jseward]]

Created attachment 8550094 [details] [diff] [review]
Proposed fix for the problem in comment 1

Comment 4

[Julian Seward [:jseward]]

In the second of the two problems, in layout/style/nsStyleContext.cpp,
gcc-4.9 produces code which is pretty gnarly to say the least, but afaics
depends on the observation that

   undefined-value < 0    is   defined

in the case where the comparison is unsigned, since the result will
always be |false|.

Comment 5

[Julian Seward [:jseward]]

Created attachment 8550100 [details]
Valgrind error reports for the problem in comment 4

Comment 6

[Julian Seward [:jseward]]

Created attachment 8550112 [details] [diff] [review]
Proposed fix for the problem in comment 4

Comment 7

[David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-8 (if account gets disabled due to email bounces, ask a bugzilla admin to reenable it)]

Comment on attachment 8550112 [details] [diff] [review]
Proposed fix for the problem in comment 4

Ugh.

I guess so.


But put a space after all the ","s.

Comment 8

[Julian Seward [:jseward]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/59dec5ab3a40
https://hg.mozilla.org/integration/mozilla-inbound/rev/170aee1be0dd

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/59dec5ab3a40
https://hg.mozilla.org/mozilla-central/rev/170aee1be0dd