Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix x64 crossbuild on macOS arm64 #91413

Merged
merged 2 commits into from
Sep 6, 2023
Merged

Conversation

janvorli
Copy link
Member

The cross build is failing due to the cpufeatures.c being compiled in. This file tries to extract cpu features using cpuid / getauxval that don't make sense to execute in the cross tools.

This change disables compiling the cpufeatures.c for cross build and changes the JitGetProcessorFeatures to return zero in this case.

@janvorli janvorli added this to the 9.0.0 milestone Aug 31, 2023
@janvorli janvorli self-assigned this Aug 31, 2023
@ghost
Copy link

ghost commented Aug 31, 2023

Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas
See info in area-owners.md if you want to be subscribed.

Issue Details

The cross build is failing due to the cpufeatures.c being compiled in. This file tries to extract cpu features using cpuid / getauxval that don't make sense to execute in the cross tools.

This change disables compiling the cpufeatures.c for cross build and changes the JitGetProcessorFeatures to return zero in this case.

Author: janvorli
Assignees: janvorli
Labels:

area-NativeAOT-coreclr

Milestone: 9.0.0

@jkotas
Copy link
Member

jkotas commented Aug 31, 2023

jitinterface should not have any target specific code. Why are we cross-compiling it with TARGET != HOST in the first place?

@janvorli
Copy link
Member Author

Why are we cross-compiling it with TARGET != HOST in the first place?

It compiles that when building libjitinterface_arm64.dylib during cross components build.

@MichalStrehovsky
Copy link
Member

Isn't the problem more that cpufeatures.h/.c uses TARGET_ ifdefs, but it should use HOST_?

ILC/crossgen2 (that use this) are both crosscompilers. They already know not to call into this logic when TARGET != HOST. I would expect x64 version of crossgen2 that we built on arm64 to be able to detect CPU features on x64.

@janvorli
Copy link
Member Author

janvorli commented Sep 1, 2023

Isn't the problem more that cpufeatures.h/.c uses TARGET_ ifdefs, but it should use HOST_?

The host doesn't help here. That was actually my first thought too. Extracting the features when TARGET != HOST doesn't seem to make sense. Say we have JIT targeting arm64 running on x64. Extracting x64 features is useless for the JIT isn't it? And vice versa.

@MichalStrehovsky
Copy link
Member

Isn't the problem more that cpufeatures.h/.c uses TARGET_ ifdefs, but it should use HOST_?

The host doesn't help here. That was actually my first thought too. Extracting the features when TARGET != HOST doesn't seem to make sense. Say we have JIT targeting arm64 running on x64. Extracting x64 features is useless for the JIT isn't it? And vice versa.

ILC/crossgen is a crosscompiler and what we're building is still a crosscompiler - one can just pass --targetarch:XYZ to the built ILC/crossgen and it will do the right thing (error out if --instruction-set:native was specified but hostarch!=targetarch, or call into this API to find the right flags if hostarch==targetarch).

@jkotas
Copy link
Member

jkotas commented Sep 1, 2023

jitinterface*.dll build should be only parametrized by the host architecture. If we are building it with HOST!=TARGET for some reason, the binary should not be affected by the TARGET in any way.

This is different for the clrjit itself. clrjit build is parametrized by both the host architecture and the target architecture.

The cross build is failing due to the cpufeatures.c being compiled in.
This file tries to extract cpu features using cpuid / getauxval that
don't make sense to execute in the cross tools.

This change disables compiling the cpufeatures.c for cross build and
changes the `JitGetProcessorFeatures` to return zero in this case.
@jkotas
Copy link
Member

jkotas commented Sep 1, 2023

Conflicts...

@janvorli
Copy link
Member Author

janvorli commented Sep 1, 2023

Resolved

Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks

@jkotas
Copy link
Member

jkotas commented Sep 1, 2023

Backport candidate?

@janvorli
Copy link
Member Author

janvorli commented Sep 1, 2023

/backport to release/8.0

@github-actions
Copy link
Contributor

github-actions bot commented Sep 1, 2023

Started backporting to release/8.0: https://github.com/dotnet/runtime/actions/runs/6051186149

@github-actions
Copy link
Contributor

github-actions bot commented Sep 1, 2023

@janvorli backporting to release/8.0 failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch

Applying: Fix x64 crossbuild on macOS arm64
Applying: Reflect PR feedback
Using index info to reconstruct a base tree...
M	src/native/minipal/cpufeatures.c
M	src/native/minipal/cpufeatures.h
Falling back to patching base and 3-way merge...
Auto-merging src/native/minipal/cpufeatures.h
Auto-merging src/native/minipal/cpufeatures.c
CONFLICT (content): Merge conflict in src/native/minipal/cpufeatures.c
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0002 Reflect PR feedback
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

@github-actions
Copy link
Contributor

github-actions bot commented Sep 1, 2023

@janvorli an error occurred while backporting to release/8.0, please check the run log for details!

Error: git am failed, most likely due to a merge conflict.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants