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

Kuba (Brecka) Mracek via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Feb 16 11:52:43 PST 2017


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;
----------------
fjricci wrote:
> 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.
I'd still prefer not to make refactoring together with functional changes.  (This particular change is okay, but it's hard to immediately see that the change is NFC.)


================
Comment at: lib/lsan/lsan_allocator.cc:111
+  *begin = (uptr)GetAllocatorCache();
+  *end = *begin + sizeof(*GetAllocatorCache());
 }
----------------
fjricci wrote:
> kubamracek wrote:
> > Looks too weird to me.  `sizeof(AllocatorCache)` should do.
> sizeof(TYPENAME) fails check-sanitizer style checks.
Does it really?  Is the linter smart enough to figure out that AllocatorCache is a type and not a variable?  Any other suggestions?  I really don't like to have a function call inside a `sizeof()` (I know it's not really calling the function, but still.)


================
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;
----------------
fjricci wrote:
> 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.
This whole class should be ifdef'd to only compile on 64-bit platforms.  Isn't that so?


https://reviews.llvm.org/D29994





More information about the llvm-commits mailing list