Bug 213487 – run-javascriptcore-tests: Support Apple Silicon running x86 processes

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Jonathan Bedard]

We should be able to run JavaScriptCore tests under Rosetta.

[Radar WebKit Bug Importer]

<rdar://problem/64606667>

[Jonathan Bedard]

Created attachment 402508 [details]
Patch

[Saam Barati]

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?

[Jonathan Bedard]

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

[Sam Weinig]

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.

[Jonathan Bedard]

Created attachment 402561 [details]
Patch

[Jonathan Bedard]

Created attachment 402573 [details]
Patch

[Jonathan Bedard]

Created attachment 402747 [details]
Patch

[Saam Barati]

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?

[Jonathan Bedard]

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.

[Jonathan Bedard]

Created attachment 402828 [details]
Patch

[EWS]

Committed r263569: <https://trac.webkit.org/changeset/263569>

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

[Guillaume Emont]

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

[Jonathan Bedard]

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

[Guillaume Emont]

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.

[Jonathan Bedard]

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

[Jonathan Bedard]

Committed r263660: <https://trac.webkit.org/changeset/263660>

[Jonathan Bedard]

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.