Bug 213487 - run-javascriptcore-tests: Support Apple Silicon running x86 processes
Summary: run-javascriptcore-tests: Support Apple Silicon running x86 processes
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jonathan Bedard
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-06-22 13:41 PDT by Jonathan Bedard
Modified: 2020-06-29 07:27 PDT (History)
7 users (show)

See Also:


Attachments
Patch (12.46 KB, patch)
2020-06-22 13:45 PDT, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (12.96 KB, patch)
2020-06-23 08:28 PDT, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (12.89 KB, patch)
2020-06-23 11:18 PDT, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (15.29 KB, patch)
2020-06-25 10:29 PDT, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (15.45 KB, patch)
2020-06-25 16:37 PDT, Jonathan Bedard
guijemont: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Bedard 2020-06-22 13:41:51 PDT
We should be able to run JavaScriptCore tests under Rosetta.
Comment 1 Radar WebKit Bug Importer 2020-06-22 13:42:07 PDT
<rdar://problem/64606667>
Comment 2 Jonathan Bedard 2020-06-22 13:45:35 PDT
Created attachment 402508 [details]
Patch
Comment 3 Saam Barati 2020-06-22 13:54:57 PDT
Comment on attachment 402508 [details]
Patch

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

> Tools/Scripts/run-javascriptcore-tests:720
> +        "-j", jscPath($productDir), "-o", $jscStressResultsDir, "--arch", $archs);

as annoying as this is, to use `--arch`, you'll either need to make all our tests recognize arm64e separately, or introduce a new option.

> Tools/Scripts/run-jsc-stress-tests:569
> +        ["/usr/bin/arch", "-" + $architecture,  pathToVM.to_s]

does this forward along all the CLI options properly?
Comment 4 Jonathan Bedard 2020-06-22 14:30:01 PDT
(In reply to Saam Barati from comment #3)
> Comment on attachment 402508 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=402508&action=review
> 
> > Tools/Scripts/run-javascriptcore-tests:720
> > +        "-j", jscPath($productDir), "-o", $jscStressResultsDir, "--arch", $archs);
> 
> as annoying as this is, to use `--arch`, you'll either need to make all our
> tests recognize arm64e separately, or introduce a new option.

I'm not sure what the concern here is. All we were using --archs for before was enabling/disabling the JIT and determining if the platform has FTL enabled. This patch just passes the user specified architecture to `arch` if the request architecture doesn't match the output of uname.

> 
> > Tools/Scripts/run-jsc-stress-tests:569
> > +        ["/usr/bin/arch", "-" + $architecture,  pathToVM.to_s]
> 
> does this forward along all the CLI options properly?

As far as I can tell, yes. But a big part of the problem with run-jsc-stress-tests is that it's not clear all of the places that jsc is actually run.
Comment 5 Sam Weinig 2020-06-22 18:09:18 PDT
Comment on attachment 402508 [details]
Patch

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

> Tools/Scripts/webkitdirs.pm:361
> +    $nativeArchitecture = "arm64e" if ($nativeArchitecture =~ /^eperm/);

This should not be necessary. If you really need it, please send me a quick email to discuss.
Comment 6 Jonathan Bedard 2020-06-23 08:28:05 PDT
Created attachment 402561 [details]
Patch
Comment 7 Jonathan Bedard 2020-06-23 11:18:18 PDT
Created attachment 402573 [details]
Patch
Comment 8 Jonathan Bedard 2020-06-25 10:29:57 PDT
Created attachment 402747 [details]
Patch
Comment 9 Saam Barati 2020-06-25 12:53:29 PDT
Comment on attachment 402747 [details]
Patch

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

> Tools/Scripts/run-javascriptcore-tests:494
> +    # Fallback is x86_64, which we replace with the native architecture if the native architecture is in the provided build
> +    $archs = "x86_64";
> +    $archs = nativeArchitecture() if index($archsInBuild, nativeArchitecture()) != -1;

what are we trying to solve here? I feel like we should always default to native architecture unless one is specified

