[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

124

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?

125

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

124

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.

125

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

125

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 OnMapSecondary(uptr p, uptr size, uptr user_begin, uptr user_size) const;
   void OnUnmap(uptr p, uptr size) const;
 };
 
 #if SANITIZER_CAN_USE_ALLOCATOR64
 # if SANITIZER_FUCHSIA
+// This is a sentinel indicating we do not want the primary allocator arena to
+// be placed at a fixed address. It will be anonymously mmap'd.
 const uptr kAllocatorSpace = ~(uptr)0;
-const uptr kAllocatorSize  =  0x40000000000ULL;  // 4T.
+#    if SANITIZER_RISCV64
+
+// These are sanitizer tunings that allow all bringup tests for RISCV-64 Sv39 +
+// Fuchsia to run with asan-instrumented. That is, we can run bringup, e2e,
+// libc, and scudo tests with this configuration.
+//
+// TODO: This is specifically tuned for Sv39. 48/57 will likely require other
+// tunings, or possibly use the same tunings Fuchsia uses for other archs. The
+// VMA size isn't technically tied to the Fuchsia System ABI, so once 48/57 is
+// supported, we'd need a way of dynamically checking what the VMA size is and
+// determining optimal configuration.
+
+// This indicates the total amount of space dedicated for the primary allocator
+// during initialization. This is roughly proportional to the size set by the
+// FuchsiaConfig for scudo (~11.25GB == ~2^33.49). Requesting any more could
+// lead to some failures in sanitized bringup tests where we can't allocate new
+// vmars because there wouldn't be enough contiguous space. We could try 2^34 if
+// we re-evaluate the SizeClassMap settings.
+const uptr kAllocatorSize = UINT64_C(1) << 33;  // 8GB
+
+// This is roughly equivalent to the configuration for the VeryDenseSizeClassMap
+// but has fewer size classes (ideally at most 32). Fewer class sizes means the
+// region size for each class is larger, thus less chances of running out of
+// space for each region. The main differences are the MidSizeLog (which is
+// smaller) and the MaxSizeLog (which is larger).
+//
+// - The MaxSizeLog is higher to allow some of the largest allocations I've
+//   observed to be placed in the primary allocator's arena as opposed to being
+//   mmap'd by the secondary allocator. This helps reduce fragmentation from
+//   large classes. A huge example of this the scudo allocator tests (and its
+//   testing infrastructure) which malloc's/new's objects on the order of
+//   hundreds of kilobytes which normally would not be in the primary allocator
+//   arena with the default VeryDenseSizeClassMap.
+// - The MidSizeLog is reduced to help shrink the number of size classes and
+//   increase region size. Without this, we'd see ASan complain many times about
+//   a region running out of available space.
+//
+// This differs a bit from the fuchsia config in scudo, mainly from the NumBits,
+// MaxSizeLog, and NumCachedHintT. This should place the number of size classes
+// for scudo at 45 and some large objects allocated by this config would be
+// placed in the arena whereas scudo would mmap them. The asan allocator needs
+// to have a number of classes that are a power of 2 for various internal things
+// to work, so we can't match the scudo settings to a tee. The sanitizer
+// allocator is slightly slower than scudo's but this is enough to get
+// memory-intensive scudo tests to run with asan instrumentation.
+typedef SizeClassMap</*kNumBits=*/2,
+                     /*kMinSizeLog=*/5,
+                     /*kMidSizeLog=*/8,
+                     /*kMaxSizeLog=*/18,
+                     /*kNumCachedHintT=*/8,
+                     /*kMaxBytesCachedLog=*/10>
+    SizeClassMap;
+static_assert(SizeClassMap::kNumClassesRounded <= 32,
+              "The above tunings were specifically selected to ensure there "
+              "would be at most 32 size classes. This restriction could be "
+              "loosened to 64 size classes if we can find a configuration of "
+              "allocator size and SizeClassMap tunings that allows us to "
+              "reliably run all bringup tests in a sanitized environment.");
+
+#    else
+// These are the default allocator tunings for non-RISCV environments where the
+// VMA is usually 48 bits and we have lots of space.
+const uptr kAllocatorSize = 0x40000000000ULL;  // 4T.
 typedef DefaultSizeClassMap SizeClassMap;
-# elif defined(__powerpc64__)
+#    endif
+#  elif defined(__powerpc64__)
 const uptr kAllocatorSpace = ~(uptr)0;
 const uptr kAllocatorSize  =  0x20000000000ULL;  // 2T.
 typedef DefaultSizeClassMap SizeClassMap;
-# elif defined(__aarch64__) && SANITIZER_ANDROID
+#  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
+#  elif SANITIZER_RISCV64
 const uptr kAllocatorSpace = ~(uptr)0;
 const uptr kAllocatorSize = 0x2000000000ULL;  // 128G.
 typedef VeryDenseSizeClassMap SizeClassMap;
-#elif defined(__sparc__)
+#  elif defined(__sparc__)
 const uptr kAllocatorSpace = ~(uptr)0;
 const uptr kAllocatorSize = 0x20000000000ULL;  // 2T.
 typedef DefaultSizeClassMap SizeClassMap;
-# elif SANITIZER_WINDOWS
+#  elif SANITIZER_WINDOWS
 const uptr kAllocatorSpace = ~(uptr)0;
 const uptr kAllocatorSize  =  0x8000000000ULL;  // 500G
 typedef DefaultSizeClassMap SizeClassMap;
 #  elif SANITIZER_APPLE
 const uptr kAllocatorSpace = 0x600000000000ULL;
 const uptr kAllocatorSize  =  0x40000000000ULL;  // 4T.
 typedef DefaultSizeClassMap SizeClassMap;
 #  else