[Bug libstdc++/109882] New: -fsanitize=thread #include <memory> transitively includes sanitizer/common_interface_defs.h

[Bug libstdc++/109882] New: -fsanitize=thread #include <memory> transitively includes sanitizer/common_interface_defs.h

[ed at catmur dot uk]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109882

            Bug ID: 109882
           Summary: -fsanitize=thread #include <memory> transitively
                    includes sanitizer/common_interface_defs.h
           Product: gcc
           Version: 14.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: libstdc++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: ed at catmur dot uk
  Target Milestone: ---

Since 12.3.0, when compiled under tsan (-fsanitize=thread), #include <memory>
transitively includes sanitizer/common_interface_defs.h:

#include <memory>
#if defined SANITIZER_COMMON_INTERFACE_DEFS_H
#error :(
#endif

This is problematic for us because sanitizer/common_interface_defs.h has
#define __has_feature(x) 0
https://github.com/gcc-mirror/gcc/blame/5d85b5d649fff675ff00adcc99371bccf4ef5944/libsanitizer/include/sanitizer/common_interface_defs.h#L20

which makes our sanitizer detector incorrectly believe that __has_feature is
available and that tsan is not in fact present since
__has_feature(thread_sanitizer) evaluates to 0.

We will fix the order of checks to put `#ifdef __SANITIZE_THREAD__` first but
thought we should probably report this to help other users.

[Bug libstdc++/109882] -fsanitize=thread #include <memory> transitively includes sanitizer/common_interface_defs.h

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109882

--- Comment #1 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
__has_feature is being added ....

[Bug sanitizer/109882] -fsanitize=thread #include <memory> transitively includes sanitizer/common_interface_defs.h

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109882

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
          Component|libstdc++                   |sanitizer
                 CC|                            |dodji at gcc dot gnu.org,
                   |                            |dvyukov at gcc dot gnu.org,
                   |                            |jakub at gcc dot gnu.org,
                   |                            |kcc at gcc dot gnu.org,
                   |                            |marxin at gcc dot gnu.org

--- Comment #2 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Anyways I am going to declare this a bug in the sanitizer headers rather than
libstdc++ headers.

[Bug sanitizer/109882] -fsanitize=thread #include <memory> transitively includes sanitizer/common_interface_defs.h

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109882

--- Comment #3 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Then you should file this bug upstream with the sanitizer for the way
__has_feature is handled incorrectly.

[Bug sanitizer/109882] -fsanitize=thread #include <memory> transitively includes sanitizer/common_interface_defs.h

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109882

Jonathan Wakely <redi at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2023-05-16
     Ever confirmed|0                           |1

--- Comment #4 from Jonathan Wakely <redi at gcc dot gnu.org> ---
The header is included since r13-2677-g0abc63a5ea4550 (and
r12-9342-g0cec4e9cd44e25 on the branch), and not including it would mean not
being able to annotate atomic<shared_ptr<T>>.

I agree with Andrew, this is a bug in libsanitizer. The correct way to use
__has_feature in a public header is to test with #ifdef __has_feature not to
define the fallback definition.

[Bug sanitizer/109882] sanitizer/common_interface_defs.h bogusly defines __has_feature

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109882

--- Comment #5 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Libstdc++ itself does this:

#if __SANITIZE_THREAD__
#  define _GLIBCXX_TSAN 1
#elif defined __has_feature
# if __has_feature(thread_sanitizer)
#  define _GLIBCXX_TSAN 1
# endif
#endif

The sanitizers could do something similar, although it looks like they don't
actually need to. The only use of __has_feature in the public API is in
asan_interface.h and that could easily be replaced. Then __has_feature can be
redefined in the internal headers, which (I assume) aren't meant to be included
by user code.

Something like this (untested):

