Draft: Start of MUSL ARM support #133
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Cool! |
|
I'm getting the following error in the test step for arm64: Did you get it too? |
|
And for the arm build, I get: I guess you had the same error? |
|
That's odd, the arm64 test step worked fine for me after I made the changes to For ARM I indeed had the same issue. I did get it to go a bit further by patching abseil-cpp a bit because it didn't seem to include the generic ARM debug/stacktrace include, but it is available in the repo. But I ran into other issues after that. Do you run these builds locally or in some virtual build environment? |
|
I suggest we focus on the I ran these builds in a fresh EC2 instance. BTW, I think that the packages |
That's fine with me.
That's possible indeed, I mainly looked at the normal ARM builds, and I remember that I did have to install some stuff to make it work, but I don't know exactly. I will also try this in a fresh VM. |
|
I'm getting the same error. It seems to be related to the options |
|
@bblanchon Fixed and pushed. It uses Alpine GCC now to compile the example. Validated with the following docker file: Do you know why the |
If I remove |
|
When I build musl x64 v8, I get the following error: So I'll merge this PR without V8 and we'll fix this some other time. |
When I look at that error, it looks like it's not using the g++ from musl? |
So, either:
|
I'll see if I can figure it out. I'm starting to wonder now if we actually need |
|
I have looked into this, it seems to have to do with NASM, it tries to compile NASM on the host CPU/OS, but I'm not sure if that is what's supposed to happen. The reason why this is only needed for x86/x64: NASM is only for x86 and x64 so it's not included when you make ARM builds, so then you won't need g++-10 on the host. I think NASM is supposed to run on the host to do these operations on the created files, so that's why it compiles it using the host g++? |
|
This cannot be for Nasm since it's written in C, but it could be for another tool. |
|
But the error literally is: I think we might have to change the host toolchain to MUSL too, but then with the CPU of the host, so: |
|
I see. I don't think we need to change the toolchain for the host, since it's working correctly. |
Ah yes, I probably don't get to that error since I don't install g++-10/g++-10-multilib in my current test environment. |
|
I just merged the branch. |
|
Thanks for merging! I think the v8 issue is related to the For musl it's currently set to: I think this was added because of v8 automatically selecting I think
Normally the v8 toolchain setup would automatically set this up, but since we override the toolchain, this is wrong on the first two options, and that's why we override So what I think we must do is either patch the v8 snapshot_toolchain.gni to take |
|
I have just tested the previous comment: diff --git a/steps/05-configure.sh b/steps/05-configure.sh
index 634caca..ec41967 100755
--- a/steps/05-configure.sh
+++ b/steps/05-configure.sh
@@ -49,7 +49,13 @@ mkdir -p "$BUILD"
echo 'is_musl = true'
echo 'is_clang = false'
echo 'use_custom_libcxx = false'
- [ "$ENABLE_V8" == "true" ] && echo "v8_snapshot_toolchain = \"//build/toolchain/linux:$TARGET_CPU\""
+ if [ "$ENABLE_V8" == "true" ]; then
+ case "$TARGET_CPU" in
+ x86|x64)
+ echo "v8_snapshot_toolchain = \"//build/toolchain/linux:$TARGET_CPU\""
+ ;;
+ esac
+ fi
;;
esacI could successfully build musl v8 builds for: x86, x64 and arm64. |
Right! I totally forgot about that part. |
|
I still can't build v8 for arm64.
In all cases, I still get the following error: My changes are in the branch |
|
Ok.... So.... Which makes sense, as the code that broke this has been changed recently: To me it feels like diff --git a/src/base/cpu.cc b/src/base/cpu.cc
index 9a21c10..57f880b 100644
--- a/src/base/cpu.cc
+++ b/src/base/cpu.cc
@@ -175,9 +175,9 @@ static V8_INLINE void __cpuid(int cpu_info[4], int info_type) {
static std::tuple<uint32_t, uint32_t> ReadELFHWCaps() {
uint32_t hwcap = 0;
uint32_t hwcap2 = 0;
-#if defined(AT_HWCAP)
+#if (V8_GLIBC_PREREQ(2, 16) || V8_OS_ANDROID) && defined(AT_HWCAP)
hwcap = static_cast<uint32_t>(getauxval(AT_HWCAP));
-#if defined(AT_HWCAP2)
+#if (V8_GLIBC_PREREQ(2, 16) || V8_OS_ANDROID) && defined(AT_HWCAP2)
hwcap2 = static_cast<uint32_t>(getauxval(AT_HWCAP2));
#endif // AT_HWCAP2
#else
@@ -196,7 +196,7 @@ static std::tuple<uint32_t, uint32_t> ReadELFHWCaps() {
break;
}
if (entry.tag == AT_HWCAP) {
- result = entry.value;
+ hwcap = entry.value;
break;
}
}I don't really know why this got merged in, it even contains compile errors because the variable |
|
Don't sweat it; it should be fixed soon. |
|
@bblanchon the patch has been merged into v8 :) |
|
Awesome, we just need to wait for PDFium to incorporate the change. |
|
PDFium now points to 06aba4270, which includes your patch. |
|
/* C++ needs to know that types and declarations are C, not C++. */
#ifdef __cplusplus
# define __BEGIN_DECLS extern "C" {
# define __END_DECLS }
#else
# define __BEGIN_DECLS
# define __END_DECLS
#endifI tried to add |
|
My patch was to fix the build for arm64 v8, not for normal arm, that still has other issues as you discovered. |
|
🤦 Sorry. Let me try the V8 build. |
|
I'm getting the following errors at the very last step: This was with |
|
What are you building exactly? The main branch? |
|
Nevermind, build passed with |
|
I think that is what this fixes: #133 (comment) |
|
Unfortunately, the branch |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.