-
-
Notifications
You must be signed in to change notification settings - Fork 204
Enable native builds on Linux aarch64 #670
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
Conversation
2aff89e
to
1b9b6ce
Compare
I think this will be a useful precursor to #670
1b9b6ce
to
fa119e9
Compare
f8cd0b1
to
801d42a
Compare
801d42a
to
e06fb79
Compare
As I mentioned at #607 (comment) — I need to review this myself as I did not review the original implementation and there may be some clean-up to be done after hacking things to work. |
cpython-unix/xcb.debian9.Dockerfile
Outdated
@@ -0,0 +1,3 @@ | |||
{% include 'build.debian9.Dockerfile' %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, I noted a couple of things that could be changed, but they don't cause incorrect behavior, so I think the builds that result from this code are fine.
build_binutils(client, get_image(client, ROOT, BUILD, "gcc"), host_platform) | ||
build_binutils( | ||
client, | ||
get_image(client, ROOT, BUILD, docker_image, host_platform), | ||
host_platform, | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note to self: This change pairs with a change to the Makefile to pass --docker-image
in the binutils
call. In turn the value passed there is rewritten from gcc.cross*
to gcc
(which is why it was previously hard-coded here), but leaves other suffixes like gcc.debian9
alone.
@@ -25,7 +25,10 @@ | |||
{"name": "build", "arch": "x86_64"}, | |||
{"name": "build.cross", "arch": "x86_64"}, | |||
{"name": "build.cross-riscv64", "arch": "x86_64"}, | |||
{"name": "build.debian9", "arch": "aarch64"}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general - is "debian9" here because we're mostly using Debian 8 and that's too old for aarch64?
If we upgrade our base builds to Debian 9, can we use the normal build
Dockerfile for aarch64
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general - is "debian9" here because we're mostly using Debian 8 and that's too old for aarch64?
I presume so, that's where I got stuck at #484 (comment) but this was Greg's choice.
@@ -579,7 +591,10 @@ def python_build_info( | |||
if lib.startswith("-l"): | |||
lib = lib[2:] | |||
|
|||
if platform == "linux_x86_64" and lib not in linux_allowed_system_libraries: | |||
if ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, if you do another rev of this PR, I think this one could also use an else: raise
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not trivial here because the else
case would also be lib in <expected>
, will omit for now.
target_cc: clang | ||
target_cxx: clang++ | ||
target_cflags: | ||
- '-fvisibility=hidden' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this is not a behavior change in practice (my current aarch64 build doesn't export symbols from dependencies, and there's a validation check for it), but I can't figure out why the cross builds seem not to need it. Opened #680.
cpython-unix/build.py
Outdated
machine = platform.machine() | ||
|
||
env["TARGET_TRIPLE"] = ( | ||
target_triple.replace("x86_64_v2-", "x86_64-") | ||
.replace("x86_64_v3-", "x86_64-") | ||
.replace("x86_64_v4-", "x86_64-") | ||
) | ||
if machine == "aarch64": | ||
env["BUILD_TRIPLE"] = "aarch64-unknown-linux-gnu" | ||
else: | ||
env["BUILD_TRIPLE"] = "x86_64-unknown-linux-gnu" | ||
env["TARGET_TRIPLE"] = ( | ||
target_triple.replace("x86_64_v2-", "x86_64-") | ||
.replace("x86_64_v3-", "x86_64-") | ||
.replace("x86_64_v4-", "x86_64-") | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't seem right - isn't platform.machine()
guaranteed to be x86_64
here? I think this code is fine as is, the new branch won't be taken, and the existing code is currently in use for x86_64 -> aarch64 cross builds (the target_triple.replace
stuff doesn't replace anything and leaves the target triple unmodified).
cpython-unix/targets.yml
Outdated
# Blocked on: | ||
# BOLT-ERROR: Cannot relax adr in non-simple function | ||
# trampoline_code_table/1. Use --strict option to override | ||
# bolt_capable: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is from BOLT getting confused at hand-written assembly in libffi. Also there seems to be no way to opt out of this optimization pass, or optimizing this one function? Reported as llvm/llvm-project#146541
This seems to "just work." Let's stay modern.
I think deletion of the legacy proto packages a few commits ago fixed up the build failures the deleted comment alluded to.
I just published an LLVM 20 toolchain for aarch64. The toolchain has support for PGO and BOLT. This commit switches the Linux aarch64 builds to be performed natively on aarch64 machines. PGO and BOLT are enabled on the builds, hopefully making them a bit faster.
Rebase of #607
Uses #672 and #673 to abstract some of the changes to the CI matrix.