[PATCH] D123822: [compiler-rt][lsan] Share platform allocator settings between ASan and LSan

[Leonard Chan via Phabricator via llvm-commits]

leonardchan added inline comments.
Herald added a subscriber: Enna1.


================
Comment at: compiler-rt/lib/asan/asan_allocator.h:129-142
-# elif defined(__aarch64__) && SANITIZER_ANDROID
-// Android needs to support 39, 42 and 48 bit VMA.
-const uptr kAllocatorSpace =  ~(uptr)0;
-const uptr kAllocatorSize  =  0x2000000000ULL;  // 128G.
-typedef VeryCompactSizeClassMap SizeClassMap;
-#elif SANITIZER_RISCV64
-const uptr kAllocatorSpace = ~(uptr)0;
----------------
vitalybuka wrote:
> leonardchan wrote:
> > vitalybuka wrote:
> > > i suspect these smaller values has something to do with ability allocate shadow, which is not an issue for lsan
> > Do you recommend I should remove them?
> we can't just remove them for asan, because there probably was a reason
> but now, with shared config, it's behavior change for lsan
Oh I see, yeah perhaps we can instead just "unify" the values that are already matching in the asan and lsan configs rather than moving everything.


================
Comment at: compiler-rt/lib/sanitizer_common/sanitizer_platform.h:252-257
 #ifndef SANITIZER_CAN_USE_ALLOCATOR64
-#  if (SANITIZER_ANDROID && defined(__aarch64__)) || SANITIZER_FUCHSIA
+#  if (SANITIZER_ANDROID && defined(__aarch64__)) || SANITIZER_FUCHSIA || \
+      defined(__aarch64__)
 #    define SANITIZER_CAN_USE_ALLOCATOR64 1
-#  elif defined(__mips64) || defined(__aarch64__)
+#  elif defined(__mips64) || defined(__i386__) || defined(__arm__) || \
+      SANITIZER_RISCV64 || defined(__hexagon__)
----------------
vitalybuka wrote:
> this part looks a little bit unrelated, would it be better to have a separate patch?
> 
> Can you swticth to  stuff like SANITIZER_ARM?
> 
> Also maybe it's better to have condition SANITIZER_WORDSIZE != 64 || the rest?
Uploaded https://reviews.llvm.org/D126825 which instead just tells lsan to use `SANITIZER_CAN_USE_ALLOCATOR64` (which should just be a no-op). Then I'll have a followup patch that sets this macro to 1 for aarch64.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D123822/new/

https://reviews.llvm.org/D123822