Bug 211724 - stress/array-buffer-view-watchpoint-can-be-fired-in-really-add-in-dfg.js failing on ppc64le and s390x
Summary: stress/array-buffer-view-watchpoint-can-be-fired-in-really-add-in-dfg.js fail...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: PC Linux
: P2 Normal
Assignee: Michael Catanzaro
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-05-11 07:11 PDT by Michael Catanzaro
Modified: 2020-05-19 09:08 PDT (History)
6 users (show)

See Also:


Attachments
Patch (2.65 KB, patch)
2020-05-12 09:49 PDT, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (3.48 KB, patch)
2020-05-15 08:53 PDT, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (3.71 KB, patch)
2020-05-15 11:05 PDT, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch for landing (3.77 KB, patch)
2020-05-19 08:36 PDT, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch for landing (3.77 KB, patch)
2020-05-19 08:41 PDT, Michael Catanzaro
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Catanzaro 2020-05-11 07:11:39 PDT
stress/array-buffer-view-watchpoint-can-be-fired-in-really-add-in-dfg.js is failing on ppc64le and s390x. Note these architectures don't support llint or JIT, they only use cloop.

As far as I can tell, the stress tests don't output *anything* when they fail, so that means all I know is "it fails."

After fixing bug #210685, all other JSC stress tests are passing on both architectures except for this one.

In order to skip the tests, Tomas discovered that we need to modify determineArchitectureFromELFBinary() in run-jsc-stress-tests to read the architecture out of the ELF file....
Comment 1 Michael Catanzaro 2020-05-11 15:12:41 PDT
This turned into quite an adventure.

Eventually I tracked down the source code for the file command. I'm attaching here the magic file it uses to determine CPU architecture. It's not easy to read, but it matches our implementation of determineArchitectureFromELFBinary. Looks like the magic number for 64-bit PowerPC is 21 (regardless of endianness), and the magic number for all variants of S/390 is 22. So we could blindly add them to determineArchitectureFromELFBinary and see whether or not it works....
Comment 2 Michael Catanzaro 2020-05-11 15:14:31 PDT
But tbh, since the test is failing on armv7 as well, the past of least-resistance would be to skip unconditionally until it can be fixed. Ideally someone with more knowledge of JSC would decide how to handle it... all I can do is make it skippable.
Comment 3 Michael Catanzaro 2020-05-12 09:48:01 PDT
I also found bug #210641. Maybe it would make sense to use fewer iterations on all architectures? If that's not enough to enter dfg, then maybe this should be an x86_64-only test...?

Anyway, here's my attempt to skip the test on ppc64le and s390x. We won't know if it works until after it lands.
Comment 4 Michael Catanzaro 2020-05-12 09:49:08 PDT
Created attachment 399137 [details]
Patch
Comment 5 Michael Catanzaro 2020-05-14 06:41:25 PDT
OK, since I see no other comments or suggestions, let's land this? Ping reviewers.
Comment 6 EWS 2020-05-14 07:34:50 PDT
Committed r261689: <https://trac.webkit.org/changeset/261689>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 399137 [details].
Comment 7 Radar WebKit Bug Importer 2020-05-14 07:35:22 PDT
<rdar://problem/63227393>
Comment 8 Michael Catanzaro 2020-05-14 08:09:05 PDT
Thanks!
Comment 9 Michael Catanzaro 2020-05-15 05:41:23 PDT
It fixed ppc64le, but not s390x, the determineArchitectureFromELFBinary change doesn't work there.
Comment 10 Michael Catanzaro 2020-05-15 07:15:20 PDT
(In reply to Michael Catanzaro from comment #9)
> It fixed ppc64le, but not s390x, the determineArchitectureFromELFBinary
> change doesn't work there.

Looking at an actual s390x binary, I got the right number (22, so 0x16) but the script fails to properly check the endianness of the binary. It's expecting 0x16 0x00 but big endian is 0x00 0x16.
Comment 11 Michael Catanzaro 2020-05-15 08:53:05 PDT
Created attachment 399483 [details]
Patch
Comment 12 Michael Catanzaro 2020-05-15 11:05:53 PDT
Created attachment 399496 [details]
Patch
Comment 13 Michael Catanzaro 2020-05-19 06:52:32 PDT
Ping reviewers ;)
Comment 14 Carlos Alberto Lopez Perez 2020-05-19 07:55:23 PDT
Comment on attachment 399496 [details]
Patch

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

> Tools/Scripts/run-jsc-stress-tests:383
> +    # MIPS and PowerPC may are bi-endian. S390 (which includes S390x) is big endian. The rest are little endian.

Please fix this comment.:

- # MIPS and PowerPC may are bi-endian. S390 (which includes S390x) is big endian. The rest are little endian.
+ # MIPS and PowerPC may be little-endian or big-endian. S390 (which includes S390x) is big-endian. The rest are little-endian.

MIPS can be little-endian or can be also big-endian.
The MIPS architecture variant we target our CI its MIPS32le (le stands for little-endian).
Comment 15 Michael Catanzaro 2020-05-19 08:29:56 PDT
I meant "bi-" as in "both," but I see I have a typo there anyway, and your wording is more clear, so sure.
Comment 16 Michael Catanzaro 2020-05-19 08:36:53 PDT
Created attachment 399737 [details]
Patch for landing
Comment 17 Michael Catanzaro 2020-05-19 08:41:10 PDT
Comment on attachment 399737 [details]
Patch for landing

Another typo :S
Comment 18 Michael Catanzaro 2020-05-19 08:41:35 PDT
Created attachment 399738 [details]
Patch for landing
Comment 19 EWS 2020-05-19 09:08:12 PDT
Committed r261862: <https://trac.webkit.org/changeset/261862>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 399738 [details].