Bug 211724 – stress/array-buffer-view-watchpoint-can-be-fired-in-really-add-in-dfg.js failing on ppc64le and s390x

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Michael Catanzaro]

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

[Michael Catanzaro]

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

[Michael Catanzaro]

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.

[Michael Catanzaro]

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.

[Michael Catanzaro]

Created attachment 399137 [details]
Patch

[Michael Catanzaro]

OK, since I see no other comments or suggestions, let's land this? Ping reviewers.

[EWS]

Committed r261689: <https://trac.webkit.org/changeset/261689>

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

[Radar WebKit Bug Importer]

<rdar://problem/63227393>

[Michael Catanzaro]

Thanks!

[Michael Catanzaro]

It fixed ppc64le, but not s390x, the determineArchitectureFromELFBinary change doesn't work there.

[Michael Catanzaro]

(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.

[Michael Catanzaro]

Created attachment 399483 [details]
Patch

[Michael Catanzaro]

Created attachment 399496 [details]
Patch

[Michael Catanzaro]

Ping reviewers ;)

[Carlos Alberto Lopez Perez]

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).

[Michael Catanzaro]

I meant "bi-" as in "both," but I see I have a typo there anyway, and your wording is more clear, so sure.

[Michael Catanzaro]

Created attachment 399737 [details]
Patch for landing

[Michael Catanzaro]

Comment on attachment 399737 [details]
Patch for landing

Another typo :S

[Michael Catanzaro]

Created attachment 399738 [details]
Patch for landing

[EWS]

Committed r261862: <https://trac.webkit.org/changeset/261862>

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