[sanitizers] Do not define __has_feature in sanitizer/common_interface_defs.h

[jwakely]

Public headers intended for user code should not define __has_feature, because this can break preprocessor checks done later in user code, e.g. if they test #ifdef __has_feature to check for real support in the compiler.

Replace the only use in the public header with a check for it being supported before trying to use it. Define the fallback definition in the internal headers, so that other internal sanitizer headers can continue to use it as preferred.

This resolves a bug reported to GCC as https://gcc.gnu.org/PR109882

[llvmbot]

@llvm/pr-subscribers-compiler-rt-sanitizer

Changes

Public headers intended for user code should not define __has_feature, because this can break preprocessor checks done later in user code, e.g. if they test #ifdef __has_feature to check for real support in the compiler.

Replace the only use in the public header with a check for it being supported before trying to use it. Define the fallback definition in the internal headers, so that other internal sanitizer headers can continue to use it as preferred.

This resolves a bug reported to GCC as https://gcc.gnu.org/PR109882

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

3 Files Affected:

• (modified) compiler-rt/include/sanitizer/asan_interface.h (+10-1)
• (modified) compiler-rt/include/sanitizer/common_interface_defs.h (-5)
• (modified) compiler-rt/lib/sanitizer_common/sanitizer_internal_defs.h (+5)

diff --git a/compiler-rt/include/sanitizer/asan_interface.h b/compiler-rt/include/sanitizer/asan_interface.h
index 9bff21c117b39a2..186269ad69448bd 100644
--- a/compiler-rt/include/sanitizer/asan_interface.h
+++ b/compiler-rt/include/sanitizer/asan_interface.h
@@ -48,7 +48,15 @@ void __asan_poison_memory_region(void const volatile *addr, size_t size);
 void __asan_unpoison_memory_region(void const volatile *addr, size_t size);
 
 // Macros provided for convenience.
-#if __has_feature(address_sanitizer) || defined(__SANITIZE_ADDRESS__)
+#ifdef __has_feature
+#if __has_feature(address_sanitizer)
+#define ASAN_DEFINE_REGION_MACROS
+#endif
+#elif defined(__SANITIZE_ADDRESS__)
+#define ASAN_DEFINE_REGION_MACROS
+#endif
+
+#ifdef ASAN_DEFINE_REGION_MACROS
 /// Marks a memory region as unaddressable.
 ///
 /// \note Macro provided for convenience; defined as a no-op if ASan is not
@@ -74,6 +82,7 @@ void __asan_unpoison_memory_region(void const volatile *addr, size_t size);
 #define ASAN_UNPOISON_MEMORY_REGION(addr, size) \
   ((void)(addr), (void)(size))
 #endif
+#undef ASAN_DEFINE_REGION_MACROS
 
 /// Checks if an address is poisoned.
 ///
diff --git a/compiler-rt/include/sanitizer/common_interface_defs.h b/compiler-rt/include/sanitizer/common_interface_defs.h
index 983df7cea16ed90..6078d695a4a601e 100644
--- a/compiler-rt/include/sanitizer/common_interface_defs.h
+++ b/compiler-rt/include/sanitizer/common_interface_defs.h
@@ -15,11 +15,6 @@
 #include <stddef.h>
 #include <stdint.h>
 
-// GCC does not understand __has_feature.
-#if !defined(__has_feature)
-#define __has_feature(x) 0
-#endif
-
 #ifdef __cplusplus
 extern "C" {
 #endif
diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_internal_defs.h b/compiler-rt/lib/sanitizer_common/sanitizer_internal_defs.h
index 552d65067944f5b..3809669dd48bb3b 100644
--- a/compiler-rt/lib/sanitizer_common/sanitizer_internal_defs.h
+++ b/compiler-rt/lib/sanitizer_common/sanitizer_internal_defs.h
@@ -15,6 +15,11 @@
 #include "sanitizer_platform.h"
 #include "sanitizer_redefine_builtins.h"
 
+// GCC does not understand __has_feature.
+#if !defined(__has_feature)
+#define __has_feature(x) 0
+#endif
+
 #ifndef SANITIZER_DEBUG
 # define SANITIZER_DEBUG 0
 #endif

[jwakely]

Previously submitted as https://reviews.llvm.org/D150866

[fmayer]

Either I am missing something or there is a second #endif missing below line 53.

[jwakely]

The #ifdef on line 51 starts a group that ends with the #elif on line 55, which starts a new group that ends with the #endif on line 57.

The #if on line 51 starts a group that ends with the #endif on line 54.

Would it be clearer like this?

#ifdef __has_feature
# if __has_feature(address_sanitizer)
#  define ASAN_DEFINE_REGION_MACROS
# endif
#elif defined(__SANITIZE_ADDRESS__)
# define ASAN_DEFINE_REGION_MACROS
#endif

That's how I would format it in my own code, but that doesn't seem to be the prevailing style in the sanitizers.

[fmayer]

Ah, my fault sorry. I missed that the second is elif and not if :)

[fmayer]

Ah, my fault sorry. I missed that the second is elif and not if :)

[jwakely]

Thanks for the approval. Is there somebody I should ping to get this merged?

[github-actions]

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:

git-clang-format --diff 1d95a071d6fc43fb413a0d3f5a9d1e52a18abab0 470f467d5c357b51ff2961e84575a2713f5c5a1c -- compiler-rt/include/sanitizer/asan_interface.h compiler-rt/include/sanitizer/common_interface_defs.h compiler-rt/lib/sanitizer_common/sanitizer_internal_defs.h

View the diff from clang-format here.

diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_internal_defs.h b/compiler-rt/lib/sanitizer_common/sanitizer_internal_defs.h
index 3809669dd..62a78e528 100644
--- a/compiler-rt/lib/sanitizer_common/sanitizer_internal_defs.h
+++ b/compiler-rt/lib/sanitizer_common/sanitizer_internal_defs.h
@@ -17,7 +17,7 @@
 
 // GCC does not understand __has_feature.
 #if !defined(__has_feature)
-#define __has_feature(x) 0
+#  define __has_feature(x) 0
 #endif
 
 #ifndef SANITIZER_DEBUG