[compiler-rt] Fix scudo build on ARM

[luporl]

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

[DavidSpickett]

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..."

[luporl]

In D142888#4090559, @DavidSpickett wrote:

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]

• 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

[phosek]

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.

[luporl]

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]

In D142888#4093468, @luporl wrote:

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]

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]

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]

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]

Address reviewers' comments

[luporl]

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]

Ping

[luporl]

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

[DavidSpickett]

Nothing from my side.

[omjavaid]

@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]

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]

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]

Ping!

[luporl]

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]

Address reviewer's comments.

[luporl]

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

[omjavaid]

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]

In D142888#4217880, @omjavaid wrote:

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.

[omjavaid]

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

[DavidSpickett]

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.

[phosek]

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]

Address reviewer's comments

[luporl]

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

49

We definitely want this for armhf too.

[Chia-hungDuan]

In D142888#4220784, @phosek wrote:

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]

In D142888#4225117, @Chia-hungDuan wrote:

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

143–153

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()

155

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.

[luporl]

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

143–153

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]

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

143–153

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]

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

143–153

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

[luporl]

Address reviewers' comments

[leonardchan]

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?

[luporl]

In D142888#4243675, @leonardchan wrote:

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]

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.

[luporl]

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

[luporl]

Relanded: e1e972689b91

[tstellar]

@luporl Can you file a ticket next time for backports: https://llvm.org/docs/GitHub.html#backporting-fixes-to-the-release-branches

[luporl]

In D142888#4303770, @tstellar wrote:

@luporl Can you file a ticket next time for backports: https://llvm.org/docs/GitHub.html#backporting-fixes-to-the-release-branches

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

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

 set(SCUDO_LINK_FLAGS)
 
 list(APPEND SCUDO_LINK_FLAGS -Wl,-z,defs,-z,now,-z,relro)
 
 list(APPEND SCUDO_LINK_FLAGS -ffunction-sections -fdata-sections -Wl,--gc-sections)
 
 # We don't use the C++ standard library, so avoid including it by mistake.
 append_list_if(COMPILER_RT_HAS_NOSTDLIBXX_FLAG -nostdlib++ SCUDO_LINK_FLAGS)
 append_list_if(CXX_SUPPORTS_UNWINDLIB_NONE_FLAG --unwindlib=none SCUDO_LINK_FLAGS)
 
 if(COMPILER_RT_SCUDO_STANDALONE_SYSROOT_PATH)
   list(APPEND SCUDO_CFLAGS "--sysroot=${COMPILER_RT_SCUDO_STANDALONE_SYSROOT_PATH}")
 endif()
 
 if(ANDROID)
   list(APPEND SCUDO_CFLAGS -fno-emulated-tls)
 
 # Put the shared library in the global group. For more details, see
 # android-changes-for-ndk-developers.md#changes-to-library-search-order
   append_list_if(COMPILER_RT_HAS_Z_GLOBAL -Wl,-z,global SCUDO_LINK_FLAGS)
 endif()
 
 set(SCUDO_HEADERS
   allocator_config.h
   atomic_helpers.h
   bytemap.h
   checksum.h
   chunk.h
   combined.h
   common.h
   flags_parser.h
   flags.h
   fuchsia.h
   internal_defs.h
   linux.h
   list.h
   local_cache.h
   memtag.h
   mutex.h
   options.h
   platform.h
set(SCUDO_SOURCES_C_WRAPPERS
   wrappers_c.cpp
   )
 
 set(SCUDO_SOURCES_CXX_WRAPPERS
   wrappers_cpp.cpp
   )
 
 set(SCUDO_OBJECT_LIBS)
+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()
+
   add_dependencies(scudo_standalone gwp_asan)
   list(APPEND SCUDO_OBJECT_LIBS
        RTGwpAsan RTGwpAsanBacktraceLibc RTGwpAsanSegvHandler
        RTGwpAsanOptionsParser)
 
   append_list_if(COMPILER_RT_HAS_OMIT_FRAME_POINTER_FLAG -fno-omit-frame-pointer
                                                          -mno-omit-leaf-frame-pointer
                  SCUDO_CFLAGS)
   list(APPEND SCUDO_CFLAGS -DGWP_ASAN_HOOKS)
 
 endif()
 
-set(SCUDO_LINK_LIBS ${COMPILER_RT_UNWINDER_LINK_LIBS})
-
 if(COMPILER_RT_BUILD_SCUDO_STANDALONE_WITH_LLVM_LIBC)
   include_directories(${COMPILER_RT_BINARY_DIR}/../libc/include/)
 
   set(SCUDO_DEPS libc-headers)
 
   list(APPEND SCUDO_CFLAGS "-ffreestanding")
 endif()