Page MenuHomePhabricator

[TSan] Add SystemZ support
ClosedPublic

Authored by iii on Jul 8 2021, 7:05 AM.

Diff Detail

Event Timeline

iii created this revision.Jul 8 2021, 7:05 AM
iii requested review of this revision.Jul 8 2021, 7:05 AM
Herald added projects: Restricted Project, Restricted Project, Restricted Project. · View Herald TranscriptJul 8 2021, 7:05 AM
Herald added subscribers: llvm-commits, Restricted Project, cfe-commits. · View Herald Transcript
iii edited subscribers, added: jonpa; removed: jnspaulsson.Jul 8 2021, 7:32 AM

Some solid work here.

I only have a question re mmap handling.

compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp
781 ↗(On Diff #357214)

TSan runtime is supposed to mprotect all ranges that are inaccessible to the application. How does it happen that kernel still returns such an address? Do we fail to mprotect all ranges on s390? If so should that be fixed instead?
I am concerned that if kernel can return unsupported addresses, it probably can returns them very early before exhausting all supported addresses (e.g. always return unsupported from the start).
Or is it that we mprotect everything, but kernel still somehow stomps on it? If so can this address overlap with, say, tsan shadow? If yes, them the munmap will unmap our shadow which is not good.

iii updated this revision to Diff 357471.Jul 9 2021, 3:59 AM
  • Reserve the address space "tail" in the "[TSan] Define C/C++ address ranges for SystemZ" patch.
  • Drop the "[TSan] Simulate OOM in mmap_interceptor()" patch.
  • Group "[TSan] Use zeroext for function parameters" with the common code patches.
iii updated this revision to Diff 357889.Jul 12 2021, 3:55 AM
  • Fix a local variable naming issue CheckAndProtect().
iii marked an inline comment as done.Jul 12 2021, 4:02 AM
iii added inline comments.
compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp
781 ↗(On Diff #357214)

The common code in CheckAndProtect() didn't mprotect the address space "tail" - I've added a special case for that.

iii marked an inline comment as done.Jul 12 2021, 6:00 AM

Regarding the openmp test failures: I tried check-openmp on a x86_64 machine, and they occur both with and without this patch series, so must be unrelated.

See inline comments ... otherwise the SystemZ platform-specific parts look good to me.

compiler-rt/lib/tsan/rtl/tsan_platform_posix.cpp
123 ↗(On Diff #357889)

Did you test this on older kernels without 5-level page table support? I believe the allocation / mprotect may fail on those ...

compiler-rt/lib/tsan/rtl/tsan_rtl_s390x.S
22 ↗(On Diff #357889)

Do we need CFI for r2/r3 ? Those are call-clobbered any cannot be unwound normally anyway ...

iii added inline comments.Jul 13 2021, 2:41 AM
compiler-rt/lib/tsan/rtl/tsan_platform_posix.cpp
123 ↗(On Diff #357889)

No, not really. Would it make sense to probe here? E.g. first try 0xfffffffffffff000, then 0x20000000000000. Or is there a way to query user_addr_max() / TASK_SIZE_MAX / TASK_SIZE?

compiler-rt/lib/tsan/rtl/tsan_rtl_s390x.S
22 ↗(On Diff #357889)

I'm not quite sure, but glibc does this (e.g. in sysdeps/s390/s390-64/dl-trampoline.h), so I figured I'll do this here as well just in case.

uweigand added inline comments.Jul 13 2021, 3:23 AM
compiler-rt/lib/tsan/rtl/tsan_platform_posix.cpp
123 ↗(On Diff #357889)

I don't know of any way to query this. You can simply do the allocation in stages (first to the top of the three-pagetable range, then four, then five), and ignore failures. As an unfortunate side effect this will force the kernel to allocate five levels of page tables even when unnecessary, but I don't think there's anything we can do about that.

compiler-rt/lib/tsan/rtl/tsan_rtl_s390x.S
22 ↗(On Diff #357889)

Huh. Well I guess it doesn't hurt either way.

iii added inline comments.Jul 13 2021, 3:31 AM
compiler-rt/lib/tsan/rtl/tsan_platform_posix.cpp
123 ↗(On Diff #357889)

3-level page table limit is quite below the application range defined in this series (0x40000000000 < 0xc00000000000). Are there any relevant kernels that do not support 4-level page tables? I tried to search the git history, and it looks as if even 2.6 ones have it.

uweigand added inline comments.Jul 13 2021, 3:33 AM
compiler-rt/lib/tsan/rtl/tsan_platform_posix.cpp
123 ↗(On Diff #357889)

Ah, right. Yes, we can certainly assume 4-level support at this point.

dvyukov accepted this revision.Jul 13 2021, 7:06 AM
This revision is now accepted and ready to land.Jul 13 2021, 7:06 AM
iii updated this revision to Diff 358363.Jul 13 2021, 11:19 AM

Based on Ulrich's feedback I tested the series on RHEL7, which, in
addition to an old kernel, contains an old glibc and an old toolchain.
This uncovered a few extra issues, which are fixed here:

  • Call __tls_get_offset() in order to force TLS allocation.
  • Align thread_registry_placeholder.
  • Protect the address space "tail" in two steps: until the 4-level page table boundary (this must succeed) and then until the 5-level page table boundary (this may fail).

Also, the Go support is finally working
(https://github.com/iii-i/go/tree/tsan-s390x), so, since we need to
make another round anyway, I've included the LLVM part here as well.

iii updated this revision to Diff 358431.Jul 13 2021, 2:48 PM
  • Fix style issues (seems like arc diff is linting only the top commit?).

The SystemZ specific changes all look good to me now, thanks!