Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[tsan] Shrink RiscV64 48-bit LowApp region slightly to speed up TSan RestoreAddr #72316

Merged
merged 1 commit into from
Nov 17, 2023

Conversation

thurstond
Copy link
Contributor

The RiscV64 48-bit mappings introduced in 46cb8d9 necessitated changing RestoreAddr to use 4-bits as the indicator. This roughly halves the speed of RestoreAddr, because it is now brute-force testing addresses in 1TB increments, rather than 2TB increments. Crucially, this slowdown applies to TSan on all platforms, not just RiscV64 48-bit.

This patch slightly shrinks the RiscV64 48-bit LowApp region mapping (from 5TB to 4TB); we hope that 4TB ought to be enough for anybody, especially since there is no ASLR applied to the binary in this region. This allows restoring RestoreAddr to use 3-bits as the indicator again, thus speeding up TSan on all platforms.

…RestoreAddr

The RiscV64 48-bit mappings introduced in 46cb8d9 necessitated changing RestoreAddr to use 4-bits as the indicator. This roughly halves the speed of RestoreAddr, because it is now brute-force testing addresses in 1TB increments, rather than 2TB increments. Crucially, this slowdown applies to TSan on all platforms, not just RiscV64 48-bit.

This patch slightly shrinks the RiscV64 48-bit LowApp region mapping (from 5TB to 4TB); we hope that 4TB ought to be enough for anybody, especially since there is no ASLR applied to the binary in this region. This allows restoring RestoreAddr to use 3-bits as the indicator again, thus speeding up TSan on all platforms.
@llvmbot
Copy link
Collaborator

llvmbot commented Nov 14, 2023

@llvm/pr-subscribers-compiler-rt-sanitizer

Author: Thurston Dang (thurstond)

Changes

The RiscV64 48-bit mappings introduced in 46cb8d9 necessitated changing RestoreAddr to use 4-bits as the indicator. This roughly halves the speed of RestoreAddr, because it is now brute-force testing addresses in 1TB increments, rather than 2TB increments. Crucially, this slowdown applies to TSan on all platforms, not just RiscV64 48-bit.

This patch slightly shrinks the RiscV64 48-bit LowApp region mapping (from 5TB to 4TB); we hope that 4TB ought to be enough for anybody, especially since there is no ASLR applied to the binary in this region. This allows restoring RestoreAddr to use 3-bits as the indicator again, thus speeding up TSan on all platforms.


Full diff: https://github.com/llvm/llvm-project/pull/72316.diff

1 Files Affected:

  • (modified) compiler-rt/lib/tsan/rtl/tsan_platform.h (+4-4)
diff --git a/compiler-rt/lib/tsan/rtl/tsan_platform.h b/compiler-rt/lib/tsan/rtl/tsan_platform.h
index d9ed3979b69eaf5..70b9ae09a990420 100644
--- a/compiler-rt/lib/tsan/rtl/tsan_platform.h
+++ b/compiler-rt/lib/tsan/rtl/tsan_platform.h
@@ -413,18 +413,18 @@ struct MappingRiscv64_39 {
 
 /*
 C/C++ on linux/riscv64 (48-bit VMA)
-0000 0000 1000 - 0500 0000 0000: main binary                      ( 5 TB)
+0000 0000 1000 - 0400 0000 0000: main binary                      ( 4 TB)
 0500 0000 0000 - 2000 0000 0000: -
 2000 0000 0000 - 4000 0000 0000: shadow memory                    (32 TB)
 4000 0000 0000 - 4800 0000 0000: metainfo                         ( 8 TB)
 4800 0000 0000 - 5555 5555 5000: -
 5555 5555 5000 - 5a00 0000 0000: main binary (PIE)                (~5 TB)
 5a00 0000 0000 - 7a00 0000 0000: -
-7a00 0000 0000 - 7fff ffff ffff: libraries and main thread stack  ( 5 TB)
+7a00 0000 0000 - 7fff ffff ffff: libraries and main thread stack  ( 6 TB)
 */
 struct MappingRiscv64_48 {
   static const uptr kLoAppMemBeg = 0x000000001000ull;
-  static const uptr kLoAppMemEnd = 0x050000000000ull;
+  static const uptr kLoAppMemEnd = 0x040000000000ull;
   static const uptr kShadowBeg = 0x200000000000ull;
   static const uptr kShadowEnd = 0x400000000000ull;
   static const uptr kMetaShadowBeg = 0x400000000000ull;
@@ -967,7 +967,7 @@ struct RestoreAddrImpl {
         Mapping::kMidAppMemEnd, Mapping::kHiAppMemBeg, Mapping::kHiAppMemEnd,
         Mapping::kHeapMemBeg,   Mapping::kHeapMemEnd,
     };
-    const uptr indicator = 0x0f0000000000ull;
+    const uptr indicator = 0x0e0000000000ull;
     const uptr ind_lsb = 1ull << LeastSignificantSetBitIndex(indicator);
     for (uptr i = 0; i < ARRAY_SIZE(ranges); i += 2) {
       uptr beg = ranges[i];

@alexfanqi
Copy link
Contributor

Tested on qemu system emulation. All tests passed, except again mmap_lots.cpp like before, which is not relevant.

@thurstond
Copy link
Contributor Author

Tested on qemu system emulation. All tests passed, except again mmap_lots.cpp like before, which is not relevant.

Thank you @alexfanqi for testing under QEMU!

@thurstond thurstond merged commit b1338d1 into llvm:main Nov 17, 2023
5 checks passed
sr-tream pushed a commit to sr-tream/llvm-project that referenced this pull request Nov 20, 2023
…RestoreAddr (llvm#72316)

The RiscV64 48-bit mappings introduced in
46cb8d9 necessitated changing
RestoreAddr to use 4-bits as the indicator. This roughly halves the
speed of RestoreAddr, because it is now brute-force testing addresses in
1TB increments, rather than 2TB increments. Crucially, this slowdown
applies to TSan on all platforms, not just RiscV64 48-bit.

This patch slightly shrinks the RiscV64 48-bit LowApp region mapping
(from 5TB to 4TB); we hope that 4TB ought to be enough for anybody,
especially since there is no ASLR applied to the binary in this region.
This allows restoring RestoreAddr to use 3-bits as the indicator again,
thus speeding up TSan on all platforms.

Co-authored-by: Thurston Dang <thurston@google.com>
zahiraam pushed a commit to zahiraam/llvm-project that referenced this pull request Nov 20, 2023
…RestoreAddr (llvm#72316)

The RiscV64 48-bit mappings introduced in
46cb8d9 necessitated changing
RestoreAddr to use 4-bits as the indicator. This roughly halves the
speed of RestoreAddr, because it is now brute-force testing addresses in
1TB increments, rather than 2TB increments. Crucially, this slowdown
applies to TSan on all platforms, not just RiscV64 48-bit.

This patch slightly shrinks the RiscV64 48-bit LowApp region mapping
(from 5TB to 4TB); we hope that 4TB ought to be enough for anybody,
especially since there is no ASLR applied to the binary in this region.
This allows restoring RestoreAddr to use 3-bits as the indicator again,
thus speeding up TSan on all platforms.

Co-authored-by: Thurston Dang <thurston@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants