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)

defect
Not set
normal

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.)
Blocks: 1345464
Attached file full clang error output (deleted) —
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.)
Possible hackaround: looks like I can get past this if I convert the MOZ_RELEASE_ASSERT to an if-check + MOZ_CRASH...
(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
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 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+
Sure, I'll add one before landing. Thanks for the review!
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
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.

Attachment

General

Created:
Updated:
Size: