-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Fix Linux ARM64 build #7051
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
base: master
Are you sure you want to change the base?
Fix Linux ARM64 build #7051
Conversation
Key issues during port: * Different ARM64 vararg calling convention between macOS (DarwinPCS) and Linux (AAPCS64). Fixed CALL_ENTRYPOINT_NOASSERT and DECLARE_ARGS_VARARRAY using va_list.__stack from the official ABI - robust and compiler-independent. However, there is also JavascriptStackWalker.cpp which depends on the exact stack layout and magic constants, especially ArgOffsetFromFramePtr, this part is more fragile. * char is unsigned in Linux ARM64 ABI unlike macOS/Win, breaking int8 code. Make sure __int8 (=char) is always prefixed with signed/unsigned. * arm64/*.S files use Microsoft-style ; comments, unsupported by GNU assembler. These were already converted in amd64/*.S files to //, but I propose a simpler fix to just strip them on the fly during the build with sed. * Missing _GetNativeSigSimdContext based on https://github.com/dotnet/runtime/blob/main/src/coreclr/pal/src/thread/context.cpp#L927 * Cpsr register is PState on Linux * wchar_t/char16_t mismatches when building with ICU. For now solved with #define wcslen PAL_wcslen etc in ChakraICU.h. It builds at least, though some Intl tests are failing. Note: binary with JIT crashes - this must be built with ./build.sh --no-jit. cmake already prints a warning that ARM64 JIT is only supported on Windows.
rhuanjl
left a comment
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.
Thank you for your work on this; it's certainly good to support another platform.
I've put a few initial comments above though I'm going to want to another slightly more detailed review.
Are you able to sign our Contributor agreement? https://github.com/chakra-core/ChakraCore/blob/master/ContributionAgreement.md
| // Linux ARM64 uses AAPCS64: first 8 args in x0-x7, rest via stack. | ||
| // Fill x2-x7 with nulls here to force the expected stack layout: | ||
| // [RetAddr] [function] [callInfo] [args...] | ||
| #define CALL_ENTRYPOINT_NOASSERT(entryPoint, function, callInfo, ...) \ |
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.
I'd expect this to break the JavascriptStackWalker, I saw you said it was "fragile", what's the status?
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.
The single hard-coded constant ArgOffsetFromFramePtr that JavascriptStackWalker relies on feels rather fragile to me. For now, I set ArgOffsetFromFramePtr=4 - I see 8 extra bytes of what seems like padding or saved registers. But I'd expect it to break with different compilers and optimization flags by analogy with my experience with DECLARE_ARGS_VARARRAY - at first (before I found va_list.__stack) I was trying to fixing it by trying different offsets in _get_va(), but it just kept breaking between optimized and debug builds.
JavascriptStackWalker in JIT-less builds so far works for me with clang 19, 20, and 22, both optimized and debug builds. Perhaps because it only needs to deal with a few specific call sites in this mode. When JIT is enabled it is crashing, but because JIT wasn't supported on macOS, I didn't investigate these crashes further.
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.
The JIT crash is not remarkable, the ARM JIT contains significant windows specific logic that needs re-writing for Linux/macOS including the way function entry and exit is setup and the managing of the stack layout.
For calling JS functions in the interpreter we go via arm64_CallFunction to set the stack exactly the way we need for the stack walker and argument handling to work: https://github.com/chakra-core/ChakraCore/blob/master/lib/Runtime/Library/arm64/arm64_CallFunction.S
Yes, added my name there. |
Include system headers that may cause to PAL_* macros to get undefined early in pal.h.
Seemingly only needed for macOS. Linker complains about references to it under some configurations.
US/Pacific is a legacy alias, unavailable in the standard installation of Debian 13, which moved it to tzdata-legacy.
|
I fixed the remaining errors that I encountered. For the unicode errors: they were ultimately caused by Debian's libstdc++ undefining wcslen etc macros. I changed the fix to include the problematic system headers early in pal.h: ivankra@6bba7ee Tested commit c8cce2106209880d1c8b8d9f3f805c9c01eaeb37 in three build modes: cat >Dockerfile <<EOF
ARG BASE=ubuntu:24.04
FROM $BASE
RUN export DEBIAN_FRONTEND=noninteractive && apt-get update -y && apt-get install -y --no-install-recommends build-essential ca-certificates clang cmake git libicu-dev llvm ninja-build python3 tzdata
EOF
BASE=ubuntu:24.04; podman build --build-arg BASE=$BASE -t chakra-$BASE . && podman run --rm -it -v $PWD:$PWD -w $PWD chakra-$BASE /bin/bash -c "rm -rf out; ./build.sh --ninja --no-jit && ninja -C out/* check"On these userlands: ubuntu:22.04 (clang 14), ubuntu:24.04 (clang 18), ubuntu:26.04 (clang 21), debian:stable (clang 19), debian:sid (clang 21). Everything builds and passes all disable_jit tests, except for a single combination: ubuntu:22.04 with |
parseInt("4294967296") returned 1 due to strict aliasing bug at BigUInt.cpp:529.
Fix LuHiDbl/LuLoDbl to prevent similar issues caused by them:
access uint32 via a union, and __may_alias__ annotation for clang.
|
@ppenzin any thoughts? I'd like to do one more read through later before merge to check for any oddities I've missed. But unless I notice anything else, I think we should bring this in and then maybe one of us could open a new PR to add CI for it (and remove the build time warning assuming we get CI to pass). |
I'm always a bit nervous when there's an error we can't understand - if the stack walker is not working it's likely an incompatibility between the Call entry point handlers in Arguments.h and arm64_CallFunction; but it's odd that it would work on the newer compilers but not Clang14. |
|
Ubuntu 22.04 is going to go into security-only mode relatively soon, that can make reproducing this a bit difficult in the future, that's why I'm not sure how important it would be to fix. @ivankra how much of test suite is failing? This PR fixes Clang 21 miscompilation that we recently found. Overall it looks reasonable, but I will take another look at it. |
It builds with clang 18-22 on recent Debian/Ubuntu, both with and without ICU, passes tests and benchmarks.
Key issues:
Different ARM64 vararg calling convention between macOS (DarwinPCS) and Linux (AAPCS64).
Fixed CALL_ENTRYPOINT_NOASSERT and DECLARE_ARGS_VARARRAY using va_list.__stack from the official ABI - robust and compiler-independent.
However, there is also JavascriptStackWalker.cpp which depends on the exact stack layout and magic constants, especially ArgOffsetFromFramePtr, this part is more fragile.
char is unsigned in Linux ARM64 ABI unlike macOS/Win, breaking int8 code. Make sure __int8 (=char) is always prefixed with signed/unsigned.
arm64/*.S files use Microsoft-style ; comments, unsupported by GNU assembler. These were already converted in amd64/*.S files to //, but I propose a simpler fix to just strip them on the fly during the build with sed.
Missing _GetNativeSigSimdContext based on https://github.com/dotnet/runtime/blob/main/src/coreclr/pal/src/thread/context.cpp#L927
Cpsr register is PState on Linux
wchar_t/char16_t mismatches when building with ICU - caused by libstdc++ on Debian undefining macros like wcslen. Solved by including problematic system headers early in pal.h.
Minor compile errors and bugs fixed along the way: MAX_INT is not an exact float, UNumberFormatFields cast, strict aliasing bug in LuHiDbl/LuLoDbl, TZ=US/Pacific doesn't always exist, __reserved in sal.h conflicts with signal.h.
Note: binary with JIT crashes - this must be built with ./build.sh --no-jit. cmake already prints a warning that ARM64 JIT is only supported on Windows.
Tested as follows:
For these combinations: ubuntu:22.04 (clang 14), ubuntu:24.04 (clang 18), ubuntu:26.04 (clang 21), debian:stable (clang 19), debian:sid (clang 21), and three build commands: ./build.sh --ninja --no-jit, ./build.sh --ninja --no-jit --debug, ./build.sh --ninja --no-jit -t.
ubuntu:22.04 with ./build.sh --ninja --no-jit -t crashes in JavascriptStackWalker, but everything else passes.
Benchmarking on Octane test suite is on par with JIT-less SpiderMonkey (on Mac M4):
Fixes #7050