This is an archive of the discontinued LLVM Phabricator instance.

[asan Win64] Implement atomic_compare_exchange_strong for 8 bit
ClosedPublic

Authored by wang0109 on Jul 1 2016, 7:47 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

wang0109 updated this revision to Diff 62595.Jul 1 2016, 7:47 PM
wang0109 retitled this revision from to [asan Win64] Implement atomic_compare_exchange_strong for 8 bit.
wang0109 updated this object.
wang0109 added a reviewer: rnk.
wang0109 added subscribers: chrisha, etienneb.
etienneb added inline comments.Jul 1 2016, 9:38 PM
lib/sanitizer_common/sanitizer_atomic_msvc.h
37 ↗(On Diff #62595)

please, align the // NOLINT comment with the previous one

186 ↗(On Diff #62595)

The 4 following line are the same on both 32/64.
Lift them after the #endif

191 ↗(On Diff #62595)

lift this line above the #ifdef _WIN64.
The parameter is not used in both case.

wang0109 updated this revision to Diff 62604.Jul 1 2016, 10:10 PM
  • update diff: remove some duplicate codes
etienneb added inline comments.Jul 4 2016, 8:21 AM
lib/sanitizer_common/sanitizer_atomic_msvc.h
190 ↗(On Diff #62604)

I wonder if it worth keeping assembly for 32-bits if intrinsic are working.
rnk@ what do you think?

I'm wondering why this unittest wasn't failing in 64-bits before you fixed the code:

TEST(SanitizerCommon, AtomicCompareExchangeTest) {

As I get it, the unittests are using the clang version.
Which means there is no coverage for the MSVC atomics operation.

#if defined(__clang__) || defined(__GNUC__)
# include "sanitizer_atomic_clang.h"
#elif defined(_MSC_VER)
# include "sanitizer_atomic_msvc.h"
#else
# error "Unsupported compiler"
#endif
rnk accepted this revision.Jul 6 2016, 8:28 AM
rnk edited edge metadata.

lgtm

This revision is now accepted and ready to land.Jul 6 2016, 8:28 AM
This revision was automatically updated to reflect the committed changes.