> Tools/Scripts/run-jsc-stress-tests:571
> +def vmCommand
> +    if ($forceArchitecture)
> +        ["/usr/bin/arch", "-" + $forceArchitecture,  pathToVM.to_s]
> +    else
> +        [pathToVM.to_s]
> +    end
> +end

does this work if you tar up the tests and run via "sh runscript"? If not, maybe let's only allow this if we're running locally

> Tools/Scripts/webkitdirs.pm:1056
> +    return $nativeArchitecture;

can you assert we don't get arm64e here?
Comment 10 Jonathan Bedard 2020-06-25 12:59:50 PDT
Comment on attachment 402747 [details]
Patch

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

>> Tools/Scripts/run-javascriptcore-tests:494
>> +    $archs = nativeArchitecture() if index($archsInBuild, nativeArchitecture()) != -1;
> 
> what are we trying to solve here? I feel like we should always default to native architecture unless one is specified

Unless the build you were provided is only x86_64 and you're on Apple Silicon and provided no arguments. In that case, you ARE running x86_64, we don't want any reporting logic to say otherwise.

>> Tools/Scripts/webkitdirs.pm:1056
>> +    return $nativeArchitecture;
> 
> can you assert we don't get arm64e here?

Will actually be in determineNativeArchitecture, but sure.
Comment 11 Jonathan Bedard 2020-06-25 16:37:46 PDT
Created attachment 402828 [details]
Patch
Comment 12 EWS 2020-06-26 10:18:47 PDT
Committed r263569: <https://trac.webkit.org/changeset/263569>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 402828 [details].
Comment 13 Guillaume Emont 2020-06-29 04:42:26 PDT
This patch breaks the $architecture variable for cross-compiling remote-running linux EWS/buildbots (currently that means the armv7 and mips pre-commit and post-commit bots), which tests rely on to be skipped, with comments like:

  //@ skip if $architecture == "arm"

Right now, I suspect we end up having $architecture==x86_64 in that case. I thought this could be worked around by using the --architecture option of run-javascriptcore-tests, but this is what I get with a cross-compiled arm build and passing "--architecture arm":

arm not supported by the provided binary, which supports '' at /home/guijemont/dev/metrological/buildroot-jsc/rpi3-buildbot/build/jsconly-custom/Tools/Scripts/run-javascriptcore-tests line 490.
Command exited with non-zero status 29
Comment 14 Jonathan Bedard 2020-06-29 06:41:47 PDT
(In reply to Guillaume Emont from comment #13)
> This patch breaks the $architecture variable for cross-compiling
> remote-running linux EWS/buildbots (currently that means the armv7 and mips
> pre-commit and post-commit bots), which tests rely on to be skipped, with
> comments like:
> 
>   //@ skip if $architecture == "arm"
> 
> Right now, I suspect we end up having $architecture==x86_64 in that case. I
> thought this could be worked around by using the --architecture option of
> run-javascriptcore-tests, but this is what I get with a cross-compiled arm
> build and passing "--architecture arm":
> 
> arm not supported by the provided binary, which supports '' at
> /home/guijemont/dev/metrological/buildroot-jsc/rpi3-buildbot/build/jsconly-
> custom/Tools/Scripts/run-javascriptcore-tests line 490.
> Command exited with non-zero status 29

I think the quickest way to resolve this is going to be to fix nativeArchitecture() so that it picks up the native architecture of the remotely targeted machine.

The assert isn't hard to fix, but if we change the assert, we need to resolve the use of arch, which would bring us back to. nativeArchitecture().

Which options trigger the remote targeting?
Comment 15 Guillaume Emont 2020-06-29 07:05:28 PDT
Comment on attachment 402828 [details]
Patch

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

> Tools/Scripts/run-javascriptcore-tests:494
> +    # Fallback is x86_64, which we replace with the native architecture if the native architecture is in the provided build
> +    $archs = "x86_64";
> +    $archs = nativeArchitecture() if index($archsInBuild, nativeArchitecture()) != -1;

This always sets $archs to x86_64 on our cross-compiling bots that run tests on a remote device of a different architecture.

> Tools/Scripts/run-javascriptcore-tests:733
> +        "-j", jscPath($productDir), "-o", $jscStressResultsDir, "--arch", $archs);

