[PATCH] D29994: Use pthreads for thread-local lsan allocator cache on darwin

[Kuba (Brecka) Mracek via Phabricator via llvm-commits]

kubamracek added inline comments.


================
Comment at: lib/lsan/lsan_allocator.cc:29
 static const uptr kMaxAllowedMallocSize = 1UL << 30;
-#else
+#elif defined(__mips64) || defined(__aarch64__)
 static const uptr kMaxAllowedMallocSize = 4UL << 30;
----------------
Again, why do we need this change?  We need to make sure not to break these platforms (mips, aarch64).


================
Comment at: lib/lsan/lsan_allocator.cc:111
+  *begin = (uptr)GetAllocatorCache();
+  *end = *begin + sizeof(*GetAllocatorCache());
 }
----------------
Looks too weird to me.  `sizeof(AllocatorCache)` should do.


================
Comment at: lib/lsan/lsan_allocator.h:58
+
+#if defined(__x86_64__)
+struct AP64 {  // Allocator64 parameters. Deliberately using a short name.
----------------
Do we need to change this code here?  It's not obvious to me that this doesn't change the parameters for some platforms.


================
Comment at: lib/sanitizer_common/sanitizer_allocator_primary64.h:49-50
  public:
-  static const uptr kSpaceBeg = Params::kSpaceBeg;
-  static const uptr kSpaceSize = Params::kSpaceSize;
+  static const u64 kSpaceBeg = Params::kSpaceBeg;
+  static const u64 kSpaceSize = Params::kSpaceSize;
   static const uptr kMetadataSize = Params::kMetadataSize;
----------------
Why is this necessary?  I mean, the change looks reasonable, but is it related to this change?  If not, please submit it as a separate patch.


https://reviews.llvm.org/D29994