[PATCH] D22595: [tsan] Enable 48-bit VMA support on aarch64

[Dmitry Vyukov via llvm-commits]

dvyukov added inline comments.

================
Comment at: lib/tsan/rtl/tsan_platform.h:197
@@ +196,3 @@
+  static const uptr kTraceMemEnd   = 0x0f06200000000ull;
+  static const uptr kHeapMemBeg    = 0x0ffff00000000ull;
+  static const uptr kHeapMemEnd    = 0x0ffff00000000ull;
----------------
I see that kHeapMemBeg/End situation is quite inconsistent across mappings. E.g. aarch64/42vma reserves a non-empty range for heap:
  static const uptr kHeapMemBeg    = 0x3e000000000ull;
  static const uptr kHeapMemEnd    = 0x3f000000000ull;
But it is actually unused.

I see ways to get situation under control:
1. Either guard the remaining uses of these constants in this file by TSAN_USE_ALLOCATOR64, and then remove definitions of the constants for configurations that don't use 64-bit allocator.
2. Or define kHeapMemBeg==kHeapMemEnd for such arches and do not other changes. Why did you guard all uses of these constants by SANITIZER_CAN_USE_ALLOCATOR64? I think ProtectRange should handle an empty range, CheckUserRegions should handle them as well. Can we remove all these ifdefs?



================
Comment at: lib/tsan/rtl/tsan_platform.h:494
@@ -461,1 +493,3 @@
 # endif
+// The heap region is used exclusively on tsan for SizeClassAllocator64, so
+// architectures that do not used it might just not define it as an user
----------------
drop "on tsan"
everything in tsan/* is about tsan

================
Comment at: lib/tsan/rtl/tsan_platform.h:495
@@ +494,3 @@
+// The heap region is used exclusively on tsan for SizeClassAllocator64, so
+// architectures that do not used it might just not define it as an user
+// region.
----------------
s/used/use/

================
Comment at: lib/tsan/rtl/tsan_platform.h:495
@@ +494,3 @@
+// The heap region is used exclusively on tsan for SizeClassAllocator64, so
+// architectures that do not used it might just not define it as an user
+// region.
----------------
dvyukov wrote:
> s/used/use/
s/an user/a user/

================
Comment at: lib/tsan/rtl/tsan_platform_posix.cc:145
@@ -144,1 +144,3 @@
+  // The heap region is used exclusively on tsan for SizeClassAllocator64.
+#if SANITIZER_CAN_USE_ALLOCATOR64
   ProtectRange(TraceMemEnd(), HeapMemBeg());
----------------
#if SANITIZER_CAN_USE_ALLOCATOR64 is a wrong condition for tsan, it uses a different condition. Please create a new define TSAN_USE_ALLOCATOR64 for tsan.

================
Comment at: lib/tsan/rtl/tsan_sync.h:53
@@ -52,2 +52,3 @@
   u64 GetId() const {
     // 47 lsb is addr, then 14 bits is low part of uid, then 3 zero bits.
+    return GetLsb((u64)addr | (uid << 48), 60);
----------------
Please also update this comment.


https://reviews.llvm.org/D22595