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

[Kostya Kortchinsky via Phabricator via llvm-commits]

cryptoad added inline comments.


================
Comment at: lib/scudo/scudo_allocator.cpp:347
     uptr UserBeg = reinterpret_cast<uptr>(UserPtr);
     if (!IsAligned(UserBeg, MinAlignment))
       return false;
----------------
alekseyshl wrote:
> Why this one is likely? My guess would be that most pointers we get to check are valid. Is it not so?
So on this, I went back and forth. Basically this is a helper function that is only used for `__sanitizer_get_ownership`.
I assume that anything can be fed to this function, as opposed to only heap pointers.
The reasoning was that `nullptr` was less likely overall, but then I have no guarantee on alignments.


================
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();
----------------
alekseyshl wrote:
> Others use CallocShouldReturnNullDueToOverflow, maybe you can use it too?
Good point. Will do.


================
Comment at: lib/scudo/scudo_allocator.cpp:667
+  if (UNLIKELY(!IsPowerOfTwo(Alignment)))
+    return nullptr;
+  return Instance.allocate(Size, Alignment, FromMemalign);
----------------
alekseyshl wrote:
> Why nullptr, why not return FailureHandler::OnBadRequest()?
Here and for the following comments, I guess my answer (which is debatable) is that it's not technically a bad request.
The man page for memalign, posix_memalign, and aligned_alloc says that hey return null for those specific cases.
Which, in my opinion, is different that trying to exceed a maximum alignment or size, or triggering an overflow in calloc.
As such I would lean towards this behavior as opposed to a failure or null based on an allocator setting.
Let me know if that makes sense.
If you judge that OnBadRequest is the way to go, I'll go that way, I am not strongly attached to the way I did it.


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


https://reviews.llvm.org/D34782