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.

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.

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