[lsan] Share platform allocator settings between ASan and LSan

[mcgrathr]

Summary

This moves the platform-specific parameter logic from asan into
lsan_common.h to lsan can share it.

[vitalybuka]

can you please clang format this?

[mcgrathr]

In D87795#2278286, @vitalybuka wrote:

can you please clang format this?

I had avoided it because I was moving existing code verbatim. But I'll do it now.

[vitalybuka]

compiler-rt/lib/sanitizer_common/sanitizer_allocator.h

73

sanitizer allocator is used by msan, scudo, hwasan and upcoming D87120
but this constants make sense only for lsan/asan, so it does not look like a right place for them

However asan can #include lsan
right now it's only #include "lsan/lsan_common.h"
so maybe move there or create another header?

[vitalybuka]

In D87795#2278297, @mcgrathr wrote:

In D87795#2278286, @vitalybuka wrote:

can you please clang format this?

I had avoided it because I was moving existing code verbatim. But I'll do it now.

I think verbatim is no worth of fighting clang-format and clang-tidy

[mcgrathr]

rebase

[mcgrathr]

Moved to lsan_common.h.

[mcgrathr]

update

[mcgrathr]

I redid this to put it in the __lsan namespace in lsan_common.h, which is definitely simpler all around.

PTAL

compiler-rt/lib/sanitizer_common/sanitizer_allocator.h

73

Good thought. I changed it like that, and it seems cleaner now.

[filcab]

Please don’t do this. You’re adding a dependency between asan and lsan, and
making it impossible to support platforms without lsan.

It’s a layering violation. I guess you can hack it if you add a *very
minimal * “allocator for asan and/or lsan” in sanitizer common. That
wouldn’t be ideal (a header shared between only two Sanitizers, but at
least it would be in the right place.

Thank you,
Filipe

[vitalybuka]

In D87795#2289139, @filcab wrote:

Please don’t do this. You’re adding a dependency between asan and lsan, and

This dependency already exists in asan_allocator.cpp

Also I think that duplication of these constants is not rely a problems and it's fine as-is without the patch.

[mcgrathr]

In D87795#2289139, @filcab wrote:

Please don’t do this. You’re adding a dependency between asan and lsan, and
making it impossible to support platforms without lsan.

It’s a layering violation. I guess you can hack it if you add a *very
minimal * “allocator for asan and/or lsan” in sanitizer common. That
wouldn’t be ideal (a header shared between only two Sanitizers, but at
least it would be in the right place.

The source dependency already exists throughout the asan code, which unconditionally includes lsan headers.
There is no change to the fact that systems without lsan support work fine. It's already the case that #if CAN_SANITIZE_LEAKS is checked throughout the asan code to control whether it assumes lsan support.
This change does not any new interdependencies. The only reliance on lsan is that it's in the compiler-rt source tree, as before.

[mcgrathr]

In D87795#2289201, @vitalybuka wrote:

In D87795#2289139, @filcab wrote:

Please don’t do this. You’re adding a dependency between asan and lsan, and

This dependency already exists in asan_allocator.cpp

Also I think that duplication of these constants is not rely a problems and it's fine as-is without the patch.

The purpose of the change is to fix the lsan allocator configuration, which is practically unusable for aarch64.
It was clear that nobody had really maintained the lsan configurations and that the asan configurations were better-maintained and the proven settings for each platform. So everyone agreed that sharing the same settings between asan and lsan was the right thing to do. It seems very error-prone for future maintenance if this is done by copying the code. That's what happened before, and then someone changed the asan configuration settings to actually work well on more platforms and neglected to update the lsan configurations. We know that will happen again next time there is a need to change the asan configurations, so it seems unwise to merely update the copy rather than share the code, especially when it's so trivial to do so and there are no new interdependencies actually introduced.

[vitalybuka]

it's LGTM as-is

@filcab CAN_SANITIZE_LEAKS is the one which controls if asan uses lsan and it's already in this header. So it's already can be considered layering violation and we not making it worse. Do we miss something?

[filcab]

Yeah. I looked at it again and looks ok. I missed that we were already
including the file. As long as we can support lsan-less asan, we’re cool.

Thank you,
Filipe

[mcgrathr]

Since everyone approves can someone mark it approved?

compiler-rt/lib/asan/asan_allocator.h

private:
   uptr size_;
 };
 
 struct AsanMapUnmapCallback {
   void OnMap(uptr p, uptr size) const;
   void OnUnmap(uptr p, uptr size) const;
 };
 
+using SizeClassMap = __sanitizer::AllocatorSizeClassMap;
+
 #if SANITIZER_CAN_USE_ALLOCATOR64
-# if 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 defined(__aarch64__)
-// AArch64/SANITIZER_CAN_USE_ALLOCATOR64 is only for 42-bit VMA
-// so no need to different values for different VMA.
-const uptr kAllocatorSpace =  0x10000000000ULL;
-const uptr kAllocatorSize  =  0x10000000000ULL;  // 3T.
-typedef DefaultSizeClassMap 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;
-# else
-const uptr kAllocatorSpace = 0x600000000000ULL;
-const uptr kAllocatorSize  =  0x40000000000ULL;  // 4T.
-typedef DefaultSizeClassMap SizeClassMap;
-# endif
 template <typename AddressSpaceViewTy>
 struct AP64 {  // Allocator64 parameters. Deliberately using a short name.
-  static const uptr kSpaceBeg = kAllocatorSpace;
-  static const uptr kSpaceSize = kAllocatorSize;
+  static const uptr kSpaceBeg = __sanitizer::kAllocatorSpace;
+  static const uptr kSpaceSize = __sanitizer::kAllocatorSize;
   static const uptr kMetadataSize = 0;
   typedef __asan::SizeClassMap SizeClassMap;
   typedef AsanMapUnmapCallback MapUnmapCallback;
   static const uptr kFlags = 0;
   using AddressSpaceView = AddressSpaceViewTy;
 };
 
 template <typename AddressSpaceView>

compiler-rt/lib/lsan/lsan_allocator.h

#if SANITIZER_WORDSIZE == 64
   uptr requested_size : 54;
 #else
   uptr requested_size : 32;
   uptr padding : 22;
 #endif
   u32 stack_trace_id;
 };
 
