[RISCV][Lsan] Set allocator for AP64

[hauhsu]

Summary

This patch uses similar allocator configuration to Asan, i.e. dynamic
allocator start address (~(uptr)0) and use VeryDenseSizeClassMap.
Since there are small memory spaces for SV39, using the
DefaultSizeClassMap causes error:

sanitizer_allocator_primary64.h:639:18: error: static assertion failed due to requirement '(kRegionSize) >= (1ULL << (64 / 2))':

639 |   COMPILER_CHECK((kRegionSize) >= (1ULL << (SANITIZER_WORDSIZE / 2)));
    |   ~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

sanitizer_common/sanitizer_allocator_primary64.h:639:32: note: expression evaluates to '2147483648 >= 4294967296'

[hauhsu]

Separate commits and add dependency.

[hauhsu]

Hi @luismarques @vitalybuka @MaskRay these patches might need your attention.
Thanks :)

[joshua-arch1]

Can lsan work for riscv64 now? I can run binaries compiled with -fsanitize=address adding ASAN_OPTIONS=detect_leaks=1, but no memory-leak error is reported.

[vitalybuka]

In D152895#4436941, @joshua-arch1 wrote:

Can lsan work for riscv64 now? I can run binaries compiled with -fsanitize=address adding ASAN_OPTIONS=detect_leaks=1, but no memory-leak error is reported.

This file applies to standalone lsan, without asan.

sanitizer_common/sanitizer_allocator_primary64.h:639:32: note: expression evaluates to '2147483648 >= 4294967296'

How do we get 2147483648 there?

[vitalybuka]

compiler-rt/lib/lsan/lsan_allocator.h

74–76

why this can be like SANITIZER_FUCHSIA
DefaultSizeClassMap
and
kAllocatorSize = 0x40000000000ULL; // 4T.

[vitalybuka]

@leonardchan and D151157

[MaskRay]

compiler-rt/lib/lsan/lsan_allocator.h

74–76

sanitizer_allocator_primary64.h:639:18: error: static assertion failed due to requirement '(kRegionSize) >= (1ULL << (64 / 2))': is due to an overly restrictive static_assert. D153664 shall relax this restriction and allow DefaultSizeClassMap to be usable.

kAllocatorSize = 0x40000000000ULL cannot be used as SV39 https://docs.kernel.org/riscv/vm-layout.html#risc-v-linux-kernel-sv39 cannot accomoodate such a large mmap request.

[joshua-arch1]

In D152895#4439966, @vitalybuka wrote:

In D152895#4436941, @joshua-arch1 wrote:

Can lsan work for riscv64 now? I can run binaries compiled with -fsanitize=address adding ASAN_OPTIONS=detect_leaks=1, but no memory-leak error is reported.

This file applies to standalone lsan, without asan.

So only x86 supports lsan with asan now? Are there extra work to support lsan with asan than standalone?

[hauhsu]

Can lsan work for riscv64 now? I can run binaries compiled with -fsanitize=address adding ASAN_OPTIONS=detect_leaks=1, but no memory-leak error is reported.

