This is an archive of the discontinued LLVM Phabricator instance.

[compiler-rt] Fix scudo build on ARM
ClosedPublic

Authored by luporl on Jan 30 2023, 5:25 AM.

Details

Summary

The build of scudo was failing on armv7l, with undefined references
to unwinder symbols, such as __aeabi_unwind_cpp_pr0. These are
needed by RTGwpAsan and thus, on ARM, scudo must also be linked
against an unwind library.

The cmake command that caused the build failure was:

cmake --fresh -S "$PWD/llvm/" -B "$PWD/build/" -G Ninja \
  -DCMAKE_INSTALL_PREFIX="$PWD/install" \
  -DCMAKE_BUILD_TYPE=Release \
  -DLLVM_ENABLE_PROJECTS="clang;lld;lldb;clang-tools-extra;polly" \
  -DLLVM_ENABLE_RUNTIMES="compiler-rt;libcxx;libcxxabi;libunwind" \
  -DLLVM_TOOLCHAIN_TOOLS="llvm-ar;llvm-ranlib;llvm-objdump;\
llvm-rc;llvm-cvtres;llvm-nm;llvm-strings;llvm-readobj;\
llvm-dlltool;llvm-pdbutil;llvm-objcopy;llvm-strip;llvm-cov;\
llvm-profdata;llvm-addr2line;llvm-symbolizer;llvm-windres;llvm-ml;\
llvm-readelf;llvm-size" \
  -DLLVM_INSTALL_BINUTILS_SYMLINKS=OFF -DLLVM_PARALLEL_LINK_JOBS=1

Fixes https://github.com/llvm/llvm-project/issues/60115

Diff Detail

Event Timeline

luporl created this revision.Jan 30 2023, 5:25 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 30 2023, 5:25 AM
luporl requested review of this revision.Jan 30 2023, 5:25 AM
DavidSpickett added a comment.EditedJan 30 2023, 5:51 AM

I would split this into 2 commits, where one is the triple fix. You can copy the relevant bit of the description as well. There is some thinking behind that change that should be preserved in it's own commit.

Then this can be "Fix building GWPASAN on ARM". Also include in the commit message the cmake command that was broken. Just because it's useful to see exactly what config was fixed, when looking back. They can get pretty specific, and it lets people compare their usage against a known good command.

Last time we fixed something like this was https://reviews.llvm.org/D131250 for scudo. I'm a bit confused to not see that change in this file too. Diana guessed at the time the cause was passing unwindlib=none and now we're adding it back to fix a similar issue.

The option COMPILER_RT_USE_LLVM_UNWINDER also adds --unwindlib=None, does that have the same effect as this change? I'm trying to understand if whatever unwind lib you get without this change, should be providing __aeabi_unwind_cpp_pr* or not. And exactly which unwind lib we attempt to use in each scenario.

compiler-rt/lib/scudo/standalone/CMakeLists.txt
44

Can you include the why here as well.

"This is because on ARM unwinding does/does not whatever..."

I would split this into 2 commits, where one is the triple fix. You can copy the relevant bit of the description as well. There is some thinking behind that change that should be preserved in it's own commit.

Then this can be "Fix building GWPASAN on ARM". Also include in the commit message the cmake command that was broken. Just because it's useful to see exactly what config was fixed, when looking back. They can get pretty specific, and it lets people compare their usage against a known good command.

Right, it makes sense, thanks for the quick review.

Last time we fixed something like this was https://reviews.llvm.org/D131250 for scudo. I'm a bit confused to not see that change in this file too. Diana guessed at the time the cause was passing unwindlib=none and now we're adding it back to fix a similar issue.

The option COMPILER_RT_USE_LLVM_UNWINDER also adds --unwindlib=None, does that have the same effect as this change? I'm trying to understand if whatever unwind lib you get without this change, should be providing __aeabi_unwind_cpp_pr* or not. And exactly which unwind lib we attempt to use in each scenario.

The cause of the issue was really passing unwindlib=none to the link command. In this case the unwind lib must be explicitly passed. It seems I took a different approach to fix it, that was removing unwindlib=none for ARM, when GwpAsan is enabled.
I don't know if both have the same effect or which one is better. I'll dig a bit further.