-#if defined(__mips64) || defined(__aarch64__) || defined(__i386__) || \
-    defined(__arm__)
+#if SANITIZER_CAN_USE_ALLOCATOR64
 template <typename AddressSpaceViewTy>
-struct AP32 {
-  static const uptr kSpaceBeg = 0;
-  static const u64 kSpaceSize = SANITIZER_MMAP_RANGE_SIZE;
+struct AP64 {  // Allocator64 parameters. Deliberately using a short name.
+  static const uptr kSpaceBeg = __sanitizer::kAllocatorSpace;
+  static const uptr kSpaceSize = __sanitizer::kAllocatorSize;
   static const uptr kMetadataSize = sizeof(ChunkMetadata);
-  typedef __sanitizer::CompactSizeClassMap SizeClassMap;
-  static const uptr kRegionSizeLog = 20;
-  using AddressSpaceView = AddressSpaceViewTy;
+  typedef __sanitizer::AllocatorSizeClassMap SizeClassMap;
   typedef NoOpMapUnmapCallback MapUnmapCallback;
   static const uptr kFlags = 0;
+  using AddressSpaceView = AddressSpaceViewTy;
 };
 template <typename AddressSpaceView>
-using PrimaryAllocatorASVT = SizeClassAllocator32<AP32<AddressSpaceView>>;
+using PrimaryAllocatorASVT = SizeClassAllocator64<AP64<AddressSpaceView>>;
 using PrimaryAllocator = PrimaryAllocatorASVT<LocalAddressSpaceView>;
-#elif defined(__x86_64__) || defined(__powerpc64__) || defined(__s390x__)
-# if SANITIZER_FUCHSIA
-const uptr kAllocatorSpace = ~(uptr)0;
-const uptr kAllocatorSize  =  0x40000000000ULL;  // 4T.
-# elif defined(__powerpc64__)
-const uptr kAllocatorSpace = 0xa0000000000ULL;
-const uptr kAllocatorSize  = 0x20000000000ULL;  // 2T.
-#elif defined(__s390x__)
-const uptr kAllocatorSpace = 0x40000000000ULL;
-const uptr kAllocatorSize = 0x40000000000ULL;  // 4T.
-# else
-const uptr kAllocatorSpace = 0x600000000000ULL;
-const uptr kAllocatorSize  = 0x40000000000ULL;  // 4T.
-# endif
+#else  // !SANITIZER_CAN_USE_ALLOCATOR64
 template <typename AddressSpaceViewTy>
-struct AP64 {  // Allocator64 parameters. Deliberately using a short name.
-  static const uptr kSpaceBeg = kAllocatorSpace;
-  static const uptr kSpaceSize = kAllocatorSize;
+struct AP32 {
+  static const uptr kSpaceBeg = 0;
+  static const u64 kSpaceSize = SANITIZER_MMAP_RANGE_SIZE;
   static const uptr kMetadataSize = sizeof(ChunkMetadata);
-  typedef DefaultSizeClassMap SizeClassMap;
+  typedef __sanitizer::CompactSizeClassMap SizeClassMap;
+  static const uptr kRegionSizeLog = 20;
+  using AddressSpaceView = AddressSpaceViewTy;
   typedef NoOpMapUnmapCallback MapUnmapCallback;
   static const uptr kFlags = 0;
-  using AddressSpaceView = AddressSpaceViewTy;
 };
-
 template <typename AddressSpaceView>
-using PrimaryAllocatorASVT = SizeClassAllocator64<AP64<AddressSpaceView>>;
+using PrimaryAllocatorASVT = SizeClassAllocator32<AP32<AddressSpaceView>>;
 using PrimaryAllocator = PrimaryAllocatorASVT<LocalAddressSpaceView>;
-#endif
+#endif  // SANITIZER_CAN_USE_ALLOCATOR64
 
 template <typename AddressSpaceView>
 using AllocatorASVT = CombinedAllocator<PrimaryAllocatorASVT<AddressSpaceView>>;
 using Allocator = AllocatorASVT<LocalAddressSpaceView>;
 using AllocatorCache = Allocator::AllocatorCache;
 
 Allocator::AllocatorCache *GetAllocatorCache();
 

compiler-rt/lib/sanitizer_common/sanitizer_allocator.h

INLINE void RandomShuffle(T *a, u32 n, u32 *rand_state) {
   for (u32 i = n - 1; i > 0; i--)
     Swap(a[i], a[RandN(&state, i + 1)]);
   *rand_state = state;
 }
 
 #include "sanitizer_allocator_size_class_map.h"
 #include "sanitizer_allocator_stats.h"
 #include "sanitizer_allocator_primary64.h"
 #include "sanitizer_allocator_bytemap.h"
 #include "sanitizer_allocator_primary32.h"
 #include "sanitizer_allocator_local_cache.h"
 #include "sanitizer_allocator_secondary.h"
 #include "sanitizer_allocator_combined.h"
 
+// The platform-specific default parameters are shared by both
+// asan_allocator.h and lsan_allocator.h.
+#if SANITIZER_CAN_USE_ALLOCATOR64
+#if SANITIZER_FUCHSIA
+const uptr kAllocatorSpace = ~(uptr)0;
+const uptr kAllocatorSize = 0x40000000000ULL;  // 4T.
+using AllocatorSizeClassMap = DefaultSizeClassMap;
+#elif defined(__powerpc64__)
+const uptr kAllocatorSpace = ~(uptr)0;
+const uptr kAllocatorSize = 0x20000000000ULL;  // 2T.
+using AllocatorSizeClassMap = DefaultSizeClassMap;
+#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.
+using AllocatorSizeClassMap = VeryCompactSizeClassMap;
+#elif defined(__aarch64__)
+// AArch64/SANITIZER_CAN_USE_ALLOCATOR64 is only for 42-bit VMA
+// so no need to different values for different VMA.
+const uptr kAllocatorSpace = 0x10000000000ULL;
+const uptr kAllocatorSize = 0x10000000000ULL;  // 3T.
+using AllocatorSizeClassMap = DefaultSizeClassMap;
+#elif defined(__sparc__)
+const uptr kAllocatorSpace = ~(uptr)0;
+const uptr kAllocatorSize = 0x20000000000ULL;  // 2T.
+using AllocatorSizeClassMap = DefaultSizeClassMap;
+#elif SANITIZER_WINDOWS
+// On Windows 64-bit there is no easy way to find a large enough fixed address
+// space that is always available. Thus, a dynamically allocated address space
+// is used instead (i.e. ~(uptr)0).
+const uptr kAllocatorSpace = ~(uptr)0;
+const uptr kAllocatorSize = 0x8000000000ULL;  // 500G
+using AllocatorSizeClassMap = DefaultSizeClassMap;
+#else
+const uptr kAllocatorSpace = 0x600000000000ULL;
+const uptr kAllocatorSize = 0x40000000000ULL;  // 4T.
+using AllocatorSizeClassMap = DefaultSizeClassMap;
+#endif
+#endif  // SANITIZER_CAN_USE_ALLOCATOR64
+
 } // namespace __sanitizer
 
 #endif // SANITIZER_ALLOCATOR_H

compiler-rt/lib/sanitizer_common/tests/sanitizer_allocator_test.cpp

 
 using namespace __sanitizer;
 
 // Too slow for debug build
 #if !SANITIZER_DEBUG
 
 #if SANITIZER_CAN_USE_ALLOCATOR64
 #if SANITIZER_WINDOWS
-// On Windows 64-bit there is no easy way to find a large enough fixed address
-// space that is always available. Thus, a dynamically allocated address space
-// is used instead (i.e. ~(uptr)0).
-static const uptr kAllocatorSpace = ~(uptr)0;
-static const uptr kAllocatorSize  =  0x8000000000ULL;  // 500G
 static const u64 kAddressSpaceSize = 1ULL << 47;
-typedef DefaultSizeClassMap SizeClassMap;
 #elif SANITIZER_ANDROID && defined(__aarch64__)
-static const uptr kAllocatorSpace = 0x3000000000ULL;
-static const uptr kAllocatorSize  = 0x2000000000ULL;
 static const u64 kAddressSpaceSize = 1ULL << 39;
-typedef VeryCompactSizeClassMap SizeClassMap;
 #else
-static const uptr kAllocatorSpace = 0x700000000000ULL;
-static const uptr kAllocatorSize  = 0x010000000000ULL;  // 1T.
 static const u64 kAddressSpaceSize = 1ULL << 47;
-typedef DefaultSizeClassMap SizeClassMap;
 #endif
 
 template <typename AddressSpaceViewTy>
 struct AP64 {  // Allocator Params. Short name for shorter demangled names..
   static const uptr kSpaceBeg = kAllocatorSpace;
   static const uptr kSpaceSize = kAllocatorSize;
   static const uptr kMetadataSize = 16;
   typedef ::SizeClassMap SizeClassMap;