[PATCH] D139686: [lsan] Add lsan support for loongarch64

[Youling Tang via Phabricator via cfe-commits]

tangyouling added inline comments.


================
Comment at: compiler-rt/lib/lsan/lsan_common.cpp:281
+#  elif defined(__loongarch_lp64)
+  return ((p >> 47) == 0);
 #  else
----------------
xen0n wrote:
> Since our VM layout is actually flexible, would it be better to document this, like "Support only the most common VM layout on LoongArch that allows 47 bits of user-space VMA"? Exact wording could be optimized, I'm only describing the gist here.
> Since our VM layout is actually flexible, would it be better to document this, like "Support only the most common VM layout on LoongArch that allows 47 bits of user-space VMA"? Exact wording could be optimized, I'm only describing the gist here.

How about "Allow 47-bit user-space VMA at current"?


================
Comment at: compiler-rt/test/asan/TestCases/Linux/leak_check_segv.cpp:4
 // REQUIRES: leak-detection
-#include <stdlib.h>
+#include <sanitizer/lsan_interface.h>
 #include <stdio.h>
----------------
xen0n wrote:
> Why unnecessarily reorder things, especially putting this //in front of// the standard library includes?
> Why unnecessarily reorder things, especially putting this //in front of// the standard library includes?

An automatic adjustment of the format.


================
Comment at: compiler-rt/test/asan/TestCases/Linux/leak_check_segv.cpp:20-21
+           16 * 1024, PROT_NONE);
+  mprotect((void *)(((unsigned long)data + kPageSize - 1) & ~(kPageSize - 1)),
+           16 * 1024, PROT_NONE);
   __lsan_do_leak_check();
----------------
xen0n wrote:
> Might be better to split this part of change out of the LoongArch enablement patch after all...
> Might be better to split this part of change out of the LoongArch enablement patch after all...

OK.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139686