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

[Francis Ricci via Phabricator via llvm-commits]

fjricci 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;
----------------
kubamracek wrote:
> Again, why do we need this change?  We need to make sure not to break these platforms (mips, aarch64).
This is exactly equivalent to the existing `#if` condition (#if (mips || aarch || i386) && !i386), but simplified because the data has been split out.


================
Comment at: lib/lsan/lsan_allocator.cc:111
+  *begin = (uptr)GetAllocatorCache();
+  *end = *begin + sizeof(*GetAllocatorCache());
 }
----------------
kubamracek wrote:
> Looks too weird to me.  `sizeof(AllocatorCache)` should do.
sizeof(TYPENAME) fails check-sanitizer style checks.


================
Comment at: lib/lsan/lsan_allocator.h:58
+
+#if defined(__x86_64__)
+struct AP64 {  // Allocator64 parameters. Deliberately using a short name.
----------------
kubamracek wrote:
> Do we need to change this code here?  It's not obvious to me that this doesn't change the parameters for some platforms.
It seemed cleaner to do this way, now that `kMaxAllowedMallocSize` has its own if block. However, I think you're right, this could potentially miss some edge cases for architecture.


================
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;
----------------
kubamracek wrote:
> 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.
It's necessary, at least on Darwin, because you'll overflow the type if you use a uptr to store a ULL. I'll move it to a separate patch.


https://reviews.llvm.org/D29994