[PATCH] D40545: [sanitizer] add msvc atomic_compare_exchange_strong for sint32_t

[Dmitry Vyukov via Phabricator via llvm-commits]

dvyukov added a comment.

In https://reviews.llvm.org/D40545#938998, @dberris wrote:

> In https://reviews.llvm.org/D40545#938984, @dvyukov wrote:
>
> > In https://reviews.llvm.org/D40545#938979, @comicfans44 wrote:
> >
> > > this is used in x-ray runtime , in xray_fdr_logging.cc  ,such as  line 56:
> > >
> > >   if (!__sanitizer::atomic_compare_exchange_strong(
> > >            &LogFlushStatus, &Result, XRayLogFlushStatus::XRAY_LOG_FLUSHING,
> > >            __sanitizer::memory_order_release))
> > >   
> > >
> > > currently x-ray didn't compile on windows,but this is a little step to make it build. I'm trying to build xray runtime on windows, some talks here:
> > >  http://lists.llvm.org/pipermail/llvm-dev/2017-November/119218.html
> >
> >
> > That's the one I referred to as "which should actually use atomic_uint32_t".
> >  Please switch xray to uint32. I don't think we want to support all of 10 atomic operations X 6 memory orderings X 20 different types, that's more than 1000 combinations.
> >  One day we should finally switch to C++ atomics, but until then it does not make sense to grow set of atomic operations infinitely.
>
>
> I'm curious though, since that type is coercing an enum into an `int`, which is the only defined conversion as far as I can tell. While it "should" be safe to use `atomic_uint32_t` instead, I'm wondering whether it's actually safe to do that. If it is, then I'm happy with using `atomic_uint32_t` instead of an `atomic_int32_t` in XRay.


The enum has values 0, 1 and 2. I don't fully understand your concern.

Well, strictly saying converting int to s32/int32_t is not safe either. int can be larger. And int does not have to be able to represent values 0, 1 and 2 as far as I see. And int32_t may not exist at all. I don't think you can beat C++ here.

We can do:

enum XRayLogFlushStatus : uint32_t {

> As an aside, what are the technical limitations for not using C++ atomics now? I understand the need to not depend on things in the C++ standard library that might have runtime dependencies, but the std::atomic<...> libs shouldn't really have those, right?

Every time I checked they were either broken or slower. I filed a bunch of bugs some time ago. I don't know what's the current status.


Repository:
  rCRT Compiler Runtime

https://reviews.llvm.org/D40545