clang 3.8 crashes while parsing an assertion in nsExpirationTracker.h

RESOLVED FIXED in Firefox 55

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

Trunk
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(2 attachments)

(Assignee)

Description

2 years ago
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)

Updated

2 years ago
Blocks: 1345464
(Assignee)

Comment 1

2 years ago
Created attachment 8855116 [details]
full clang error output

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

2 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

2 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

2 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

2 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

2 years ago
Sure, I'll add one before landing.

Thanks for the review!
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Assignee: nobody → dholbert
Status: NEW → ASSIGNED

Comment 9

2 years ago
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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5eecc08a74e3
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55

Updated

2 years ago
Duplicate of this bug: 1353940
You need to log in before you can comment on or make changes to this bug.