[PATCH] D123814: [compiler-rt][lsan] Update CanBeAHeapPointer for AArch64

[Leonard Chan via Phabricator via llvm-commits]

leonardchan added inline comments.


================
Comment at: compiler-rt/lib/lsan/lsan_common.cpp:244
+static inline bool CouldBeUserPointer(uptr p) {
   // Since our heap is located in mmap-ed memory, we can assume a sensible lower
   // bound on heap addresses.
----------------
MaskRay wrote:
> The s/Can/Could/ change seems unnecessary?
> 
> If you want to change it, in other places of LLVM, MayBe is more common.
Change was more intended on the `Heap` part since this code didn't really look like it was for checking if something is heap-allocated.


================
Comment at: compiler-rt/lib/lsan/lsan_common.cpp:255
 #  elif defined(__aarch64__)
-  unsigned runtimeVMA = (MostSignificantSetBitIndex(GET_CURRENT_FRAME()) + 1);
-  return ((p >> runtimeVMA) == 0);
+  // Accept up to 48 bit VMA.
+  return ((p >> 48) == 0);
----------------
pcc wrote:
> I think AArch64 can have up to 52 bit VAs.
Yeah this value was more to fall in line with the other hardcoded values in this function. x86_64 could also be extended to 52bits depending on the page table format IIRC. I think what matters is if there are any lsan users that do use any higher order VMAs. Not sure if there are any that use 52bit, but I can increase to that value if there's suspicion of it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123814