Clang 3.9.1 crashes when building CSSOrderAwareFrameIterator.h on gentoo

RESOLVED FIXED in Firefox 58

Status

()

Core
Layout
P3
normal
RESOLVED FIXED
4 months ago
a month ago

People

(Reporter: tedd, Assigned: tedd)

Tracking

unspecified
mozilla58
Points:
---

Firefox Tracking Flags

(firefox57 fix-optional, firefox58 fixed)

Details

Attachments

(2 attachments)

(Assignee)

Description

4 months ago
When compiling with clang, clang crashes in line 170 of layout/generic/CSSOrderAwareFrameIterator.h [1]

> 167   void SetItemCount(size_t aItemCount)
> 168   {
> 169 #ifndef CLANG_CRASH_BUG
> 170     MOZ_ASSERT(mIter.isSome() || aItemCount <= mArray->Length(),
> 171                "item count mismatch");
> 172 #endif

I am using clang 3.9.1 on a Gentoo system, clang version output:

> $ clang --version
> clang version 3.9.1 (tags/RELEASE_391/final)
> Target: x86_64-pc-linux-gnu
> Thread model: posix
> InstalledDir: /usr/bin

> $ uname -a
> Linux teddbox 4.4.26-gentoo #14 SMP Mon Jul 17 18:20:44 CEST 2017 x86_64 Intel(R) Xeon(R) CPU E3-1505M v5 @ 2.80GHz GenuineIntel GNU/Linux

Changing the ifdef in line 8:
> #if defined(__clang__) && __clang_major__ == 3 && __clang_minor__ <= 8

to __clang_minor__ <= 9 lets the build succeed.

[1] http://searchfox.org/mozilla-central/rev/30a47c4339bd397b937abdb2305f99d3bb537ba6/layout/generic/CSSOrderAwareFrameIterator.h#170

Updated

2 months ago
Priority: -- → P3
status-firefox57: --- → fix-optional
FWIW, the CLANG_CRASH_BUG workaround was added in bug 1151204, for the crash happening in clang 3.6 (with the thought that later versions were OK):
  https://hg.mozilla.org/integration/mozilla-inbound/rev/bb3080bc1aad#l1.28

...and then we loosened the exemption to extend up to version 3.8, in bug 1307226 (though under some configurations, 3.8 is fine, as noted in that bug):
  https://hg.mozilla.org/mozilla-central/rev/1a4288cc3cea#l1.13

So it's not too surprising that 3.9 is affected as well, I guess.

(Those ^^ changes were in nsGridContainerFrame, and then later this chunk of code was moved from there to its current home in the newly-created file CSSOrderAwareFrameIterator.h)

Julian, it sounds like you might already have a local patch that fixes this -- maybe you'd be up for posting that patch for review? I'd be happy to serve as reviewer.
Flags: needinfo?(julian.r.hector)
Depends on: 1307226
Summary: Clang 3.9.1 crashes when building CSSOrderAwareFrameIterator.h → Clang 3.9.1 crashes when building CSSOrderAwareFrameIterator.h on gentoo
(Assignee)

Comment 2

a month ago
Created attachment 8916535 [details] [diff] [review]
Fix compilation crash for clang 3.9.1

(In reply to Daniel Holbert [:dholbert] from comment #1)
> Julian, it sounds like you might already have a local patch that fixes this
> -- maybe you'd be up for posting that patch for review? I'd be happy to
> serve as reviewer.

Sure thing, I couldn't push to try because it has been a while so my access has been removed. I compiles locally for me.
Flags: needinfo?(julian.r.hector)
Attachment #8916535 - Flags: review?(dholbert)
Thanks! I pushed your patch to Try, and I'll land it if that looks good:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=612f78748bc28c2e40575307b561bed564dc9822

(After pushing to try, I realized that "hg import" didn't quite know what to do with the headers in the patch file, apparently -- it thought "commit 84a15ed25f746e00c4240786b288f0c60b88de70" was the commit message, and didn't know how to interpret the "Author" line.  I'll fix that up before I do the actual push.)
Hmm, I'm just noticing that we use clang 3.9.0 on our Mac builders on TreeHerder.  So -- since the CLANG_CRASH_BUG hackaround essentially disables an assertion, this change means we'll be disabling the assertion *on our mac builds on TreeHerder* (and all of their test runs).  That's too bad.

I'm not too concerned about that, though, because the assertion will still be enabled on our other builders (which use gcc on linux and MSVC on Windows), so we should still catch regressions in it. And it's in some layout code that's not really platform-dependent.  So, the remaining coverage for the assertion should still be fine.

So, though we could conceivably come up with a more targeted #if expression to exempt our mac builders, it's probably not worth the complexity, and I think we should still take this.
Created attachment 8916754 [details] [diff] [review]
amended patch that hg accepts

Here's the patch that I'll land (which "hg import" understands).

I took the liberty of clarifying the commit message (formerly "Fix compilation crash for clang 3.9.1" -- clarified to "Adjust condition for compiler-crash hackaround, to avoid crash in clang 3.9 as well")

Comment 6

a month ago
Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/821e6c2ceda2
Adjust condition for compiler-crash hackaround, to avoid crash in clang 3.9 as well. r=dholbert
Comment on attachment 8916535 [details] [diff] [review]
Fix compilation crash for clang 3.9.1

(Just realized I forgot to r+ the patch before landing it. :D  Doing so now.)
Attachment #8916535 - Flags: review?(dholbert) → review+
Assignee: nobody → julian.r.hector
https://hg.mozilla.org/mozilla-central/rev/821e6c2ceda2
Status: NEW → RESOLVED
Last Resolved: a month ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.