[llvm-dev] How to resolve conflicts between sanitizer_common and system headers

[Anna Zaks via llvm-dev]

> 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>