[compiler-rt][asan][Fuchsia] Tune the 64-bit asan allocator for riscv+fuchsia

[leonardchan]

Summary

This uses a custom size class map and primary allocator arena size that allows us to run all bringup tests on riscv64 with asan instrumentation reliabely.

[mcgrathr]

compiler-rt/lib/asan/asan_allocator.h

123

I don't know anything about the allocator. Is this making 128G the upper limit on how much the allocator can ever hand out? Or is it just setting 128G as a size of contiguous address space to use, and more than one such chunk might ever be used in the process?

124

I don't know what the rationales for choosing between {Very,}{Compact,Dense}SizeClassMap are. It would be nice if there were more comments about the meanings of the tuning parameters and the rationale for the choices.

Aside from the opaqueness of the code, the concern I'm getting at is whether this is making things compatible with a 39-bit address space (38-bit user address space) but still able to take advantage of a larger (48/47, 57/56) address space at runtime if that's what's available, or making things such that no matter how much is available this will never use more than it would use in the 39-bit address space?

For all Fuchsia platforms, there is theoretically variable options here and it won't be a change in the Fuchsia System ABI if more bits are supported on a future version and/or on future hardware. On x86 and AArch64 48 bits is the smallest size, but 57 bits may happen later, and for AArch64 maybe even 39 bits might happen later on some future hardware configuration (less likely). On RISC-V, 39, 48, and 57 are all likely to be supported whenever the hardware supports them, but the choice may well be dynamic based on how much actual RAM a machine has and other such factors.

[fabio-d]

compiler-rt/lib/asan/asan_allocator.h

123

Judging from compiler-rt/lib/sanitizer_common/sanitizer_allocator_primary64.h's Init(), this number is used by the 64-bit primary allocator as the size of the *single* contiguous reserved address space region, so this looks safe to me.

I think it's worth observing that we (@Caslyn in D148475) recently reduced the equivalent value in Scudo (PrimarySize = RegionSize * NumClasses on Fuchsia RISC-V from 1 GiB * 45 = 45 GiB to 256 MiB * 45 ~= 11 GB). Overall the values in this file seem to be a few order of magnitudes bigger than the corresponding Scudo values, but I don't know what the reason could be.

124

I don't know the details either. I can see that, with your change, Fuchsia RISC-V is moving to a different configuration:

typedef SizeClassMap<3, 4, 8, 17, 128, 16> DefaultSizeClassMap; // <- the old one
typedef SizeClassMap<2, 5, 9, 16, 8, 10> VeryDenseSizeClassMap; // <- the new one

template <uptr kNumBits, uptr kMinSizeLog, uptr kMidSizeLog, uptr kMaxSizeLog,
          uptr kMaxNumCachedHintT, uptr kMaxBytesCachedLog>
class SizeClassMap;

If it's similar to Scudo (as it seems to be), the first four values control the distribution of the size classes and the last two numbers control how big each per-thread cache is allowed to be. Intuitively, I would say that we shouldn't need to have different parameters here depending on the cpu architecture, but I don't have concrete data to support this intuition.

Changing the distribution of the size classes does not limit on the amount of *total* memory allocated by the primary allocator (which is limited by kAllocatorSize), but the maximum amount of memory allocated from each class is limited by kAllocatorSize / round_to_next_pow_of_two(NumClasses) (see kRegionSize in sanitizer_allocator_primary64.h:625).

Not marking as Done because I too would like to hear from other people :)

[leonardchan]

compiler-rt/lib/asan/asan_allocator.h

124

I updated this patch with a more fine-grained and tested configuration and comments which hopefully explain the justification/impact each setting will have. TBH a lot of this was driven by getting the combined scudo tests running in an asan-instrumented build. If we didn't have to run those tests with asan, the SizeClassMap could be "looser" in that we could have something closer to what scudo has or the DefaultSizeClassMap.

[leonardchan]

ping for thoughts on this

compiler-rt/lib/asan/asan_allocator.h

 };
 
 struct AsanMapUnmapCallback {
   void OnMap(uptr p, uptr size) const;
   void OnUnmap(uptr p, uptr size) const;
 };
 
 #if SANITIZER_CAN_USE_ALLOCATOR64
-# if SANITIZER_FUCHSIA
+#if SANITIZER_RISCV64
+const uptr kAllocatorSpace = ~(uptr)0;
+const uptr kAllocatorSize = 0x2000000000ULL;  // 128G.
+typedef VeryDenseSizeClassMap SizeClassMap;
+# elif SANITIZER_FUCHSIA
 const uptr kAllocatorSpace = ~(uptr)0;
 const uptr kAllocatorSize  =  0x40000000000ULL;  // 4T.
 typedef DefaultSizeClassMap SizeClassMap;
 # elif defined(__powerpc64__)
 const uptr kAllocatorSpace = ~(uptr)0;
 const uptr kAllocatorSize  =  0x20000000000ULL;  // 2T.
 typedef DefaultSizeClassMap SizeClassMap;
 # elif defined(__aarch64__) && SANITIZER_ANDROID
 // Android needs to support 39, 42 and 48 bit VMA.
 const uptr kAllocatorSpace =  ~(uptr)0;
 const uptr kAllocatorSize  =  0x2000000000ULL;  // 128G.
 typedef VeryCompactSizeClassMap SizeClassMap;
-#elif SANITIZER_RISCV64
-const uptr kAllocatorSpace = ~(uptr)0;
-const uptr kAllocatorSize = 0x2000000000ULL;  // 128G.
-typedef VeryDenseSizeClassMap SizeClassMap;
 #elif defined(__sparc__)
 const uptr kAllocatorSpace = ~(uptr)0;
 const uptr kAllocatorSize = 0x20000000000ULL;  // 2T.
 typedef DefaultSizeClassMap SizeClassMap;
 # elif SANITIZER_WINDOWS
 const uptr kAllocatorSpace = ~(uptr)0;
 const uptr kAllocatorSize  =  0x8000000000ULL;  // 500G
 typedef DefaultSizeClassMap SizeClassMap;