This is an archive of the discontinued LLVM Phabricator instance.

[compiler] [tsan] Enable tsan for aarch64
ClosedPublic

Authored by zatrazz on Jul 24 2015, 6:49 AM.

Details

Summary

This patch enabled TSAN for aarch64 with 39-bit VMA layout. As defined by
tsan_platform.h the layout used is:

0000 4000 00 - 0200 0000 00: main binary
2000 0000 00 - 4000 0000 00: shadow memory
4000 0000 00 - 5000 0000 00: metainfo
5000 0000 00 - 6000 0000 00: -
6000 0000 00 - 6200 0000 00: traces
6200 0000 00 - 7d00 0000 00: -
7d00 0000 00 - 7e00 0000 00: heap
7e00 0000 00 - 7fff ffff ff: modules and main thread stack

Which gives it about 8GB for main binary, 4GB for heap and 8GB for
modules and main thread stack.

Most of tests are passing, with the execption of:

  • ignore_lib0, ignore_lib1, ignore_lib3 due a kernel limitation for no support to make mmap page non-executable.
  • longjmp tests due missing specialized assembly routines.

These tests are xfail for now. The only tsan issue still showing is:

rtl/TsanRtlTest/Posix.ThreadLocalAccesses

Which still required further investigation.

Diff Detail

Event Timeline

zatrazz updated this revision to Diff 30572.Jul 24 2015, 6:49 AM
zatrazz retitled this revision from to [compiler] [tsan] Enable tsan for aarch64.
zatrazz updated this object.
zatrazz added reviewers: kcc, eugenis, rengolin.
zatrazz added a subscriber: llvm-commits.
rengolin added inline comments.Jul 24 2015, 6:58 AM
cmake/config-ix.cmake
261

If you enable this by default, and there's still one test failing, the AArch64 buildbot will break.

eugenis edited edge metadata.Jul 24 2015, 10:50 AM

Great job! I'll leave the real review to Dmitry. Just a few comments inline.

lib/sanitizer_common/sanitizer_linux_libcdep.cc
267

Looks like this if() is unnecessary, here and above.

300

Clang does not recognize this builtin on aarch64. It looks like it only exists on arm. Are you building with gcc?

rengolin added inline comments.Jul 25 2015, 7:13 AM
lib/sanitizer_common/sanitizer_linux_libcdep.cc
300

shouldn't be too hard to implement that, now that we have direct access to reading and writing to specific special registers.

Hi Renato,

Thanks for review it.

Hi Evgeniy,

Thanks for the review.

jasonk added a subscriber: jasonk.Jul 27 2015, 11:43 AM
rengolin added inline comments.Jul 30 2015, 12:38 PM
lib/sanitizer_common/sanitizer_linux_libcdep.cc
300

This is now supported in Clang and LLVM.

zatrazz updated this revision to Diff 31262.Aug 3 2015, 1:45 PM
zatrazz edited edge metadata.

Updated patch now that __builtin_thread_pointer is supported on clang/llvm. I also disabled the 'rtl/TsanRtlTest/Posix.ThreadLocalAccesses' for now, since it shows failures with high thread number (current 8, with 4 it shows no issue).

rengolin added inline comments.Aug 4 2015, 3:50 AM
lib/tsan/tests/rtl/tsan_posix.cc
95 ↗(On Diff #31262)

Don't need to add your name to a TODO. :)

Also, we normally use FIXME.

dvyukov edited edge metadata.Aug 4 2015, 7:12 AM

Please reupload the change with full context, obtain the diff with:
$ svn diff --diff-cmd=diff -x -U999999

lib/sanitizer_common/sanitizer_linux_libcdep.cc
198

Please use a constant instead of macro:

const uptr kStackAlign = 16;
212

Why did you remove !SANITIZER_GO?

zatrazz updated this revision to Diff 31346.Aug 5 2015, 6:13 AM
zatrazz edited edge metadata.

Update patch with full-context, change to use a constant to define the stack requirement and undo a comment removal.

dvyukov accepted this revision.Aug 5 2015, 7:21 AM
dvyukov edited edge metadata.

LGTM
This is awesome!
Thank you

This revision is now accepted and ready to land.Aug 5 2015, 7:21 AM
zatrazz closed this revision.Aug 5 2015, 9:08 AM