Page MenuHomePhabricator

[compiler-rt] Support more CPUs in LSan Allocator Address Space
Needs ReviewPublic

Authored by krytarowski on Nov 3 2019, 8:18 AM.

Details

Summary

Handle more CPU modes of: m68k, hppa, sh3, vax, mips, sparc, ppc, alpha.

These modes are requires for NetBSD (and typically LSan/GCC).

Diff Detail

Event Timeline

krytarowski created this revision.Nov 3 2019, 8:18 AM
arichardson added inline comments.Nov 3 2019, 8:46 AM
compiler-rt/lib/lsan/lsan_allocator.h
55

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).

krytarowski marked an inline comment as done.Nov 3 2019, 1:32 PM
krytarowski added inline comments.
compiler-rt/lib/lsan/lsan_allocator.h
55

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).

arichardson added inline comments.Nov 3 2019, 2:40 PM
compiler-rt/lib/lsan/lsan_allocator.h
55

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.

73

__mips64 should only be defined for 64-bit architectures and as far as I know n32 is the only ABI with 32-bit pointers. Using the _LP64 macro will force us to make changes.

krytarowski marked an inline comment as done.Nov 3 2019, 3:48 PM
krytarowski added a subscriber: christos.
krytarowski added inline comments.
compiler-rt/lib/lsan/lsan_allocator.h
55

@christos what is the proper MIPS combination here?

christos marked an inline comment as done.Nov 3 2019, 5:21 PM
christos added inline comments.
compiler-rt/lib/lsan/lsan_allocator.h
55

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@e markup. How does one make double underscore not become underline?

krytarowski marked an inline comment as done.Nov 3 2019, 5:31 PM
krytarowski added inline comments.
compiler-rt/lib/lsan/lsan_allocator.h
55

So why aarch64 is above and ppc64 below?

christos marked an inline comment as done.Nov 3 2019, 5:41 PM
christos added inline comments.
compiler-rt/lib/lsan/lsan_allocator.h
55

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.

krytarowski marked an inline comment as done.Nov 4 2019, 5:47 AM
krytarowski added subscribers: fjricci, sebpop.
krytarowski added inline comments.
compiler-rt/lib/lsan/lsan_allocator.h
55

CC @sebpop and @fjricci for insight. Could you please comment on this?

sebpop added inline comments.Nov 5 2019, 8:16 AM
compiler-rt/lib/lsan/lsan_allocator.h
55

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.

krytarowski marked an inline comment as done.Nov 5 2019, 1:59 PM
krytarowski added inline comments.
compiler-rt/lib/lsan/lsan_allocator.h
55

@sebpop What is the minimal VMA for 64bit allocator? Full 64bit? But x86 as far as I know uses 40bit and 48bit... is it considered large?

vitalybuka resigned from this revision.Apr 8 2020, 6:10 PM