1387431 - Clang 3.9.1 crashes when building CSSOrderAwareFrameIterator.h on gentoo

Status: RESOLVED FIXED

(Core :: Layout, enhancement, P3)

Description

[Julian Hector [:tedd] [:jhector]]

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

Comment 1

[Daniel Holbert [:dholbert]]

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.

Comment 2

[Julian Hector [:tedd] [:jhector]]

Attachment: 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.

Comment 3

[Daniel Holbert [: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.)

Comment 4

[Daniel Holbert [:dholbert]]

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.

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: 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

[Pulsebot]

Pushed by [e-mail]:
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 7

[Daniel Holbert [: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.)

Comment 8

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/821e6c2ceda2