luporl updated this revision to Diff 493330.Jan 30 2023, 9:09 AM
luporl retitled this revision from [compiler-rt] Fix build on ARM to [compiler-rt] Fix building GWPASAN on ARM.
luporl edited the summary of this revision. (Show Details)
  • Split ARM build fixes in 2 parts: this and D142906
  • Add cmake command to commit message
  • Explain why ARM must be linked against an unwind library in the modified CMakeLists.txt file
luporl marked an inline comment as done.Jan 30 2023, 9:10 AM

When libunwind is being built as part of the CMake build as is the case in your example (using -DLLVM_ENABLE_RUNTIMES="compiler-rt;libcxx;libcxxabi;libunwind"), you want GWP-ASan to link against the libunwind target from the same build, but that's not guaranteed even if you remove --unwindlib=none because there's no explicit dependency in the generated build graph. In fact, removing --unwindlib=none is potentially incorrect because your build may accidentally pick up libunwind from the host.

A correct solution is to keep --unwindlib=none, and to manually link libunwind from the same build. That's the solution we also use for libc++, see https://github.com/llvm/llvm-project/blob/b2bf995c02a04fd7f453d6931b79ce5a51871489/compiler-rt/CMakeLists.txt#L600. You'll want to use the same pattern here.

Ok, thanks for clarifying this.

There is just one thing that is still not clear to me: if there's no explicit dependency of Scudo or GWP-Asan on libunwind, wouldn't it be possible for libunwind to not have finished building when Scudo is being linked, causing a link error?

luporl edited the summary of this revision. (Show Details)Jan 31 2023, 8:37 AM

There is just one thing that is still not clear to me: if there's no explicit dependency of Scudo or GWP-Asan on libunwind, wouldn't it be possible for libunwind to not have finished building when Scudo is being linked, causing a link error?

I guess I found the answer.
Adding $<TARGET_LINKER_FILE:$<IF:$<TARGET_EXISTS:unwind_shared>,unwind_shared,unwind_static>> to SCUDO_LINK_LIBS also adds an implicit dependency to libunwind on build.ninja.

BTW, the previous Scudo fix doesn't work with this CMake config because COMPILER_RT_USE_LLVM_UNWINDER ends up being off, which results in an empty COMPILER_RT_UNWINDER_LINK_LIBS.

And indeed, removing --unwindlib=none was making the linker use -lgcc_s.

luporl updated this revision to Diff 494045.Feb 1 2023, 1:04 PM

Use built libunwind when possible.

This new diff correctly handles the following configurations:

  • COMPILER_RT_USE_LLVM_UNWINDER=ON
    • LIBUNWIND_ENABLE_STATIC=ON, LIBUNWIND_ENABLE_SHARED=OFF
    • LIBUNWIND_ENABLE_STATIC=OFF, LIBUNWIND_ENABLE_SHARED=ON
  • COMPILER_RT_USE_LLVM_UNWINDER=OFF
    • LIBUNWIND_ENABLE_STATIC=ON, LIBUNWIND_ENABLE_SHARED=OFF
    • LIBUNWIND_ENABLE_STATIC=OFF, LIBUNWIND_ENABLE_SHARED=ON
    • libunwind removed from LLVM_ENABLE_RUNTIMES

The built libunwind is always used, except when it's not actually built, then --unwindlib=none is removed, to use the host's unwind library, that is the only one available.

phosek added inline comments.Feb 2 2023, 1:18 AM
compiler-rt/lib/scudo/standalone/CMakeLists.txt
47

This is not necessary, in CMake you don't need to define variables beforehand.

52–58

The comment should be inside the elseif branch.

60–62

This could be a single generator expression, see https://github.com/llvm/llvm-project/blob/4f4b2161ec3cc42890aa9a445588d5a9a67e9033/compiler-rt/CMakeLists.txt#L596. I'm also not sure if we should prefer static over shared library.

DavidSpickett added inline comments.Feb 2 2023, 1:23 AM
compiler-rt/lib/scudo/standalone/CMakeLists.txt
51

I would remove the blank line here, then put the ARM related comment after the elseif line. So it's easier to see that this is an if/elseif construct.

64

Add a comment # Use the system unwind library., assuming that's what this does.

68

Say we are on AArch64, COMPILER_RT_USE_LLVM_UNWINDER is set ON. That means we're using llvm's unwinder.

