ASan builds should turn orange when the symbolizer fails

RESOLVED FIXED in mozilla33

Status

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mccr8, Assigned: mccr8)

Tracking

unspecified
mozilla33
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

Assignee

Description

5 years ago
As we saw in bug 1020590, the ASAN symbolizer can start failing without anybody noticing right away.  This can be really bad when an intermittent UAF lands and it takes longer to figure out what is going on.  So we should make it burn the tree if somebody breaks the symbolizer.

Decoder said in bug 1020590 "The problem is that when running locally, the symbolizer isn't really a prerequisite, so we would just want this to happen when the llvm-symbolizer binary isn't found in the package that is shipped to the testers." This sounds reasonable to me, but it doesn't seem like a huge deal to me if we printed out TEST-UNEXPECTED-FAIL when it can't find the symbolizer.  If somebody is running ASan locally, the test will still run, they will still just get an additional failure.
Assignee

Comment 1

5 years ago
Note that when LSan lands, breaking the symbolizer will cause the tree to go orange, because the suppressions list will fail to suppress anything, and a bunch of leaks will show up.  That will look odd though and is probably hard to decipher what the real problem is, so I'd like to more explicitly check this.
Summary: ASAN builds should turn orange when the symbolizer fails → ASan builds should turn orange when the symbolizer fails
Assignee

Comment 2

5 years ago
Decoder, what do you think about just doing TEST-UNEXPECTED-FAIL when the symbolizer file doesn't exist (when the check |os.path.isfile(llvmsym)| fails in automationutils.py)?
Flags: needinfo?(choller)
This sounds reasonable to me. I don't know though how many people run e.g. Mochitests locally and would be confused by the unexpected failure. If it's clear though that it is just the symbolizer missing that is causing the failure, then I have no objections.
Flags: needinfo?(choller)
Assignee

Comment 4

5 years ago
Great.
Assignee: nobody → continuation
(In reply to Christian Holler (:decoder) from comment #3)
> This sounds reasonable to me. I don't know though how many people run e.g.
> Mochitests locally and would be confused by the unexpected failure. 

We could make this conditional on the MOZ_AUTOMATION variable being set, perhaps?
Comment on attachment 8440019 [details] [diff] [review]
ASan tests should show an error when the symbolizer isn't found.

Review of attachment 8440019 [details] [diff] [review]:
-----------------------------------------------------------------

so much duplication of code- glad you found all these places.
Attachment #8440019 - Flags: review?(jmaher) → review+
Assignee

Comment 8

5 years ago
I just MXR'ed for ASAN_SYMBOLIZER_PATH. :)
https://hg.mozilla.org/mozilla-central/rev/7cafd3d4f0d7
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
A 'green' run, but with the error showing:
https://tbpl.mozilla.org/php/getParsedLog.php?id=42264044&tree=Mozilla-Central
Windows 7 32-bit mozilla-central opt test cppunit on 2014-06-23 06:33:34 PDT for push 335b6610fe0c

My guess is that the line prefix is preventing the regex from seeing the TEST-UNEXPECTED-FAIL 

\o/
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee

Comment 12

5 years ago
Hmm. Also, we should not be producing that error on a non-ASan Win7 run.
Assignee

Comment 13

5 years ago
There are two problems here:
1. The code I added should use log.testFail for a failure, not log.info. Apparently CPPUnit test logging has the type of the output baked into the function you call.
2. We always run the symbolizer code for CPPUnit tests.

1 is easy to fix, but it can't be fixed without 2, or the tree will turn orange.

2 is not as easy to fix, because it requires being able to tell whether ASAN is enabled or not, which in turn requires mozinfo.find_and_update_from_json be run, which cpp unit tests don't seem to do.  Getting that to run looks a little involved, so maybe in the short term the right thing to do is to revert my changes to CPPUnit tests and file a new bug for the followup work.
Assignee

Comment 14

5 years ago
This changes the cppunittests so it just provides information about whether the symbolizer was found or not. I filed bug 1029107 for making this particular test turn the tree orange.
Attachment #8444669 - Flags: review?(jmaher)
Comment on attachment 8444669 [details] [diff] [review]
Non-fatally provide information about the LLVM symbolizer in cppunittests.

Review of attachment 8444669 [details] [diff] [review]:
-----------------------------------------------------------------

have you verified on a non asan build that this won't turn orange?  otherwise, thanks for working on this- sounds like a long road
Attachment #8444669 - Flags: review?(jmaher) → review+
Thank you for this :-)
Assignee

Comment 17

5 years ago
(In reply to Joel Maher (:jmaher) from comment #15)
> have you verified on a non asan build that this won't turn orange? 

Yeah, this fixes the problem Ed pointed out in comment 11.  The log.info won't trigger any orange, it will just show something in the log if somebody has to debug failures later.
Assignee

Comment 18

5 years ago
Also, I figure that it isn't too likely we're have symbolizer failures only on cpp so we should be okay with this for a bit.
https://hg.mozilla.org/mozilla-central/rev/225ad76a4c79
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
(In reply to Andrew McCreight [:mccr8] from comment #13)
> 2 is not as easy to fix, because it requires being able to tell whether ASAN
> is enabled or not, which in turn requires mozinfo.find_and_update_from_json
> be run, which cpp unit tests don't seem to do.  Getting that to run looks a
> little involved, so maybe in the short term the right thing to do is to
> revert my changes to CPPUnit tests and file a new bug for the followup work.

The mozinfo bit should be pretty innocuous to add, most of our other test suites do it. (cppunittests don't only because they don't use test manifests currently.)
You need to log in before you can comment on or make changes to this bug.