Asan and Lsan should work on SV39 virtual memory mapping mode without applying this patchset.
There was an Asan/Lsan porting done by @EccoTheDolphin (https://reviews.llvm.org/D87580) and @luismarques (https://reviews.llvm.org/D92403).

What Kito and I tries to do is to enable Asan and Lsan under different virtual memory mapping mode. I.e. SV39, SV48 and SV57.
The commits are:

• the sanitizer allocator from AP32 to AP64 (https://reviews.llvm.org/D139823)
• uses dynamic allocator memory space for Asan (https://reviews.llvm.org/D139827)
• uses dynamic allocator memory space for Lsan (https://reviews.llvm.org/D152895)
• set SANITIZER_MMAP_RANGE_SIZE for sv57 (https://reviews.llvm.org/D152990)
• loose Asan test CHECK conditions (https://reviews.llvm.org/D152991)

So only x86 supports lsan with asan now? Are there extra work to support lsan with asan than standalone?

Lsan with asan and standalone lsan for SV39 should work without any changes.

Lsan with asan for SV39/48 should be supported once asan commits are applied, that is with the first 2 patches (D139823, D139827).
standalone lsan for SV39/48 needs the first 3 patches (D139823, D139827, D152895)

Lsan with asan and standalone lsan for SV39/48/57 need 4 patches (D139823, D139827, D152895, D152990)

[joshua-arch1]

In D152895#4448058, @hauhsu wrote:

Can lsan work for riscv64 now? I can run binaries compiled with -fsanitize=address adding ASAN_OPTIONS=detect_leaks=1, but no memory-leak error is reported.

Asan and Lsan should work on SV39 virtual memory mapping mode without applying this patchset.
There was an Asan/Lsan porting done by @EccoTheDolphin (https://reviews.llvm.org/D87580) and @luismarques (https://reviews.llvm.org/D92403).

Only with this two patches, I can run binaries compiled with -fsanitize=address adding ASAN_OPTIONS=detect_leaks=1, but no memory-leak error is reported.

[hauhsu]

In D152895#4450985, @joshua-arch1 wrote:

In D152895#4448058, @hauhsu wrote:

Can lsan work for riscv64 now? I can run binaries compiled with -fsanitize=address adding ASAN_OPTIONS=detect_leaks=1, but no memory-leak error is reported.

Asan and Lsan should work on SV39 virtual memory mapping mode without applying this patchset.
There was an Asan/Lsan porting done by @EccoTheDolphin (https://reviews.llvm.org/D87580) and @luismarques (https://reviews.llvm.org/D92403).

Only with this two patches, I can run binaries compiled with -fsanitize=address adding ASAN_OPTIONS=detect_leaks=1, but no memory-leak error is reported.

Hmmm That's weird, I can use Lsan with our old toolchain (built in 2022.08):

$ cat lsan.c
// Source:
// https://github.com/google/sanitizers/wiki/AddressSanitizerLeakSanitizer
//
#include <stdlib.h>

void *p;

int main() {
  p = malloc(7);
  p = 0; // The memory is leaked here.
  return 0;
}

$ riscv64-unknown-linux-gnu-clang -mcpu=sifive-x280n -fsanitize=address lsan.c -o lsan -static-libsan 

(I added -static-libsan to make sure the old librt is used.)

Then copy the binary lsan to QEMU (rv39 mode) and run it:

root@qemuriscv64:~# ./lsan

=================================================================
==750==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 7 byte(s) in 1 object(s) allocated from:
    #0 0x2ae4e0edcc in __interceptor_malloc (/home/root/lsan+0xc5dcc)
    #1 0x2ae4e4242e in main (/home/root/lsan+0xf942e)
    #2 0x3f8fe19700 in __libc_start_call_main libc-start.c

SUMMARY: AddressSanitizer: 7 byte(s) leaked in 1 allocation(s).

[MaskRay]

@hauhsu There are many folks who care about the sanitizers but don't have much experience setting up RISC-V environments, especially the more nuanced variants like Sv39, Sv48.
It'd be useful if you can provide the environment setup instructions somewhere, with concrete examples (raw commands).

Have you tested that D153664 makes typedef VeryDenseSizeClassMap SizeClassMap; unnecessary?

[vitalybuka]

In D152895#4454190, @MaskRay wrote:

@hauhsu There are many folks who care about the sanitizers but don't have much experience setting up RISC-V environments, especially the more nuanced variants like Sv39, Sv48.
It'd be useful if you can provide the environment setup instructions somewhere, with concrete examples (raw commands).

Actually build bot would be even better, otherwise no guaranty that any fixes will continue to work.

Have you tested that D153664 makes typedef VeryDenseSizeClassMap SizeClassMap; unnecessary?

[hauhsu]

In D152895#4454424, @vitalybuka wrote:

In D152895#4454190, @MaskRay wrote:

@hauhsu There are many folks who care about the sanitizers but don't have much experience setting up RISC-V environments, especially the more nuanced variants like Sv39, Sv48.
It'd be useful if you can provide the environment setup instructions somewhere, with concrete examples (raw commands).

That make sense. After discussing with @kito-cheng I, 'll try to produce an environment for building and testing RISC-V LLVM. But it might take some time.
Our plane is to use QEMU system mode + Fedora image.

Actually build bot would be even better, otherwise no guaranty that any fixes will continue to work.

Sadly we can't promise to have resources for a build bot. But we'd love to see others to provide one.

Have you tested that D153664 makes typedef VeryDenseSizeClassMap SizeClassMap; unnecessary?

I'll test it later and update my commits if necessary. Thanks for reminding me.

[hauhsu]

Have you tested that D153664 makes typedef VeryDenseSizeClassMap SizeClassMap; unnecessary?

That commit works. I'll update this patch later.

[hauhsu]

There is no need to use VeryDenseSizeClassMap after applying D153664.

compiler-rt/lib/lsan/lsan_allocator.h

 };
 template <typename AddressSpaceView>
 using PrimaryAllocatorASVT = SizeClassAllocator32<AP32<AddressSpaceView>>;
 using PrimaryAllocator = PrimaryAllocatorASVT<LocalAddressSpaceView>;
 #else
 # if SANITIZER_FUCHSIA || defined(__powerpc64__)
 const uptr kAllocatorSpace = ~(uptr)0;
 const uptr kAllocatorSize  =  0x40000000000ULL;  // 4T.
+#  elif SANITIZER_RISCV64
+const uptr kAllocatorSpace = ~(uptr)0;
+const uptr kAllocatorSize = 0x2000000000ULL;  // 128G.
 #  elif SANITIZER_APPLE
 const uptr kAllocatorSpace = 0x600000000000ULL;
 const uptr kAllocatorSize  = 0x40000000000ULL;  // 4T.
 #  else
 const uptr kAllocatorSpace = 0x500000000000ULL;
 const uptr kAllocatorSize = 0x40000000000ULL;  // 4T.
 #  endif
 template <typename AddressSpaceViewTy>
 struct AP64 {  // Allocator64 parameters. Deliberately using a short name.
   static const uptr kSpaceBeg = kAllocatorSpace;