Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Avoid migrating existing patches. Phabricator shutdown timeline

Differential D142888 Diff 512414 compiler-rt/lib/scudo/standalone/CMakeLists.txt

Changeset View

Standalone View

View Options

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

Show All 35 Lines
set(SCUDO_LINK_FLAGS) set(SCUDO_LINK_FLAGS)
list(APPEND SCUDO_LINK_FLAGS -Wl,-z,defs,-z,now,-z,relro) list(APPEND SCUDO_LINK_FLAGS -Wl,-z,defs,-z,now,-z,relro)
list(APPEND SCUDO_LINK_FLAGS -ffunction-sections -fdata-sections -Wl,--gc-sections) 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. # 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(COMPILER_RT_HAS_NOSTDLIBXX_FLAG -nostdlib++ SCUDO_LINK_FLAGS)
append_list_if(CXX_SUPPORTS_UNWINDLIB_NONE_FLAG --unwindlib=none SCUDO_LINK_FLAGS) append_list_if(CXX_SUPPORTS_UNWINDLIB_NONE_FLAG --unwindlib=none SCUDO_LINK_FLAGS)
DavidSpickettUnsubmitted
Done
ReplyInline Actions

Can you include the why here as well.

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

DavidSpickett: Can you include the why here as well. "This is because on ARM unwinding does/does not whatever.
if(COMPILER_RT_SCUDO_STANDALONE_SYSROOT_PATH) if(COMPILER_RT_SCUDO_STANDALONE_SYSROOT_PATH)
list(APPEND SCUDO_CFLAGS "--sysroot=${COMPILER_RT_SCUDO_STANDALONE_SYSROOT_PATH}") list(APPEND SCUDO_CFLAGS "--sysroot=${COMPILER_RT_SCUDO_STANDALONE_SYSROOT_PATH}")
phosekUnsubmitted
Done
ReplyInline Actions

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

phosek: This is not necessary, in CMake you don't need to define variables beforehand.
endif() endif()
phosekUnsubmitted
Done
ReplyInline Actions

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?

phosek: I don't think this is the right condition. `LLVM_NATIVE_ARCH` defaults to host architecture…
luporlAuthorUnsubmitted
Done
ReplyInline Actions

We definitely want this for armhf too.

luporl: We definitely want this for `armhf` too.
if(ANDROID) if(ANDROID)
list(APPEND SCUDO_CFLAGS -fno-emulated-tls) list(APPEND SCUDO_CFLAGS -fno-emulated-tls)
DavidSpickettUnsubmitted
Done
ReplyInline Actions

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.

