[PATCH] D40545: [sanitizer] add msvc atomic_compare_exchange_strong for sint32_t
Dmitry Vyukov via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Nov 29 06:16:34 PST 2017
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
More information about the llvm-commits
mailing list