Closed Bug 1458382 Opened Last year Closed Last year

Work around GCC 8.0.1 internal crash.

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox-esr60 --- fixed
firefox61 --- fixed
firefox62 --- fixed
firefox63 --- fixed

People

(Reporter: sstangl, Assigned: ptomato)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 1 obsolete file)

When building the JS shell with GCC 8.0.1 on Fedora 28, the following error occurs:

 /home/sstangl/dev/gecko-dev/js/src/frontend/Parser.cpp: In member function ‘void js::frontend::GeneralParser<ParseHandler, CharT>::checkDestructuringAssignmentName(js::frontend::GeneralParser<ParseHandler, CharT>::Node, js::frontend::TokenPos, js::frontend::GeneralParser<ParseHandler, CharT>::PossibleError*)’:
/home/sstangl/dev/gecko-dev/js/src/dbg32/dist/include/mozilla/Assertions.h:417:71: internal compiler error: in cp_tree_equal, at cp/tree.c:3889
      static_assert(mozilla::detail::AssertionConditionType<decltype(x)>::isValid, \
                                                                       ^
/home/sstangl/dev/gecko-dev/js/src/dbg32/dist/include/mozilla/Assertions.h:432:5: note: in expansion of macro ‘MOZ_VALIDATE_ASSERT_CONDITION_TYPE’
     MOZ_VALIDATE_ASSERT_CONDITION_TYPE(expr); \

... etc, etc.

The error appears to be in a tree comparison function when expanding through a bunch of macros.

An easy workaround is to pre-calculate the value being asserted outside of the macro, so that GCC's tree operations don't have to deal with whatever precise conditions are giving rise to the error.

With this patch applied, the shell compiles.
Attachment #8972464 - Flags: review?(jwalden+bmo)
Comment on attachment 8972464 [details] [diff] [review]
0001-Break-up-a-one-liner-to-prevent-an-internal-GCC-8.0..patch

Review of attachment 8972464 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/frontend/Parser.cpp
@@ +9163,5 @@
>                                                                       PossibleError* possibleError)
>  {
> +#ifdef DEBUG
> +    // GCC 8.0.1 crashes if this is a one-liner.
> +    bool is_name = handler.isName(name);

isName for camelCaps naming consistency.

This should get backported to all branches, IMO, so those of us building on Fedora 28 and later -- or with newer gcc -- can keep compiling older releases.
Attachment #8972464 - Flags: review?(jwalden+bmo) → review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0b238edd9258
Break up a one-liner to prevent an internal GCC 8.0.1 error. r=Waldo
Keywords: checkin-needed
Have we reported this upstream?
https://hg.mozilla.org/mozilla-central/rev/0b238edd9258
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
I ran into this compiling ESR60 with the Freedesktop 18.07 Flatpak SDK. I agree that it should be backported. Strangely, it only occurred for me when I changed some unrelated code (with the patch from bug 1478679.) Was it ever reported upstream to GCC?
Blocks: sm.embedding
In fact it happened to me in a different assert. Here's a patch to put the same workaround in that assert as well.
Attachment #8995550 - Flags: review?(sstangl)
Assignee: sstangl → philip.chimento
Here's a version of the patch suitable for backporting to ESR60, since the code changed slightly in the meantime.
Attachment #8995551 - Flags: review?(sstangl)
Attachment #8995550 - Flags: review?(sstangl) → review+
Attachment #8995551 - Flags: review?(sstangl) → review+
Attachment #8995550 - Flags: checkin?
Keywords: checkin-needed
Pushed by dluca@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/019ed2fbfc31
Repeat GCC bug workaround in another place. r=sstangl
Keywords: checkin-needed
Comment on attachment 8995551 [details] [diff] [review]
Repeat GCC bug workaround in another place (ESR60 version)

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: Bug 1478679, also under consideration for uplift, triggers a compiler bug in GCC 8.1, which these patches work around.
User impact if declined: Bug 1478679 can't be uplifted, so the memory leak in the developer tools will remain.
Fix Landed on Version: 63
Risk to taking this patch (and alternatives if risky): Low risk, the fix is not invasive.
String or UUID changes made by this patch: None

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Attachment #8995551 - Flags: approval-mozilla-esr60?
Philip, the patch with the flag "checkin?", is that for trunk (mozilla-central, mozilla-inbound, autoland) or is it e.g. for beta? It's unusual for a patch to land on central and ESR60 but not beta.
Flags: needinfo?(philip.chimento)
The "checkin?" patch was for trunk. I didn't realize I should provide a patch for beta as well. Looking at https://dxr.mozilla.org/mozilla-beta/source/js/src/jit/RegisterSets.h#754, it looks like the "ESR60" patch should apply to beta.
Flags: needinfo?(philip.chimento)
Comment on attachment 8995550 [details] [diff] [review]
Repeat GCC bug workaround in another place

See comment 11, 13 and 14

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: Bug 1478679, also under consideration for uplift, triggers a compiler bug in GCC 8.1, which these patches work around.
User impact if declined: Bug 1478679 can't be uplifted, so the memory leak in the developer tools will remain.
Fix Landed on Version: 63
Risk to taking this patch (and alternatives if risky): Low risk, the fix is not invasive.
String or UUID changes made by this patch: None
Attachment #8995550 - Flags: approval-mozilla-beta?
Comment on attachment 8995550 [details] [diff] [review]
Repeat GCC bug workaround in another place

Workaround for a gcc compiler issue, let's uplift for beta 15.
Attachment #8995550 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Marking fixed in 63 from comment 12.
Can you follow up to make sure someone reports this upstream? Thanks.
Flags: needinfo?(philip.chimento)
Comment on attachment 8995551 [details] [diff] [review]
Repeat GCC bug workaround in another place (ESR60 version)

Supporting patch for Spidermonkey embedders. Approved for ESR 60.2.
Attachment #8995551 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+
I can't reproduce this anymore with GCC 8.2.0, so I assume it's been fixed and there's no longer a need to report it.
Flags: needinfo?(philip.chimento)
You need to log in before you can comment on or make changes to this bug.