[PATCH] D34782: [scudo] Change aligned alloc functions to be more compliant & perf changes

[Aleksey Shlyapnikov via Phabricator via llvm-commits]

alekseyshl added inline comments.


================
Comment at: lib/scudo/scudo_allocator.cpp:347
     uptr UserBeg = reinterpret_cast<uptr>(UserPtr);
     if (!IsAligned(UserBeg, MinAlignment))
       return false;
----------------
Why this one is likely? My guess would be that most pointers we get to check are valid. Is it not so?


================
Comment at: lib/scudo/scudo_allocator.cpp:585
     uptr Total = NMemB * Size;
-    if (Size != 0 && Total / Size != NMemB)  // Overflow check
+    if (UNLIKELY(Size != 0 && Total / Size != NMemB))  // Overflow check
       return FailureHandler::OnBadRequest();
----------------
Others use CallocShouldReturnNullDueToOverflow, maybe you can use it too?


================
Comment at: lib/scudo/scudo_allocator.cpp:667
+  if (UNLIKELY(!IsPowerOfTwo(Alignment)))
+    return nullptr;
+  return Instance.allocate(Size, Alignment, FromMemalign);
----------------
Why nullptr, why not return FailureHandler::OnBadRequest()?


================
Comment at: lib/scudo/scudo_allocator.cpp:673
+  if (UNLIKELY(!IsPowerOfTwo(Alignment) || (Alignment % sizeof(void *)) != 0)) {
+    *MemPtr = nullptr;
+    return errno_EINVAL;
----------------
Same here


================
Comment at: lib/scudo/scudo_allocator.cpp:682
+  if (UNLIKELY(!IsPowerOfTwo(Alignment) || (Size & (Alignment - 1)) != 0))
+    return nullptr;
   return Instance.allocate(Size, Alignment, FromMalloc);
----------------
And here


================
Comment at: lib/scudo/scudo_allocator.h:119
 
+INLINE uptr RoundUpTo(uptr Size, uptr Boundary) {
+  return (Size + Boundary - 1) & ~(Boundary - 1);
----------------
Please add a comment why you have your own RoundUpTo, otherwise someone later will "improve" the code back.


https://reviews.llvm.org/D34782