[PATCH] D69765: [compiler-rt] Support more CPUs in LSan Allocator Address Space

Tue Nov 5 08:19:25 PST 2019

sebpop added inline comments.

================
Comment at: compiler-rt/lib/lsan/lsan_allocator.h:55
+    defined(__sh3__) || defined(__vax__) ||                           \
+    (defined(__mips64) && !defined(_LP64)) ||                         \
+    (defined(__sparc__) && !defined(_LP64)) ||                        \
----------------
krytarowski wrote:
> christos wrote:
> > krytarowski wrote:
> > > christos wrote:
> > > > krytarowski wrote:
> > > > > arichardson wrote:
> > > > > > krytarowski wrote:
> > > > > > > arichardson wrote:
> > > > > > > > Maybe change this to `&& !defined(__mips_n32)`?
> > > > > > > > For our CHERI CPU we use n64 but don't have _LP64 defined (since pointers are 128 bits).
> > > > > > > There is also o32. I don't know CHERI specifics and it is not supported today on NetBSD. (And there is no LSan for FreeBSD either).
> > > > > > I actually meant to add this to the change below not this one. Sorry.
> > > > > > 
> > > > > > However, the MIPS change seems wrong since the initial `#if defined(__mips64)` condition means the newly added condition never triggers.
> > > > > > 
> > > > > @christos what is the proper MIPS combination here?
> > > > I think that all this complex mess of ifdefs is really answering the question "Are pointers 32 bits?" so why don't we switch the ifdef to that (for example: #if \_\_UINTPTR_MAX\_\_ == 0xffffffffU)
> > > > 
> > > > PS: h at e markup. How does one make double underscore not become underline?
> > > So why aarch64 is above and ppc64 below?
> > I don't know why aarch64 is in the 32 bit group. I think it should not be. It is the only 64 bit arch there, which looks fishy. Whoever added it should have put a comment there. Perhaps it is in the file commit history.
> CC @sebpop and @fjricci  for insight. Could you please comment on this?
aarch64 is currently using the 32 bit allocator because the 64 bit allocator does not work in small VMA spaces, i.e., 39bit or 42bit.  https://reviews.llvm.org/D60243 is trying to select the 32 or 64 bit allocators based on a runtime test.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69765