Page MenuHomePhabricator

[sanitizer] Use __atomic_load/store() built-ins for generic 32-bit targets
ClosedPublic

Authored by glaubitz on Oct 22 2020, 2:46 AM.

Details

Summary

Simplifies the code and fixes the build on SPARC.

See discussion in: http://lists.llvm.org/pipermail/llvm-dev/2020-October/145937.html

Diff Detail

Event Timeline

glaubitz created this revision.Oct 22 2020, 2:46 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 22 2020, 2:46 AM
Herald added subscribers: Restricted Project, jfb, fedor.sergeev, jyknight. · View Herald Transcript
glaubitz requested review of this revision.Oct 22 2020, 2:46 AM

Looks good with nits.
Please also upload with full file context, I wonder what's before that "} else {" on line 80.

compiler-rt/lib/sanitizer_common/sanitizer_atomic_clang_other.h
54

Also remove this comment.

82

Also remove this comment.

glaubitz updated this revision to Diff 300496.Oct 24 2020, 11:11 AM

Addressed all comments and suggestions.

glaubitz updated this revision to Diff 300497.Oct 24 2020, 11:12 AM
glaubitz marked 2 inline comments as done.

Replace tabs with spaces to fix indentation.

I suggested this initially as a quick fix, not the final solution. Really if we want to fix this properly we should just always use __atomic_store with an appropriately-translated memory order, as is done in libcxx (this is really just reimplementing a subset of libcxx as a standalone library).

This patch unbreaks the build on bi-arch systems which are not x86 or MIPS, so I think it's okay to pick up the change and rewrite the code in cleaner terms later.

dvyukov accepted this revision.Oct 26 2020, 12:19 AM
This revision is now accepted and ready to land.Oct 26 2020, 12:19 AM

Could someone with access commit this patch for me?