[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 ↗

(On Diff #292339)

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 ↗

(On Diff #292339)

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

 //
 // ASan-private header for asan_allocator.cpp.
 //===----------------------------------------------------------------------===//
 
 #ifndef ASAN_ALLOCATOR_H
 #define ASAN_ALLOCATOR_H
 
 #include "asan_flags.h"
-#include "asan_internal.h"
 #include "asan_interceptors.h"
+#include "asan_internal.h"
+#include "lsan/lsan_common.h"
 #include "sanitizer_common/sanitizer_allocator.h"
 #include "sanitizer_common/sanitizer_list.h"
 
 namespace __asan {
 
+// These are defined in lsan_common.h because they are shared between the asan
+// allocator and the standalone lsan allocator.
+using __lsan::AllocatorSizeClassMap;
+using __lsan::kAllocatorSize;
+using __lsan::kAllocatorSpace;
+
 enum AllocType {
   FROM_MALLOC = 1,  // Memory block came from malloc, calloc, realloc, etc.
   FROM_NEW = 2,     // Memory block came from operator new.
   FROM_NEW_BR = 3   // Memory block came from operator new [ ]
 };
 
 class AsanChunk;
 

  private:
   AsanChunk *const chunk_;
 };
 
 AsanChunkView FindHeapChunkByAddress(uptr address);
 AsanChunkView FindHeapChunkByAllocBeg(uptr address);
 
 // List of AsanChunks with total size.
-class AsanChunkFifoList: public IntrusiveList<AsanChunk> {
+class AsanChunkFifoList : public IntrusiveList<AsanChunk> {
  public:
-  explicit AsanChunkFifoList(LinkerInitialized) { }
+  explicit AsanChunkFifoList(LinkerInitialized) {}
   AsanChunkFifoList() { clear(); }
   void Push(AsanChunk *n);
   void PushList(AsanChunkFifoList *q);
   AsanChunk *Pop();
   uptr size() { return size_; }
   void clear() {
     IntrusiveList<AsanChunk>::clear();
     size_ = 0;
   }
+
  private:
   uptr size_;
 };
 
 struct AsanMapUnmapCallback {
   void OnMap(uptr p, uptr size) const;
   void OnUnmap(uptr p, uptr size) const;
 };
 
+using SizeClassMap = 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 kMetadataSize = 0;
   typedef __asan::SizeClassMap SizeClassMap;
   typedef AsanMapUnmapCallback MapUnmapCallback;
   static const uptr kFlags = 0;
struct AP32 {
   static const uptr kMetadataSize = 0;
   typedef __asan::SizeClassMap SizeClassMap;
   static const uptr kRegionSizeLog = 20;
   using AddressSpaceView = AddressSpaceViewTy;
   typedef AsanMapUnmapCallback MapUnmapCallback;
   static const uptr kFlags = 0;
 };
 template <typename AddressSpaceView>
-using PrimaryAllocatorASVT = SizeClassAllocator32<AP32<AddressSpaceView> >;
+using PrimaryAllocatorASVT = SizeClassAllocator32<AP32<AddressSpaceView>>;
 using PrimaryAllocator = PrimaryAllocatorASVT<LocalAddressSpaceView>;
 #endif  // SANITIZER_CAN_USE_ALLOCATOR64
 
 static const uptr kNumberOfSizeClasses = SizeClassMap::kNumClasses;
 
 template <typename AddressSpaceView>
 using AsanAllocatorASVT =
     CombinedAllocator<PrimaryAllocatorASVT<AddressSpaceView>>;
 using AsanAllocator = AsanAllocatorASVT<LocalAddressSpaceView>;
 using AllocatorCache = AsanAllocator::AllocatorCache;
 
 struct AsanThreadLocalMallocStorage {
   uptr quarantine_cache[16];
   AllocatorCache allocator_cache;
   void CommitBack();
+
  private:
   // These objects are allocated via mmap() and are zero-initialized.
   AsanThreadLocalMallocStorage() {}
 };
 
 void *asan_memalign(uptr alignment, uptr size, BufferedStackTrace *stack,
                     AllocType alloc_type);
 void asan_free(void *ptr, BufferedStackTrace *stack, AllocType alloc_type);

compiler-rt/lib/lsan/lsan_allocator.h

 //=-- lsan_allocator.h ----------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 // This file is a part of LeakSanitizer.
 // Allocator for standalone LSan.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LSAN_ALLOCATOR_H
 #define LSAN_ALLOCATOR_H
 
+#include "lsan_common.h"
 #include "sanitizer_common/sanitizer_allocator.h"
 #include "sanitizer_common/sanitizer_common.h"
 #include "sanitizer_common/sanitizer_internal_defs.h"
-#include "lsan_common.h"
 
 namespace __lsan {
 
 void *Allocate(const StackTrace &stack, uptr size, uptr alignment,
                bool cleared);
 void Deallocate(void *p);
 void *Reallocate(const StackTrace &stack, void *p, uptr new_size,
                  uptr alignment);
 uptr GetMallocUsableSize(const void *p);
 
-template<typename Callable>
+template <typename Callable>
 void ForEachChunk(const Callable &callback);
 
 void GetAllocatorCacheRange(uptr *begin, uptr *end);
 void AllocatorThreadFinish();
 void InitializeAllocator();
 
 const bool kAlwaysClearMemory = true;
 
 struct ChunkMetadata {
   u8 allocated : 8;  // Must be first.
   ChunkTag tag : 2;
 #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 = kAllocatorSpace;
+  static const uptr kSpaceSize = kAllocatorSize;
   static const uptr kMetadataSize = sizeof(ChunkMetadata);
-  typedef __sanitizer::CompactSizeClassMap SizeClassMap;
-  static const uptr kRegionSizeLog = 20;
-  using AddressSpaceView = AddressSpaceViewTy;
+  typedef 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/lsan/lsan_common.h

 #if (SANITIZER_LINUX && !SANITIZER_ANDROID || SANITIZER_MAC) &&          \
     (SANITIZER_WORDSIZE == 64) &&                                        \
     (defined(__x86_64__) || defined(__mips64) || defined(__aarch64__) || \
      defined(__powerpc64__) || defined(__s390x__))
 #define CAN_SANITIZE_LEAKS 1
 #elif defined(__i386__) && \
     (SANITIZER_LINUX && !SANITIZER_ANDROID || SANITIZER_MAC)
 #define CAN_SANITIZE_LEAKS 1
-#elif defined(__arm__) && \
-    SANITIZER_LINUX && !SANITIZER_ANDROID
+#elif defined(__arm__) && SANITIZER_LINUX && !SANITIZER_ANDROID
 #define CAN_SANITIZE_LEAKS 1
 #elif SANITIZER_NETBSD || SANITIZER_FUCHSIA
 #define CAN_SANITIZE_LEAKS 1
 #else
 #define CAN_SANITIZE_LEAKS 0
 #endif
 
 namespace __sanitizer {
 class FlagParser;
 class ThreadRegistry;
 struct DTLS;
-}
+}  // namespace __sanitizer
 
 namespace __lsan {
 
+// The platform-specific allocator parameters are shared by both
+// asan_allocator.h and lsan_allocator.h.
+#if SANITIZER_CAN_USE_ALLOCATOR64
+#if SANITIZER_FUCHSIA
+constexpr uptr kAllocatorSpace = ~(uptr)0;
+constexpr uptr kAllocatorSize = 0x40000000000ULL;  // 4T.
+using AllocatorSizeClassMap = DefaultSizeClassMap;
+#elif defined(__powerpc64__)
+constexpr uptr kAllocatorSpace = ~(uptr)0;
+constexpr uptr kAllocatorSize = 0x20000000000ULL;  // 2T.
+using AllocatorSizeClassMap = DefaultSizeClassMap;
+#elif defined(__aarch64__) && SANITIZER_ANDROID
+// Android needs to support 39, 42 and 48 bit VMA.
+constexpr uptr kAllocatorSpace = ~(uptr)0;
+constexpr 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.
+constexpr uptr kAllocatorSpace = 0x10000000000ULL;
+constexpr uptr kAllocatorSize = 0x10000000000ULL;  // 3T.
+using AllocatorSizeClassMap = DefaultSizeClassMap;
+#elif defined(__sparc__)
+constexpr uptr kAllocatorSpace = ~(uptr)0;
+constexpr 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).
+constexpr uptr kAllocatorSpace = ~(uptr)0;
+constexpr uptr kAllocatorSize = 0x8000000000ULL;  // 500G
+using AllocatorSizeClassMap = DefaultSizeClassMap;
+#else
+constexpr uptr kAllocatorSpace = 0x600000000000ULL;
+constexpr uptr kAllocatorSize = 0x40000000000ULL;  // 4T.
+using AllocatorSizeClassMap = DefaultSizeClassMap;
+#endif
+#endif  // SANITIZER_CAN_USE_ALLOCATOR64
+
 // Chunk tags.
 enum ChunkTag {
   kDirectlyLeaked = 0,  // default
   kIndirectlyLeaked = 1,
   kReachable = 2,
   kIgnored = 3
 };
 
-const u32 kInvalidTid = (u32) -1;
+const u32 kInvalidTid = (u32)-1;
 
 struct Flags {
 #define LSAN_FLAG(Type, Name, DefaultValue, Description) Type Name;
 #include "lsan_flags.inc"
 #undef LSAN_FLAG
 
   void SetDefaults();
-  uptr pointer_alignment() const {
-    return use_unaligned ? 1 : sizeof(uptr);
-  }
+  uptr pointer_alignment() const { return use_unaligned ? 1 : sizeof(uptr); }
 };
 
 extern Flags lsan_flags;
 inline Flags *flags() { return &lsan_flags; }
 void RegisterLsanFlags(FlagParser *parser, Flags *f);
 
 struct Leak {
   u32 id;
struct CheckForLeaksParam {
   Frontier frontier;
   LeakReport leak_report;
   bool success = false;
 };
 
 InternalMmapVector<RootRegion> const *GetRootRegions();
 void ScanRootRegion(Frontier *frontier, RootRegion const &region,
                     uptr region_begin, uptr region_end, bool is_readable);
-void ForEachExtraStackRangeCb(uptr begin, uptr end, void* arg);
+void ForEachExtraStackRangeCb(uptr begin, uptr end, void *arg);
 // Run stoptheworld while holding any platform-specific locks, as well as the
 // allocator and thread registry locks.
 void LockStuffAndStopTheWorld(StopTheWorldCallback callback,
-                              CheckForLeaksParam* argument);
+                              CheckForLeaksParam *argument);
 
-void ScanRangeForPointers(uptr begin, uptr end,
-                          Frontier *frontier,
+void ScanRangeForPointers(uptr begin, uptr end, Frontier *frontier,
                           const char *region_type, ChunkTag tag);
 void ScanGlobalRange(uptr begin, uptr end, Frontier *frontier);
 
 enum IgnoreObjectResult {
   kIgnoreObjectSuccess,
   kIgnoreObjectAlreadyIgnored,
   kIgnoreObjectInvalid
 };

  public:
   // Constructor accepts address of user-visible chunk.
   explicit LsanMetadata(uptr chunk);
   bool allocated() const;
   ChunkTag tag() const;
   void set_tag(ChunkTag value);
   uptr requested_size() const;
   u32 stack_trace_id() const;
+
  private:
   void *metadata_;
 };
 
 }  // namespace __lsan
 
 extern "C" {
-SANITIZER_INTERFACE_ATTRIBUTE SANITIZER_WEAK_ATTRIBUTE
-const char *__lsan_default_options();
+SANITIZER_INTERFACE_ATTRIBUTE SANITIZER_WEAK_ATTRIBUTE const char *
+__lsan_default_options();
 
-SANITIZER_INTERFACE_ATTRIBUTE SANITIZER_WEAK_ATTRIBUTE
-int __lsan_is_turned_off();
+SANITIZER_INTERFACE_ATTRIBUTE SANITIZER_WEAK_ATTRIBUTE int
+__lsan_is_turned_off();
 
-SANITIZER_INTERFACE_ATTRIBUTE SANITIZER_WEAK_ATTRIBUTE
-const char *__lsan_default_suppressions();
+SANITIZER_INTERFACE_ATTRIBUTE SANITIZER_WEAK_ATTRIBUTE const char *
+__lsan_default_suppressions();
 }  // extern "C"
 
 #endif  // LSAN_COMMON_H