Skip to content
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

Allow multilibs to always set _LIBCPP_INSTRUMENTED_WITH_ASAN #90291

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

PiJoules
Copy link
Contributor

If a toolchain uses the multilib layout for different instrumented binaries, it's possible to run into nondeterministic behavior when generating __config_site because there's only one __config_site per target triple and doesn't consider multilibs. For example, both an unininstrumented libcxx and ASAN-instrumented libcxx for riscv64-unknown-fuchsia will write to the exact same __config_site. Depending on which is the last step to execute, the __config_site could come from the uninstrumented multilib build which means _LIBCPP_INSTRUMENTED_WITH_ASAN won't be defined which can result in false positives when doing container overflow checks. Ideally, each multilib would have its own unique __config_site and clang would know which __config_site to use. This option allows multilib users to just always have this #cmakedefine set. This is fine for multilib users because clang will know which runtime to used based on sanitizer flags. Likewise, headers that check if container annotations are available are gated on __has_feature(address_sanitizer).

If a toolchain uses the multilib layout for different instrumented binaries,
it's possible to run into nondeterministic behavior when generating __config_site
because there's only one __config_site per target triple and doesn't consider
multilibs. For example, both an unininstrumented libcxx and ASAN-instrumented
libcxx for riscv64-unknown-fuchsia will write to the exact same __config_site.
Depending on which is the last step to execute, the __config_site could come
from the uninstrumented multilib build which means _LIBCPP_INSTRUMENTED_WITH_ASAN
won't be defined which can result in false positives when doing container overflow
checks. Ideally, each multilib would have its own unique __config_site and
clang would know which __config_site to use. This option allows multilib users
to just always have this #cmakedefine set. This is fine for multilib users because
clang will know which runtime to used based on sanitizer flags. Likewise, headers
that check if container annotations are available are gated on __has_feature(address_sanitizer).
@PiJoules PiJoules requested a review from petrhosek April 26, 2024 23:02
@PiJoules PiJoules marked this pull request as ready for review April 29, 2024 18:55
@PiJoules PiJoules requested a review from a team as a code owner April 29, 2024 18:55
@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Apr 29, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Apr 29, 2024

@llvm/pr-subscribers-libcxx

Author: None (PiJoules)

Changes

If a toolchain uses the multilib layout for different instrumented binaries, it's possible to run into nondeterministic behavior when generating __config_site because there's only one __config_site per target triple and doesn't consider multilibs. For example, both an unininstrumented libcxx and ASAN-instrumented libcxx for riscv64-unknown-fuchsia will write to the exact same __config_site. Depending on which is the last step to execute, the __config_site could come from the uninstrumented multilib build which means _LIBCPP_INSTRUMENTED_WITH_ASAN won't be defined which can result in false positives when doing container overflow checks. Ideally, each multilib would have its own unique __config_site and clang would know which __config_site to use. This option allows multilib users to just always have this #cmakedefine set. This is fine for multilib users because clang will know which runtime to used based on sanitizer flags. Likewise, headers that check if container annotations are available are gated on __has_feature(address_sanitizer).


Full diff: https://github.com/llvm/llvm-project/pull/90291.diff

1 Files Affected:

  • (modified) libcxx/CMakeLists.txt (+19-1)
diff --git a/libcxx/CMakeLists.txt b/libcxx/CMakeLists.txt
index 2977c26646cb2e..ee655d1d4bbab1 100644
--- a/libcxx/CMakeLists.txt
+++ b/libcxx/CMakeLists.txt
@@ -329,6 +329,23 @@ endif()
 option(LIBCXX_HERMETIC_STATIC_LIBRARY
   "Do not export any symbols from the static library." ${LIBCXX_HERMETIC_STATIC_LIBRARY_DEFAULT})
 
+# If a toolchain uses the multilib layout for different instrumented binaries,
+# it's possible to run into nondeterministic behavior when generating __config_site
+# because there's only one __config_site per target triple and doesn't consider
+# multilibs. For example, both an unininstrumented libcxx and ASAN-instrumented
+# libcxx for riscv64-unknown-fuchsia will write to the exact same __config_site.
+# Depending on which is the last step to execute, the __config_site could come
+# from the uninstrumented multilib build which means _LIBCPP_INSTRUMENTED_WITH_ASAN
+# won't be defined which can result in false positives when doing container overflow
+# checks. Ideally, each multilib would have its own unique __config_site and
+# clang would know which __config_site to use. This option allows multilib users
+# to just always have this #cmakedefine set. This is fine for multilib users because
+# clang will know which runtime to used based on sanitizer flags. Likewise, headers
+# that check if container annotations are available are gated on __has_feature(address_sanitizer).
+option(LIBCXX_FORCE_LIBCPP_INSTRUMENTED_WITH_ASAN_FOR_MULTILIBS
+       "For multilib toolchains, always ensure _LIBCPP_INSTRUMENTED_WITH_ASAN is set."
+       OFF)
+
 #===============================================================================
 # Check option configurations
 #===============================================================================
@@ -663,7 +680,8 @@ target_compile_options(cxx-sanitizer-flags INTERFACE ${SANITIZER_FLAGS})
 # will not be compiled into it, resulting in false positives.
 # For context, read: https://github.com/llvm/llvm-project/pull/72677#pullrequestreview-1765402800
 string(FIND "${LLVM_USE_SANITIZER}" "Address" building_with_asan)
-if (NOT "${building_with_asan}" STREQUAL "-1")
+if ((NOT "${building_with_asan}" STREQUAL "-1") OR
+    ${LIBCXX_FORCE_LIBCPP_INSTRUMENTED_WITH_ASAN_FOR_MULTILIBS})
   config_define(ON _LIBCPP_INSTRUMENTED_WITH_ASAN)
 endif()
 

@ldionne ldionne self-assigned this Apr 30, 2024
Copy link
Member

@ldionne ldionne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would much much prefer if we solved the underlying problem and ensured that each multilib config has its own __config_site instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants