Closed
Bug 1353941
Opened 8 years ago
Closed 8 years ago
clang 3.8 crashes while parsing an assertion in nsExpirationTracker.h
Categories
(Core :: XPCOM, defect)
Core
XPCOM
Tracking
()
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: dholbert, Assigned: dholbert)
References
Details
Attachments
(2 files)
It seems some new code from bug 1345464 is lucky enough to trigger a bug in clang 3.8 (which means build failure).
I'll attach a text file with all the output, but the gist of it is:
===========
1. ../../../dist/include/nsExpirationTracker.h:120:7 <Spelling=../../../dist/include/mozilla/Assertions.h:402:71>: current parser token '>'
2. ../../../dist/include/nsExpirationTracker.h:92:1: parsing struct/union/class body 'ExpirationTrackerImpl'
3. ../../../dist/include/nsExpirationTracker.h:114:3: parsing function body 'ExpirationTrackerImpl<T, K, Mutex, AutoLock>'
4. ../../../dist/include/nsExpirationTracker.h:114:3: in compound statement ('{}')
5. ../../../dist/include/nsExpirationTracker.h:118:23: in compound statement ('{}')
6. ../../../dist/include/nsExpirationTracker.h:120:7 <Spelling=../../../dist/include/mozilla/Assertions.h:426:6>: in compound statement ('{}')
clang: error: unable to execute command: Segmentation fault (core dumped)
clang: error: clang frontend command failed due to signal (use -v to see invocation)
clang version 3.8.1-12ubuntu1 (tags/RELEASE_381/final)
===========
This points to nsExpirationTracker.h:120 which is here:
https://dxr.mozilla.org/mozilla-central/rev/720b9177c6856c1c4339d0fac1bf5149c0d53950/xpcom/ds/nsExpirationTracker.h#120
...which is a MOZ_RELEASE_ASSERT that was just added over in bug 1345464.
I think this is the same clang bug that we've been working around in CSS Grid code with the CLANG_CRASH_BUG special-case. Seems related at least -- an assertion inside of some templated code triggers a clang 3.8 crash.
We probably need to come up with a way to prevent this from biting Firefox developres, since my impression is that clang 3.8 is still in pretty wide use. (It's the default version of clang on Ubuntu 16.10, the current latest Ubuntu version. Not sure if it's also the default on 16.04 LTS.)
Assignee | ||
Comment 1•8 years ago
|
||
Here's the full error output, for reference.
(I believe this is fixed in newer versions of clang -- at least, if it's the same problem we hit in grid code, then it's definitely fixed for clang 3.9 and above, IIRC.)
Assignee | ||
Comment 2•8 years ago
|
||
Possible hackaround: looks like I can get past this if I convert the MOZ_RELEASE_ASSERT to an if-check + MOZ_CRASH...
Comment 3•8 years ago
|
||
(In reply to Daniel Holbert [:dholbert] from comment #0)
> We probably need to come up with a way to prevent this from biting Firefox
> developres, since my impression is that clang 3.8 is still in pretty wide
> use. (It's the default version of clang on Ubuntu 16.10, the current latest
> Ubuntu version. Not sure if it's also the default on 16.04 LTS.)
On 16.04.2 LTS it's: clang version 3.8.0-2ubuntu4
Comment hidden (mozreview-request) |
Assignee | ||
Comment 5•8 years ago
|
||
Thanks Eric! I think we'll need to accommodate developers with clang 3.8 for the foreseeable future, then. So a workaround is merited. --> Strawman patch attached.
Notes:
- My intent is to just directly invert the existing logic in my "if" check. The idea (in both the old & new code) is: if we can't authoritatively prove that the event target is on the current thread, then BAIL!
- I'm not doing the "CLANG_CRASH_BUG" thing that we do in CSS grid code, because we can't just skip this particular check -- it seems we depend on it for safety/sanity even in opt builds.
Comment 6•8 years ago
|
||
mozreview-review |
Comment on attachment 8855123 [details]
Bug 1353941: Convert a MOZ_RELEASE_ASSERT() expression to "if" + MOZ_CRASH(), to work around clang 3.8 segfault.
https://reviewboard.mozilla.org/r/127004/#review129742
Do you think it's worth a brief comment explaining why we're not using `MOZ_RELEASE_ASSERT` here?
Attachment #8855123 -
Flags: review?(nfroyd) → review+
Assignee | ||
Comment 7•8 years ago
|
||
Sure, I'll add one before landing.
Thanks for the review!
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5eecc08a74e3
Convert a MOZ_RELEASE_ASSERT() expression to "if" + MOZ_CRASH(), to work around clang 3.8 segfault. r=froydnj
Comment 10•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in
before you can comment on or make changes to this bug.
Description
•