This is an archive of the discontinued LLVM Phabricator instance.

[compiler-rt] [xray] Disable alignas() for thread_local objects on NetBSD
ClosedPublic

Authored by mgorny on Dec 21 2018, 6:05 AM.

Details

Summary

Disable enforcing alignas() for structs that are used as thread_local
data on NetBSD. The NetBSD ld.so implementation is buggy and does
not enforce correct alignment; however, clang seems to take it for
granted and generates instructions that segv on wrongly aligned objects.
Therefore, disable those alignas() statements on NetBSD until we can
establish a better fix.

Apparently, std::aligned_storage<> does not have any real effect
at the moment, so we can leave it as-is.

Diff Detail

Event Timeline

mgorny created this revision.Dec 21 2018, 6:05 AM
Herald added subscribers: Restricted Project, llvm-commits. · View Herald TranscriptDec 21 2018, 6:05 AM
krytarowski added inline comments.Dec 21 2018, 7:46 AM
lib/xray/xray_basic_logging.cc
58

Can we introduce a macro like:

#if SANITIZER_NETBSD
#define XRAY_TLS_ALIGNAS64 /* Nor supported */
#else
#define XRAY_TLS_ALIGNED_SUPPORTED
#define XRAY_TLS_ALIGNAS64 alignas(64)
#endif

And later:

struct XRAY_TLS_ALIGNAS64 ThreadLocalData {

and

#ifdef XRAY_TLS_ALIGNED_SUPPORTED
static_assert(alignof(ThreadLocalData) >= 64,
              "ThreadLocalData must be cache line aligned.");
#endif
mgorny marked an inline comment as done.Dec 21 2018, 8:29 AM
mgorny added inline comments.
lib/xray/xray_basic_logging.cc
58

Maybe. Though i suppose it'd make more sense to make 64 an argument to the macro.

krytarowski added inline comments.Dec 21 2018, 8:40 AM
lib/xray/xray_basic_logging.cc
58
#if !SANITIZER_NETBSD
#define XRAY_TLS_ALIGNAS(x) alignas(x)
#endif
struct XRAY_TLS_ALIGNAS(64) ThreadLocalData {

and

#ifdef XRAY_TLS_ALIGNAS
static_assert(alignof(ThreadLocalData) >= 64,
              "ThreadLocalData must be cache line aligned.");
#endif
58

Oops, actually not fully correct, but some variation of the above.

mgorny updated this revision to Diff 179297.Dec 21 2018, 9:04 AM
mgorny marked 3 inline comments as done.

Updated as requested.

krytarowski added inline comments.Dec 21 2018, 9:11 AM
lib/xray/xray_defs.h
22 ↗(On Diff #179297)

I would switch the order, in order to remove unneeded negation.

#if SANITIZER_NETBSD
...
#else
...
#endif

#define XRAY_HAS_TLS_ALIGNAS 0 is not needed as #if XRAY_HAS_TLS_ALIGNAS will evaluate to false anyway and #define XRAY_HAS_TLS_ALIGNAS 1 is equivalent to #define XRAY_HAS_TLS_ALIGNAS. But it's just a matter of style.

dberris added inline comments.Dec 21 2018, 4:09 PM
lib/xray/xray_defs.h
22 ↗(On Diff #179297)

FWIW, I agree to this suggestion. I also prefer:

#if SANITIZER_NETBSD
...
#else
...

But I do like the explicit definition of XRAY_HAS_TLS_ALIGNAS.

mgorny marked an inline comment as done.Dec 22 2018, 12:32 AM
mgorny added inline comments.
lib/xray/xray_defs.h
22 ↗(On Diff #179297)

I've chosen the explicit method to match what's already done e.g. in sanitizer_platform.h. I'll swap the order.

mgorny updated this revision to Diff 179414.Dec 22 2018, 1:11 AM
mgorny marked 2 inline comments as done.

Reordered.

krytarowski accepted this revision.Dec 22 2018, 1:47 AM
This revision is now accepted and ready to land.Dec 22 2018, 1:47 AM
This revision was automatically updated to reflect the committed changes.