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, fujii.hironori, ggaren, guijemont, 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

Angelos Oikonomopoulos
Reported 2021-06-17 08:24:07 PDT
[JSC] Work around apparent miscompilation on ARM/GCC >=8.4
Attachments
Patch (1.85 KB, patch)
2021-06-17 08:28 PDT, Angelos Oikonomopoulos 		ews-feeder: commit-queue-
DetailsFormatted DiffDiff
Patch (1.95 KB, patch)
2021-06-17 08:41 PDT, Angelos Oikonomopoulos 		ews-feeder: commit-queue-
DetailsFormatted DiffDiff
Patch (2.00 KB, patch)
2021-06-17 08:47 PDT, Angelos Oikonomopoulos 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Angelos Oikonomopoulos
Comment 1 2021-06-17 08:28:45 PDT
Created attachment 431667 [details] Patch
Angelos Oikonomopoulos
Comment 2 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.
Angelos Oikonomopoulos
Comment 3 2021-06-17 08:41:42 PDT
Created attachment 431669 [details] Patch
Angelos Oikonomopoulos
Comment 4 2021-06-17 08:47:56 PDT
Created attachment 431670 [details] Patch
Filip Pizlo
Comment 5 2021-06-17 08:48:28 PDT
Comment on attachment 431670 [details] Patch Yikes!
EWS
Comment 6 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].
Radar WebKit Bug Importer
Comment 7 2021-06-17 09:37:41 PDT
<rdar://problem/79455660>
Darin Adler
Comment 8 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.
Guillaume Emont
Comment 9 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.
Angelos Oikonomopoulos
Comment 10 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.
Note You need to log in before you can comment on or make changes to this bug.