llvm dev - Jul 2016 - How to resolve conflicts between sanitizer_common and system headers

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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20160701/419cf3d0/attachment.html>

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?

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.

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