diff --git a/libsanitizer/include/sanitizer/asan_interface.h
b/libsanitizer/include/sanitizer/asan_interface.h
index 9bff21c117b..186269ad694 100644
--- a/libsanitizer/include/sanitizer/asan_interface.h
+++ b/libsanitizer/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/libsanitizer/include/sanitizer/common_interface_defs.h
b/libsanitizer/include/sanitizer/common_interface_defs.h
index 2f415bd9e85..2f9c83ef74e 100644
--- a/libsanitizer/include/sanitizer/common_interface_defs.h
+++ b/libsanitizer/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/libsanitizer/sanitizer_common/sanitizer_internal_defs.h
b/libsanitizer/sanitizer_common/sanitizer_internal_defs.h
index 98186c429e9..7574dce7f4a 100644
--- a/libsanitizer/sanitizer_common/sanitizer_internal_defs.h
+++ b/libsanitizer/sanitizer_common/sanitizer_internal_defs.h
@@ -14,6 +14,11 @@

 #include "sanitizer_platform.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

[Bug sanitizer/109882] sanitizer/common_interface_defs.h bogusly defines __has_feature

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109882

--- Comment #6 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Looks ok to me.  Now how to convince upstream to apply this? (Or we could keep
it as LOCAL_PATCHES.)

[Bug sanitizer/109882] sanitizer/common_interface_defs.h bogusly defines __has_feature

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109882

--- Comment #7 from Jonathan Wakely <redi at gcc dot gnu.org> ---
I'll do a little more testing and submit it upstream.

[Bug sanitizer/109882] sanitizer/common_interface_defs.h bogusly defines __has_feature

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109882

--- Comment #8 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Submitted upstream as https://reviews.llvm.org/D150866

[Bug sanitizer/109882] sanitizer/common_interface_defs.h bogusly defines __has_feature

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109882

--- Comment #9 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Resubmitted now that LLVM uses github pull requests:
https://github.com/llvm/llvm-project/pull/66628

[Bug sanitizer/109882] sanitizer/common_interface_defs.h bogusly defines __has_feature

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109882

--- Comment #10 from Jonathan Wakely <redi at gcc dot gnu.org> ---
The fix has been committed upstream now.

[Bug sanitizer/109882] sanitizer/common_interface_defs.h bogusly defines __has_feature

[sjames at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109882

Sam James <sjames at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           See Also|                            |https://github.com/llvm/llv
                   |                            |m-project/pull/66628

--- Comment #11 from Sam James <sjames at gcc dot gnu.org> ---
We could really do with a general sync of libsanitizer as well.

[Bug sanitizer/109882] sanitizer/common_interface_defs.h bogusly defines __has_feature

[sjames at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109882

--- Comment #12 from Sam James <sjames at gcc dot gnu.org> ---
(In reply to Sam James from comment #11)
> We could really do with a general sync of libsanitizer as well.

Done by jakub in r14-5487.

[Bug sanitizer/109882] sanitizer/common_interface_defs.h bogusly defines __has_feature

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109882

Jonathan Wakely <redi at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|---                         |FIXED

--- Comment #13 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Nice, that includes my fix, so we can close this.

[Bug sanitizer/109882] sanitizer/common_interface_defs.h bogusly defines __has_feature

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109882

--- Comment #14 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to Jonathan Wakely from comment #4)
> The header is included since r13-2677-g0abc63a5ea4550 (and
> r12-9342-g0cec4e9cd44e25 on the branch), and not including it would mean not
> being able to annotate atomic<shared_ptr<T>>.

Although it's still present on the branches. Do we want to fix it there too?

[Bug sanitizer/109882] sanitizer/common_interface_defs.h bogusly defines __has_feature

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109882

--- Comment #15 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
I think so, but for release branches obviously only as a cherry-pick of that
single change, not a merge.

[Bug sanitizer/109882] sanitizer/common_interface_defs.h bogusly defines __has_feature

[sjames at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109882

--- Comment #16 from Sam James <sjames at gcc dot gnu.org> ---
While at it, should do PR111057 too.

[Bug sanitizer/109882] sanitizer/common_interface_defs.h bogusly defines __has_feature

[acoplan at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109882
Bug 109882 depends on bug 60512, which changed state.

Bug 60512 Summary: would be useful if gcc implemented __has_feature similarly to clang
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=60512

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|---                         |FIXED