[PATCH] D60243: [LSan][AArch64] Speed-up leak and address sanitizers on AArch64 for 48-bit VMA

[Vitaly Buka via Phabricator via llvm-commits]

vitalybuka added inline comments.


================
Comment at: compiler-rt/lib/asan/asan_allocator.cc:36
+template <class Allocator1, class Allocator2>
+bool __sanitizer::DoubleAllocator<Allocator1, Allocator2>::UseAllocator1 =
+    GetMaxVirtualAddress() < (((uptr)1ULL << 48) - 1);
----------------
same as lsan and preinit_arrays


================
Comment at: compiler-rt/lib/asan/asan_allocator.cc:122
   uptr UsedSize(bool locked_version = false) {
-    if (user_requested_size != SizeClassMap::kMaxSize)
+    if (user_requested_size != getKMaxSize())
       return user_requested_size;
----------------
get_allocator().getKMaxSize() ?
same for the rest


================
Comment at: compiler-rt/lib/asan/asan_allocator.h:163
+
+static inline uptr getKNumClasses() {
+  if (Allocator32or64::UseAllocator1)
----------------
Historically it's Google style and we start function names with capitals.


================
Comment at: compiler-rt/lib/asan/asan_allocator.h:169
+
+static inline uptr getKMaxSize() {
+  if (Allocator32or64::UseAllocator1)
----------------
can you make it get_allocator().getKMaxSize() ?
it would be nice to move them into a separate patch.



================
Comment at: compiler-rt/lib/lsan/lsan_allocator.cc:32
+bool __sanitizer::DoubleAllocator<Allocator1, Allocator2>::UseAllocator1 =
+    GetMaxVirtualAddress() < (((uptr)1ULL << 48) - 1);
+
----------------
it's too late if we use SANITIZER_CAN_USE_PREINIT_ARRAY
Please move into DoubleAllocator::Init


================
Comment at: compiler-rt/lib/sanitizer_common/sanitizer_doubleallocator.h:3
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
----------------
Please update the first line and rename the file to sanitizer_double_allocator.h
we usually split words with _


================
Comment at: compiler-rt/lib/sanitizer_common/sanitizer_doubleallocator.h:18
+class DoubleAllocator {
+  Allocator1 a1;
+  Allocator2 a2;
----------------
sorry for these naming in my sample patch
historically we use Google style here
a1, a2, UseAllocator1 -> first_, second_, use_first_

I am open for better recommendations


================
Comment at: compiler-rt/lib/sanitizer_common/sanitizer_doubleallocator.h:22
+ public:
+  static bool UseAllocator1;
+
----------------
is possible to make it non static?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60243/new/

https://reviews.llvm.org/D60243