On our cross-compiling remote-test-running bots, this passes "--arch x86_64". This means that tests that have //@ headers checking $architecture get a wrong value when running on the remote device (x86_64 instead of arm or mips).

> Tools/Scripts/run-javascriptcore-tests:737
> +        push(@jscStressDriverCmd, "--force-architecture");
> +        push(@jscStressDriverCmd, $archs);

The behavior of --force-architecture seems to only be implemented for a specific emulation on macOS. Thus if we fix $archs in this script for cross-compilation cases, we should either avoid passing that argument, or fix its implementation in run-jsc-stress-tests.

> Tools/Scripts/webkitdirs.pm:844
> +    # Most ports don't have emulation, assume that the user gave us an accurate architecture

This comment is inaccurate. Though we don't currently use it on bots, we do use qemu for development on armv7 and mips on linux. I guess you mean a specific kind of emulation from macOS?

> Tools/Scripts/webkitdirs.pm:846
> +        return determineArchitecture();

I think you mean "return architecture()", since determineArchitecture() does not return anything.
Comment 16 Jonathan Bedard 2020-06-29 07:11:43 PDT
(In reply to Guillaume Emont from comment #15)
> Comment on attachment 402828 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=402828&action=review
> 
> > Tools/Scripts/run-javascriptcore-tests:494
> > +    # Fallback is x86_64, which we replace with the native architecture if the native architecture is in the provided build
> > +    $archs = "x86_64";
> > +    $archs = nativeArchitecture() if index($archsInBuild, nativeArchitecture()) != -1;
> 
> This always sets $archs to x86_64 on our cross-compiling bots that run tests
> on a remote device of a different architecture.

I didn't consider the remotely running case, seems like nativeArchitecture needs to target a remote host if one is provided.

> 
> > Tools/Scripts/run-javascriptcore-tests:733
> > +        "-j", jscPath($productDir), "-o", $jscStressResultsDir, "--arch", $archs);
> 
> On our cross-compiling remote-test-running bots, this passes "--arch
> x86_64". This means that tests that have //@ headers checking $architecture
> get a wrong value when running on the remote device (x86_64 instead of arm
> or mips).
> 
> > Tools/Scripts/run-javascriptcore-tests:737
> > +        push(@jscStressDriverCmd, "--force-architecture");
> > +        push(@jscStressDriverCmd, $archs);
> 
> The behavior of --force-architecture seems to only be implemented for a
> specific emulation on macOS. Thus if we fix $archs in this script for
> cross-compilation cases, we should either avoid passing that argument, or
> fix its implementation in run-jsc-stress-tests.

If the nativeArchitecture matches the specified architecture, we won't pass the option. Which is why I think the bug is that we're getting the nativeArchitecture wrong.

> 
> > Tools/Scripts/webkitdirs.pm:844
> > +    # Most ports don't have emulation, assume that the user gave us an accurate architecture
> 
> This comment is inaccurate. Though we don't currently use it on bots, we do
> use qemu for development on armv7 and mips on linux. I guess you mean a
> specific kind of emulation from macOS?

I have Rosetta in mind. If there is another kind of emulation that also makes sense, we could add it (just not aware of how it might work on other ports and if it's even valuable to test.

> 
> > Tools/Scripts/webkitdirs.pm:846
> > +        return determineArchitecture();
> 
> I think you mean "return architecture()", since determineArchitecture() does
> not return anything.

Looks like I did mess that up, I will land a fix for that one right now.
Comment 17 Jonathan Bedard 2020-06-29 07:22:07 PDT
Committed r263660: <https://trac.webkit.org/changeset/263660>
Comment 18 Jonathan Bedard 2020-06-29 07:27:01 PDT
Most urgently, we need https://bugs.webkit.org/show_bug.cgi?id=213727. May be some work to support emulation on other platforms, but I don't this that is a practical concern at the moment.