[PATCH] D35358: [scudo] Do not grab a cache for secondary allocation & per related changes

[Kostya Kortchinsky via Phabricator via llvm-commits]

cryptoad added inline comments.


================
Comment at: lib/scudo/scudo_allocator.cpp:385
+        Salt = getPrng(ThreadContext)->getU8();
+        Ptr = BackendAllocator.Allocate(
+            getAllocatorCache(ThreadContext), AlignedSize, MinAlignment, true);
----------------
alekseyshl wrote:
> Now it feels like your combined allocator API should be AllocatePrimary/AllocateSecondary and the same for Deallocate/etc to do not pass useless params and branch on those. 
Sounds good.


================
Comment at: lib/scudo/scudo_allocator.cpp:386
+        Ptr = BackendAllocator.Allocate(
+            getAllocatorCache(ThreadContext), AlignedSize, MinAlignment, true);
+        ThreadContext->unlock();
----------------
alekseyshl wrote:
> Yep, it is more compact now, but I'd still add /*parameter name*/ comments next to those literals. They do help readability.
Ok.


================
Comment at: lib/scudo/scudo_allocator.cpp:390
+        SpinMutexLock l(&FallbackMutex);
+        Salt = FallbackPrng.getU8();
+        Ptr = BackendAllocator.Allocate(
----------------
alekseyshl wrote:
> I believe you want to {} two lines above too.
I am not sure I understand what you mean here.
The `FallbackMutex` encompasses both the Prng and the Cache, so I am using it for the whole else statement.


================
Comment at: lib/scudo/scudo_allocator.cpp:421
+    CHECK_LE(UserBeg + Size,
+             AllocBeg + (FromPrimary ? AlignedSize : NeededSize));
     Header.State = ChunkAllocated;
----------------
alekseyshl wrote:
> Nit: I get why you did this, but now there's a logical disconnect between the size passed to the allocator and this statement.
So bringing back `AllocationSize` here?


https://reviews.llvm.org/D35358