Then we check if SCUDO_UNWINDLIB_NONE is ON and let's say it is, what effect does --unwindlib=none have? Does that flag just prevent another unwind library being linked on top of the one that COMPILER_RT_USE_LLVM_UNWINDER already added?

luporl updated this revision to Diff 494609.Feb 3 2023, 6:35 AM

Address reviewers' comments

luporl marked 5 inline comments as done.Feb 3 2023, 6:47 AM
luporl added inline comments.
compiler-rt/lib/scudo/standalone/CMakeLists.txt
60–62

Right, now I'm using the same generator expression as that of COMPILER_RT_UNWINDER_LINK_LIBS, that prefers shared over static library.

68

By comparing the link commands issued by clang++ with and without --unwindlib=none, yes, the only difference is that when --unwindlib=none is omitted the host's unwind library is also included.

When building with gcc in my test, -lgcc_s was the extra library linked. The resulting Scudo shared library then ended up with dependencies to both libunwind and libgcc_s.

luporl marked 3 inline comments as done.Feb 3 2023, 6:48 AM

@phosek, @DavidSpickett, is there anything else that needs to be done here?

Nothing from my side.

@DavidSpickett Its been while since no further comments on this rev. So I was wondering if you can kindly approve this patch. This is blocker for llvm 16.x armv7 release.
@luporl kindly port this to 16.x release branch once approved.

phosek added inline comments.Feb 26 2023, 12:21 PM
compiler-rt/lib/scudo/standalone/CMakeLists.txt
51–58

