[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:111
+  *begin = (uptr)GetAllocatorCache();
+  *end = *begin + sizeof(*GetAllocatorCache());
 }
----------------
kubamracek wrote:
> 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.)
Ahh, yeah, the linter isn't smart enough.


================
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:
> 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?
On non-windows platforms, `uptr` is defined as an `unsigned long`, not an `unsigned long long` (sanitizer_internal_defs.h:103)


https://reviews.llvm.org/D29994