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

RESOLVED FIXED in mozilla38

Status

()

Core
General
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jseward, Assigned: jseward)

Tracking

unspecified
mozilla38
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments)

(Assignee)

Description

3 years ago
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.
(Assignee)

Comment 1

3 years ago
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.
(Assignee)

Comment 2

3 years ago
Created attachment 8550092 [details]
Valgrind error report for the problem in comment 1
(Assignee)

Comment 3

3 years ago
Created attachment 8550094 [details] [diff] [review]
Proposed fix for the problem in comment 1
Attachment #8550094 - Flags: review?(roc)
(Assignee)

Comment 4

3 years ago
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|.
(Assignee)

Comment 5

3 years ago
Created attachment 8550100 [details]
Valgrind error reports for the problem in comment 4
(Assignee)

Comment 6

3 years ago
Created attachment 8550112 [details] [diff] [review]
Proposed fix for the problem in comment 4
Attachment #8550112 - Flags: review?(dbaron)
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.
Attachment #8550112 - Flags: review?(dbaron) → review+
Attachment #8550094 - Flags: review?(roc) → review+
(Assignee)

Comment 8

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/59dec5ab3a40
https://hg.mozilla.org/integration/mozilla-inbound/rev/170aee1be0dd
https://hg.mozilla.org/mozilla-central/rev/59dec5ab3a40
https://hg.mozilla.org/mozilla-central/rev/170aee1be0dd
Assignee: nobody → jseward
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in before you can comment on or make changes to this bug.