Could we use libgcc in this case (and report an error if it's unavailable)? We already have a variable to control whether to use LLVM libunwind or libgcc. This block silently picks up libunwind even when COMPILER_RT_USE_LLVM_UNWINDER isn't set which is unexpected.

luporl added inline comments.Feb 27 2023, 1:23 PM
compiler-rt/lib/scudo/standalone/CMakeLists.txt
51–58

We could, but what if llvm is built with a compiler that is not clang nor gcc, wouldn't it fail in this case?

Also, building with clang on a system where libgcc isn't installed would fail by default on ARM, while it would work on other architectures. In this case, it would be nice to suggest the use of COMPILER_RT_USE_LLVM_UNWINDER=ON when reporting the error.

I see the following options here:

  1. Link with libgcc if COMPILER_RT_USE_LLVM_UNWINDER is OFF, assuming it's ok to possibly break the build with other compilers. We could also try to detect the system unwind library and use it only when it's not libunwind, but this is probably not an easy task.
  2. Always use the system unwind library when COMPILER_RT_USE_LLVM_UNWINDER is OFF, which may end up using libunwind. We could also try to detect if the system unwind library is libunwind and report an error in this case, to preserve the expected behavior of COMPILER_RT_USE_LLVM_UNWINDER.

What do you think?

luporl added inline comments.Mar 14 2023, 1:41 PM
compiler-rt/lib/scudo/standalone/CMakeLists.txt
51–58

I've investigated building llvm-project, with compiler-rt enabled, with MSVC and with a clang that uses libc++ and libunwind by default.
With MSVC Scudo is not built. With clang it builds without errors, even if no unwind library is supplied.

So I'll just use libgcc here as suggested, to honor COMPILER_RT_USE_LLVM_UNWINDER. Hopefully this will unblock this patch and fix armv7 builds.

luporl updated this revision to Diff 505585.Mar 15 2023, 12:04 PM

Address reviewer's comments.

luporl marked an inline comment as done.Mar 15 2023, 12:06 PM

@phosek , do you still have any objection against this patch? If not, could you please approve it?

Hi I was wondering if we could fast track merge of this patch into LLVM main and 16.0.0 release branch. This patch fixes LLVM 16.0.0 release build for armv7 Linux and release is waiting on this patch to be merged.

Apparently patch looks in good shape so I was wondering if any one of the reviewers can press the accept button if there are no further objections.

MaskRay added a comment.EditedMar 23 2023, 4:24 PM

Hi I was wondering if we could fast track merge of this patch into LLVM main and 16.0.0 release branch. This patch fixes LLVM 16.0.0 release build for armv7 Linux and release is waiting on this patch to be merged.

Apparently patch looks in good shape so I was wondering if any one of the reviewers can press the accept button if there are no further objections.

Do you have CMake commands that scudo causes build breakage on Armv7?

Even if the build is fixed, scudo is likely never tested on AArch32. I suggest that we do the following for https://github.com/llvm/llvm-project/tree/release/16.x
https://github.com/MaskRay/llvm-project/tree/16.x-scudo)

--- i/compiler-rt/cmake/Modules/AllSupportedArchDefs.cmake
+++ w/compiler-rt/cmake/Modules/AllSupportedArchDefs.cmake
@@ -69,7 +69,7 @@ set(ALL_SAFESTACK_SUPPORTED_ARCH ${X86} ${X86_64} ${ARM64} ${MIPS32} ${MIPS64}
     ${HEXAGON} ${LOONGARCH64})
 set(ALL_CFI_SUPPORTED_ARCH ${X86} ${X86_64} ${ARM32} ${ARM64} ${MIPS64}
     ${HEXAGON})
-set(ALL_SCUDO_STANDALONE_SUPPORTED_ARCH ${X86} ${X86_64} ${ARM32} ${ARM64}
+set(ALL_SCUDO_STANDALONE_SUPPORTED_ARCH ${X86} ${X86_64} ${ARM64}
     ${MIPS32} ${MIPS64} ${PPC64} ${HEXAGON} ${LOONGARCH64} ${RISCV64})
 if(APPLE)
 set(ALL_XRAY_SUPPORTED_ARCH ${X86_64})

Likewise, I am unsure mips32/hexagon are really supported for scudo. Perhaps we can remove the two entries as well.

@luporl @DavidSpickett any thoughts on suggestion by @MaskRay?

DavidSpickett added a comment.EditedMar 24 2023, 2:43 AM

Do you have CMake commands that scudo causes build breakage on Armv7?

See the commit message and linked issue, I'm not sure how to reduce those to just scudo itself.

Even if the build is fixed, scudo is likely never tested on AArch32.

I'm not even sure what scudo is exactly. I have a vague idea that it is used for something in Android and that still supports Armv7, doesn't it? I suppose if it did you (Google) would be testing that yourselves anyway. Do we have a list of known users?

None of our bots have been building it. This one https://github.com/llvm/llvm-zorg/blob/e3a5b28f7fc6af8820441f0f11c6cded87f18acf/buildbot/osuosl/master/config/builders.py#L431 did but due to https://github.com/llvm/llvm-project/issues/38038 it currently doesn't. I don't know if that is still a problem.

How about we:

  • Land this.
  • Investigate the linked bug.
  • If it's still a problem we disable scudo for Arm 32 bit.
  • Otherwise we'll re-enable testing on that bot.

Scudo is used as a default system allocator on Android (it is also intended as the default allocator for LLVM libc) and it supports ARM32. @Chia-hungDuan do you know why this issue doesn't show up on Android?

compiler-rt/lib/scudo/standalone/CMakeLists.txt
49

I don't think this is the right condition. LLVM_NATIVE_ARCH defaults to host architecture where LLVM is being built, but that may be different from the architecture we're compiling compiler-rt for when cross-compiling.

We should likely be using if (CAN_TARGET_arm). Should this apply to both arm and armhf?

64

I'd probably something like No suitable unwinder library.

luporl updated this revision to Diff 508646.Mar 27 2023, 7:19 AM
luporl retitled this revision from [compiler-rt] Fix building GWPASAN on ARM to [compiler-rt] Fix scudo build on ARM.

Address reviewer's comments

luporl marked 2 inline comments as done.Mar 27 2023, 7:22 AM
luporl added inline comments.
compiler-rt/lib/scudo/standalone/CMakeLists.txt
49

We definitely want this for armhf too.

Scudo is used as a default system allocator on Android (it is also intended as the default allocator for LLVM libc) and it supports ARM32. @Chia-hungDuan do you know why this issue doesn't show up on Android?

Right, Scudo works on arm32 on Android. Android doesn't use cmake so I guess there's a proper unwind lib linked on arm32 Android in Android.bp when Scudo+gwp_asan is enabled. I didn't try it on arm32 Android, maybe @hctim can comment more.

hctim added a comment.Mar 30 2023, 8:46 AM

Right, Scudo works on arm32 on Android. Android doesn't use cmake so I guess there's a proper unwind lib linked on arm32 Android in Android.bp when Scudo+gwp_asan is enabled. I didn't try it on arm32 Android, maybe @hctim can comment more.

Yeah, I can't say I've tested arm32, and it seems like nobody else here has either. I can't say I have intentions to do it.

compiler-rt/lib/scudo/standalone/CMakeLists.txt
149

it seems like this is not needed for non-gwp-asan. it can be removed and merged (as i've suggested) under the COMPILER_RT_HAS_GWP_ASAN branch.

157

Maybe instead, just:

set(SCUDO_LINK_LIBS)

if (COMPILER_RT_HAS_GWP_ASAN)
  if(COMPILER_RT_USE_LLVM_UNWINDER)
    list(APPEND SCUDO_LINK_LIBS ${COMPILER_RT_UNWINDER_LINK_LIBS} dl)
  elseif (COMPILER_RT_HAS_GCC_S_LIB)
    list(APPEND SCUDO_LINK_LIBS gcc_s)
  elseif (COMPILER_RT_HAS_GCC_LIB)
    list(APPEND SCUDO_LINK_LIBS gcc)
  else()
    message(FATAL_ERROR "No suitable unwinder library")
  endif()

  ... other stuff already in this branch
endif()
luporl marked an inline comment as done.Mar 31 2023, 6:02 AM
luporl added inline comments.
compiler-rt/lib/scudo/standalone/CMakeLists.txt
157

Just to confirm, is it ok to make this change for all architectures?
Non-ARM32 architectures will start to get an an error when COMPILER_RT_USE_LLVM_UNWINDER is OFF and libgcc is not found, even if they don't need to link against an unwind library.

hctim added inline comments.Mar 31 2023, 12:07 PM
compiler-rt/lib/scudo/standalone/CMakeLists.txt
157

I'm kinda guessing that most people aren't missing an unwinder. Just checking the synthetic command line from a clang-link invocation on my machine, it just includes -lunwind by default.

So it's only the fancy-schmancy people who are using COMPILER_RT_USE_LLVM_UNWINDER who will get their own unwinder. Everyone else gets libunwind from gcc.

If people complain though, we can just disable GWP-ASan if there's no platform unwinder, and message(WARNING ... that it's happening.

luporl added inline comments.Apr 3 2023, 6:13 AM
compiler-rt/lib/scudo/standalone/CMakeLists.txt
157

Right, it makes sense. I'll proceed with the suggested changes, thanks!

luporl updated this revision to Diff 510556.Apr 3 2023, 10:51 AM

Address reviewers' comments

luporl marked 3 inline comments as done.Apr 3 2023, 10:53 AM
hctim accepted this revision.Apr 3 2023, 11:00 AM
This revision is now accepted and ready to land.Apr 3 2023, 11:00 AM
This revision was automatically updated to reflect the committed changes.

Hi. I suspect this broke our cross-compile arm builder at https://luci-milo.appspot.com/ui/p/fuchsia/builders/toolchain.ci/clang-linux-xarm64/b8784742626732805873/overview with:

-- Supported architectures for crt: aarch64
CMake Error at /b/s/w/ir/x/w/llvm-llvm-project/compiler-rt/lib/scudo/standalone/CMakeLists.txt:151 (message):
  No suitable unwinder library


-- Configuring incomplete, errors occurred!

Would you be able to take a look or revert?

Hi. I suspect this broke our cross-compile arm builder at https://luci-milo.appspot.com/ui/p/fuchsia/builders/toolchain.ci/clang-linux-xarm64/b8784742626732805873/overview with:

-- Supported architectures for crt: aarch64
CMake Error at /b/s/w/ir/x/w/llvm-llvm-project/compiler-rt/lib/scudo/standalone/CMakeLists.txt:151 (message):
  No suitable unwinder library


-- Configuring incomplete, errors occurred!

Would you be able to take a look or revert?

Sorry, I'll revert it and investigate what is causing this error.

luporl reopened this revision.Apr 11 2023, 5:51 AM
This revision is now accepted and ready to land.Apr 11 2023, 5:51 AM
luporl updated this revision to Diff 512414.Apr 11 2023, 5:51 AM

Fix Fuchsia AArch64 cross-compile

It turns out we were missing the case of when COMPILER_RT_USE_BUILTINS_LIBRARY is set.
In this case, COMPILER_RT_HAS_GCC_S_LIB/COMPILER_RT_HAS_GCC_LIB are not set, as libgcc is not needed.

Does anyone have any objections to this latest change?
I'm planning to reland it tomorrow.

Yes, sorry, I didn't know I had to file a ticket.