Anna Zaks via llvm-dev
2016-Jul-01 20:03 UTC
[llvm-dev] How to resolve conflicts between sanitizer_common and system headers
> On Jul 1, 2016, at 12:10 PM, Dmitry Vyukov <dvyukov at google.com> wrote: > > On Fri, Jul 1, 2016 at 8:53 PM, Anna Zaks <ganna at apple.com> wrote: >> Hi Sanitizer Runtime Developers, >> >> We recently ran into a problem building clang because some of the >> definitions in sanitizer_common conflicted with system definitions and later >> another system header was trying to use the system definition: >> >> .../usr/include/libkern/OSAtomicDeprecated.h:756:17: error: reference to >> 'memory_order_relaxed' is ambiguous >> __theAmount, memory_order_relaxed) + __theAmount); >> ^ >> .../usr/bin/../include/c++/v1/atomic:548:5: note: candidate found by name >> lookup is 'std::__1::memory_order::memory_order_relaxed' >> memory_order_relaxed, memory_order_consume, memory_order_acquire, >> ^ >> ../src/projects/compiler-rt/lib/tsan/../sanitizer_common/sanitizer_atomic.h:22:3: >> note: candidate found by name lookup is >> '__sanitizer::memory_order::memory_order_relaxed' >> memory_order_relaxed = 1 << 0, >> ^ >> >> >> The problem is due to the combination of the following: >> 1. The runtime includes the system headers after the project headers (as >> per LLVM coding guidelines). >> 2. lib/sanitizer_common/sanitizer_internal_defs.h pollutes the namespace of >> everything defined after it, which is all/most of the sanitizer .h and .cc >> files and the included system headers with: >> using namespace __sanitizer; // NOLINT >> 3. These are the definitions that conflict in this particular case, but >> this problem could reoccur in the future with other symbols as well: >> >> enum memory_order { >> memory_order_relaxed = 1 << 0, >> memory_order_consume = 1 << 1, >> memory_order_acquire = 1 << 2, >> memory_order_release = 1 << 3, >> memory_order_acq_rel = 1 << 4, >> memory_order_seq_cst = 1 << 5 >> }; >> >> >> We currently have a workaround (in the system header) that makes this >> non-blocking, but it would be good to cleanly address this problem. Removing >> the "using namespace" from the header seems like the cleanest solution. >> WDYT? >> >> Thanks, >> Anna. > > > Hi Anna, > > What does OSAtomicDeprecated.h do? Does it also pull > memory_order_relaxed into global namespace?The header used to use std namespace locally before accessing those symbols, which caused the error when building clang: #define _OSATOMIC_USING_NAMESPACE_STD() using namespace std ... { _OSATOMIC_USING_NAMESPACE_STD(); return (atomic_fetch_add_explicit((volatile _OSAtomic_int32_t*) __theValue, __theAmount, memory_order_relaxed) + __theAmount); } They worked around the problem by qualifying the symbol with "std::": #define OSATOMIC_STD(_a) std::_a ... { return (OSATOMIC_STD(atomic_fetch_add_explicit)( (volatile _OSAtomic_int32_t*) __theValue, __theAmount, OSATOMIC_STD(memory_order_relaxed)) + __theAmount); } There are quite a few of these conflicts so it made the system header uglier. On the other hand, we do have a workaround. It would be great to have a general solution that would prevent this issue from happening in the future. + Bob, who reported the problem. Anna.> > For this particular problem we could just rename sanitizer internal constants. > > Maybe doing something like "namespace __tsan { using namespace > __sanitizer_common; }" will help to resolve it in general case. But I > am not sure, need to know what system headers do.
Dmitry Vyukov via llvm-dev
2016-Jul-01 20:18 UTC
[llvm-dev] How to resolve conflicts between sanitizer_common and system headers
On Fri, Jul 1, 2016 at 10:03 PM, Anna Zaks <ganna at apple.com> wrote:> >> On Jul 1, 2016, at 12:10 PM, Dmitry Vyukov <dvyukov at google.com> wrote: >> >> On Fri, Jul 1, 2016 at 8:53 PM, Anna Zaks <ganna at apple.com> wrote: >>> Hi Sanitizer Runtime Developers, >>> >>> We recently ran into a problem building clang because some of the >>> definitions in sanitizer_common conflicted with system definitions and later >>> another system header was trying to use the system definition: >>> >>> .../usr/include/libkern/OSAtomicDeprecated.h:756:17: error: reference to >>> 'memory_order_relaxed' is ambiguous >>> __theAmount, memory_order_relaxed) + __theAmount); >>> ^ >>> .../usr/bin/../include/c++/v1/atomic:548:5: note: candidate found by name >>> lookup is 'std::__1::memory_order::memory_order_relaxed' >>> memory_order_relaxed, memory_order_consume, memory_order_acquire, >>> ^ >>> ../src/projects/compiler-rt/lib/tsan/../sanitizer_common/sanitizer_atomic.h:22:3: >>> note: candidate found by name lookup is >>> '__sanitizer::memory_order::memory_order_relaxed' >>> memory_order_relaxed = 1 << 0, >>> ^ >>> >>> >>> The problem is due to the combination of the following: >>> 1. The runtime includes the system headers after the project headers (as >>> per LLVM coding guidelines). >>> 2. lib/sanitizer_common/sanitizer_internal_defs.h pollutes the namespace of >>> everything defined after it, which is all/most of the sanitizer .h and .cc >>> files and the included system headers with: >>> using namespace __sanitizer; // NOLINT >>> 3. These are the definitions that conflict in this particular case, but >>> this problem could reoccur in the future with other symbols as well: >>> >>> enum memory_order { >>> memory_order_relaxed = 1 << 0, >>> memory_order_consume = 1 << 1, >>> memory_order_acquire = 1 << 2, >>> memory_order_release = 1 << 3, >>> memory_order_acq_rel = 1 << 4, >>> memory_order_seq_cst = 1 << 5 >>> }; >>> >>> >>> We currently have a workaround (in the system header) that makes this >>> non-blocking, but it would be good to cleanly address this problem. Removing >>> the "using namespace" from the header seems like the cleanest solution. >>> WDYT? >>> >>> Thanks, >>> Anna. >> >> >> Hi Anna, >> >> What does OSAtomicDeprecated.h do? Does it also pull >> memory_order_relaxed into global namespace? > > The header used to use std namespace locally before accessing those symbols, which caused the error when building clang: > > #define _OSATOMIC_USING_NAMESPACE_STD() using namespace std > ... > { > _OSATOMIC_USING_NAMESPACE_STD(); > return (atomic_fetch_add_explicit((volatile _OSAtomic_int32_t*) __theValue, > __theAmount, memory_order_relaxed) + __theAmount); > } > > They worked around the problem by qualifying the symbol with "std::": > #define OSATOMIC_STD(_a) std::_a > ... > { > return (OSATOMIC_STD(atomic_fetch_add_explicit)( > (volatile _OSAtomic_int32_t*) __theValue, __theAmount, > OSATOMIC_STD(memory_order_relaxed)) + __theAmount); > } > > There are quite a few of these conflicts so it made the system header uglier. On the other hand, we do have a workaround. > > It would be great to have a general solution that would prevent this issue from happening in the future. > > + Bob, who reported the problem. > > Anna. >> >> For this particular problem we could just rename sanitizer internal constants. >> >> Maybe doing something like "namespace __tsan { using namespace >> __sanitizer_common; }" will help to resolve it in general case. But I >> am not sure, need to know what system headers do.For such case we indeed can change using namespace __sanitizer_common; to: namespace __tsan { using namespace __sanitizer_common; } Here is my litmus test. Fails without -DFIX, compiles successfully with -DFIX. namespace sanitizer { const int FOO = 1; } #ifdef FIX namespace tsan { using namespace sanitizer; } #else using namespace sanitizer; #endif namespace cxx { const int FOO = 1; } namespace tsan { const int BAR = FOO; } int main() { using namespace cxx; const int BAR = FOO; return 0; }
Anna Zaks via llvm-dev
2016-Jul-01 20:32 UTC
[llvm-dev] How to resolve conflicts between sanitizer_common and system headers
> On Jul 1, 2016, at 1:18 PM, Dmitry Vyukov <dvyukov at google.com> wrote: > > On Fri, Jul 1, 2016 at 10:03 PM, Anna Zaks <ganna at apple.com <mailto:ganna at apple.com>> wrote: >> >>> On Jul 1, 2016, at 12:10 PM, Dmitry Vyukov <dvyukov at google.com> wrote: >>> >>> On Fri, Jul 1, 2016 at 8:53 PM, Anna Zaks <ganna at apple.com> wrote: >>>> Hi Sanitizer Runtime Developers, >>>> >>>> We recently ran into a problem building clang because some of the >>>> definitions in sanitizer_common conflicted with system definitions and later >>>> another system header was trying to use the system definition: >>>> >>>> .../usr/include/libkern/OSAtomicDeprecated.h:756:17: error: reference to >>>> 'memory_order_relaxed' is ambiguous >>>> __theAmount, memory_order_relaxed) + __theAmount); >>>> ^ >>>> .../usr/bin/../include/c++/v1/atomic:548:5: note: candidate found by name >>>> lookup is 'std::__1::memory_order::memory_order_relaxed' >>>> memory_order_relaxed, memory_order_consume, memory_order_acquire, >>>> ^ >>>> ../src/projects/compiler-rt/lib/tsan/../sanitizer_common/sanitizer_atomic.h:22:3: >>>> note: candidate found by name lookup is >>>> '__sanitizer::memory_order::memory_order_relaxed' >>>> memory_order_relaxed = 1 << 0, >>>> ^ >>>> >>>> >>>> The problem is due to the combination of the following: >>>> 1. The runtime includes the system headers after the project headers (as >>>> per LLVM coding guidelines). >>>> 2. lib/sanitizer_common/sanitizer_internal_defs.h pollutes the namespace of >>>> everything defined after it, which is all/most of the sanitizer .h and .cc >>>> files and the included system headers with: >>>> using namespace __sanitizer; // NOLINT >>>> 3. These are the definitions that conflict in this particular case, but >>>> this problem could reoccur in the future with other symbols as well: >>>> >>>> enum memory_order { >>>> memory_order_relaxed = 1 << 0, >>>> memory_order_consume = 1 << 1, >>>> memory_order_acquire = 1 << 2, >>>> memory_order_release = 1 << 3, >>>> memory_order_acq_rel = 1 << 4, >>>> memory_order_seq_cst = 1 << 5 >>>> }; >>>> >>>> >>>> We currently have a workaround (in the system header) that makes this >>>> non-blocking, but it would be good to cleanly address this problem. Removing >>>> the "using namespace" from the header seems like the cleanest solution. >>>> WDYT? >>>> >>>> Thanks, >>>> Anna. >>> >>> >>> Hi Anna, >>> >>> What does OSAtomicDeprecated.h do? Does it also pull >>> memory_order_relaxed into global namespace? >> >> The header used to use std namespace locally before accessing those symbols, which caused the error when building clang: >> >> #define _OSATOMIC_USING_NAMESPACE_STD() using namespace std >> ... >> { >> _OSATOMIC_USING_NAMESPACE_STD(); >> return (atomic_fetch_add_explicit((volatile _OSAtomic_int32_t*) __theValue, >> __theAmount, memory_order_relaxed) + __theAmount); >> } >> >> They worked around the problem by qualifying the symbol with "std::": >> #define OSATOMIC_STD(_a) std::_a >> ... >> { >> return (OSATOMIC_STD(atomic_fetch_add_explicit)( >> (volatile _OSAtomic_int32_t*) __theValue, __theAmount, >> OSATOMIC_STD(memory_order_relaxed)) + __theAmount); >> } >> >> There are quite a few of these conflicts so it made the system header uglier. On the other hand, we do have a workaround. >> >> It would be great to have a general solution that would prevent this issue from happening in the future. >> >> + Bob, who reported the problem. >> >> Anna. >>> >>> For this particular problem we could just rename sanitizer internal constants. >>> >>> Maybe doing something like "namespace __tsan { using namespace >>> __sanitizer_common; }" will help to resolve it in general case. But I >>> am not sure, need to know what system headers do. > > > For such case we indeed can change > using namespace __sanitizer_common; > to: > namespace __tsan { using namespace __sanitizer_common; } > > > Here is my litmus test. Fails without -DFIX, compiles successfully with -DFIX. > > namespace sanitizer { > const int FOO = 1; > } > #ifdef FIX > namespace tsan { using namespace sanitizer; } > #else > using namespace sanitizer; > #endif > > namespace cxx { > const int FOO = 1; > } > > namespace tsan { > const int BAR = FOO; > } > > int main() { > using namespace cxx; > const int BAR = FOO; > return 0; > }Looks like a great suggestion! Since “using” is in sanitizer_common, I'll define in namespaces of all sanitizers we have and send a patch. Thanks, Anna. -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20160701/f0f478ef/attachment.html>