DavidSpickett: I would remove the blank line here, then put the ARM related comment after the elseif line. So…
# Put the shared library in the global group. For more details, see # Put the shared library in the global group. For more details, see
# android-changes-for-ndk-developers.md#changes-to-library-search-order # 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) append_list_if(COMPILER_RT_HAS_Z_GLOBAL -Wl,-z,global SCUDO_LINK_FLAGS)
endif() endif()
set(SCUDO_HEADERS set(SCUDO_HEADERS
phosekUnsubmitted
Done
ReplyInline Actions
list(APPEND SCUDO_LINK_LIBS ${COMPILER_RT_UNWINDER_LINK_LIBS} dl)
- # On ARM, we must link against an unwind lib when GWPAsan is used, otherwise
- # we get undefined references to __aeabi_unwind_cpp_pr* symbols.
- # This is because object files compiled with -funwind-tables end up referencing
- # these symbols on ARM, while on other architectures, such as x86_64 and
- # AArch64, only the .eh_frame section is added, but no references to an unwind
- # library are made.
- elseif(COMPILER_RT_HAS_GWP_ASAN AND "${LLVM_NATIVE_ARCH}" STREQUAL "ARM")
+ elseif(COMPILER_RT_HAS_GWP_ASAN AND "${LLVM_NATIVE_ARCH}" STREQUAL "ARM")
+ # On ARM, we must link against an unwind lib when GWPAsan is used, otherwise
+ # we get undefined references to __aeabi_unwind_cpp_pr* symbols.
+ # This is because object files compiled with -funwind-tables end up referencing
+ # these symbols on ARM, while on other architectures, such as x86_64 and
+ # AArch64, only the .eh_frame section is added, but no references to an unwind
+ # library are made.
if(LIBUNWIND_ENABLE_STATIC)

The comment should be inside the elseif branch.

phosek: The comment should be inside the `elseif` branch.
phosekUnsubmitted
Done
ReplyInline Actions

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.

phosek: Could we use libgcc in this case (and report an error if it's unavailable)? We already have a…
luporlAuthorUnsubmitted
Done
ReplyInline Actions

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: We could, but what if llvm is built with a compiler that is not clang nor gcc, wouldn't it fail…
luporlAuthorUnsubmitted
Done
ReplyInline Actions

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: I've investigated building llvm-project, with compiler-rt enabled, with MSVC and with a clang…
allocator_config.h allocator_config.h
atomic_helpers.h atomic_helpers.h
bytemap.h bytemap.h
checksum.h checksum.h
phosekUnsubmitted
Done
ReplyInline Actions

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.

phosek: This could be a single generator expression, see https://github.com/llvm/llvm…
luporlAuthorUnsubmitted
Done
ReplyInline Actions

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

luporl: Right, now I'm using the same generator expression as that of `COMPILER_RT_UNWINDER_LINK_LIBS`…
chunk.h chunk.h
combined.h combined.h
DavidSpickettUnsubmitted
Done
ReplyInline Actions

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

DavidSpickett: Add a comment `# Use the system unwind library.`, assuming that's what this does.
phosekUnsubmitted
Done
ReplyInline Actions

I'd probably something like No suitable unwinder library.

phosek: I'd probably something like `No suitable unwinder library`.
common.h common.h
flags_parser.h flags_parser.h
flags.h flags.h
fuchsia.h fuchsia.h
DavidSpickettUnsubmitted
Done
ReplyInline Actions

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?

DavidSpickett: Say we are on AArch64, COMPILER_RT_USE_LLVM_UNWINDER is set ON. That means we're using llvm's…
luporlAuthorUnsubmitted
Done
ReplyInline Actions

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: By comparing the link commands issued by `clang++` with and without `--unwindlib=none`, yes…
internal_defs.h internal_defs.h
linux.h linux.h
list.h list.h
local_cache.h local_cache.h
memtag.h memtag.h
mutex.h mutex.h
options.h options.h
platform.h platform.h
▲ Show 20 Lines • Show All 50 Lines • ▼ Show 20 Linesset(SCUDO_SOURCES_C_WRAPPERS
wrappers_c.cpp wrappers_c.cpp
) )
set(SCUDO_SOURCES_CXX_WRAPPERS set(SCUDO_SOURCES_CXX_WRAPPERS
wrappers_cpp.cpp wrappers_cpp.cpp
) )
set(SCUDO_OBJECT_LIBS) set(SCUDO_OBJECT_LIBS)
set(SCUDO_LINK_LIBS)
if (COMPILER_RT_HAS_GWP_ASAN) 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)
elseif (NOT COMPILER_RT_USE_BUILTINS_LIBRARY)
message(FATAL_ERROR "No suitable unwinder library")
endif()
hctimUnsubmitted
Done
ReplyInline Actions

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()
hctim: Maybe instead, just: ``` set(SCUDO_LINK_LIBS) if (COMPILER_RT_HAS_GWP_ASAN) if…
luporlAuthorUnsubmitted
Done
ReplyInline Actions

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.

luporl: Just to confirm, is it ok to make this change for all architectures? Non-ARM32 architectures…
hctimUnsubmitted
Done
ReplyInline Actions

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.

hctim: I'm kinda guessing that most people aren't missing an unwinder. Just checking the synthetic…
luporlAuthorUnsubmitted
Done
ReplyInline Actions

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

luporl: Right, it makes sense. I'll proceed with the suggested changes, thanks!
add_dependencies(scudo_standalone gwp_asan) add_dependencies(scudo_standalone gwp_asan)
list(APPEND SCUDO_OBJECT_LIBS list(APPEND SCUDO_OBJECT_LIBS
RTGwpAsan RTGwpAsanBacktraceLibc RTGwpAsanSegvHandler RTGwpAsan RTGwpAsanBacktraceLibc RTGwpAsanSegvHandler
RTGwpAsanOptionsParser) RTGwpAsanOptionsParser)
append_list_if(COMPILER_RT_HAS_OMIT_FRAME_POINTER_FLAG -fno-omit-frame-pointer append_list_if(COMPILER_RT_HAS_OMIT_FRAME_POINTER_FLAG -fno-omit-frame-pointer
-mno-omit-leaf-frame-pointer -mno-omit-leaf-frame-pointer
SCUDO_CFLAGS) SCUDO_CFLAGS)
list(APPEND SCUDO_CFLAGS -DGWP_ASAN_HOOKS) list(APPEND SCUDO_CFLAGS -DGWP_ASAN_HOOKS)
endif() endif()
set(SCUDO_LINK_LIBS ${COMPILER_RT_UNWINDER_LINK_LIBS})
hctimUnsubmitted
Done
ReplyInline Actions

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.

hctim: it seems like this is not needed for non-gwp-asan. it can be removed and merged (as i've…
if(COMPILER_RT_BUILD_SCUDO_STANDALONE_WITH_LLVM_LIBC) if(COMPILER_RT_BUILD_SCUDO_STANDALONE_WITH_LLVM_LIBC)
include_directories(${COMPILER_RT_BINARY_DIR}/../libc/include/) include_directories(${COMPILER_RT_BINARY_DIR}/../libc/include/)
set(SCUDO_DEPS libc-headers) set(SCUDO_DEPS libc-headers)
list(APPEND SCUDO_CFLAGS "-ffreestanding") list(APPEND SCUDO_CFLAGS "-ffreestanding")
endif() endif()
▲ Show 20 Lines • Show All 65 Lines • Show Last 20 Lines