Bug 227125

Summary: [JSC] Work around apparent miscompilation on ARM/GCC >=8.4
Product: WebKit Reporter: Angelos Oikonomopoulos <angelos>
Component: New BugsAssignee: Angelos Oikonomopoulos <angelos>
Status: RESOLVED FIXED    
Severity: Normal CC: darin, ews-watchlist, fpizlo, ggaren, guijemont, Hironori.Fujii, joepeck, keith_miller, mark.lam, msaboff, saam, tzagallo, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
ews-feeder: commit-queue-
Patch
ews-feeder: commit-queue-
Patch none

Description Angelos Oikonomopoulos 2021-06-17 08:24:07 PDT
[JSC] Work around apparent miscompilation on ARM/GCC >=8.4
Comment 1 Angelos Oikonomopoulos 2021-06-17 08:28:45 PDT
Created attachment 431667 [details]
Patch
Comment 2 Angelos Oikonomopoulos 2021-06-17 08:31:08 PDT
We get upwards of 1300 failing tests on ARMv7 after https://trac.webkit.org/changeset/278942/webkit. AFAICT the change itself is innocuous, but triggers a bug in GCC, so a revert seems inappropriate.
Comment 3 Angelos Oikonomopoulos 2021-06-17 08:41:42 PDT
Created attachment 431669 [details]
Patch
Comment 4 Angelos Oikonomopoulos 2021-06-17 08:47:56 PDT
Created attachment 431670 [details]
Patch
Comment 5 Filip Pizlo 2021-06-17 08:48:28 PDT
Comment on attachment 431670 [details]
Patch

Yikes!
Comment 6 EWS 2021-06-17 09:36:44 PDT
Committed r278991 (238917@main): <https://commits.webkit.org/238917@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 431670 [details].
Comment 7 Radar WebKit Bug Importer 2021-06-17 09:37:41 PDT
<rdar://problem/79455660>
Comment 8 Darin Adler 2021-06-19 13:15:06 PDT
Comment on attachment 431670 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=431670&action=review

> Source/JavaScriptCore/jit/RegisterSet.h:41
> +#if CPU(ARM) && COMPILER(GCC)
> +
> +#if GCC_VERSION_AT_LEAST(8, 4, 0) && !GCC_VERSION_AT_LEAST(9, 0, 0)

No reason for the nested #if here. Could just be a single string of &&, I think.

> Source/JavaScriptCore/jit/RegisterSet.h:46
> +#define REGISTERSET_BITMAP_SLACK 1

This can be a constexpr int; does not need to be a #define.
Comment 9 Guillaume Emont 2021-06-21 04:14:02 PDT
(In reply to Darin Adler from comment #8)
> Comment on attachment 431670 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=431670&action=review
> 
> > Source/JavaScriptCore/jit/RegisterSet.h:41
> > +#if CPU(ARM) && COMPILER(GCC)
> > +
> > +#if GCC_VERSION_AT_LEAST(8, 4, 0) && !GCC_VERSION_AT_LEAST(9, 0, 0)
> 
> No reason for the nested #if here. Could just be a single string of &&, I
> think.

The previous version of the patch without nested #ifs did not compile on clang EWS bots, as GCC_VERSION_AT_LEAST is not defined.
Comment 10 Angelos Oikonomopoulos 2021-06-21 06:26:22 PDT
Note: apparently this wasn't a miscompilation; it only looked like one because the bug depended on the compiler configuration and we tested with GCC versions that were not configured identically. Submitted what should be a proper fix in bug 227212.