[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;
----------------
cryptoad wrote:
> 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.
Makes sense.


================
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();
----------------
cryptoad wrote:
> alekseyshl wrote:
> > Others use CallocShouldReturnNullDueToOverflow, maybe you can use it too?
> Good point. Will do.
D34799 renames it to CheckForCallocOverflow. Whoever is late to commit, gets to merge, I guess :)


================
Comment at: lib/scudo/scudo_allocator.cpp:667
+  if (UNLIKELY(!IsPowerOfTwo(Alignment)))
+    return nullptr;
+  return Instance.allocate(Size, Alignment, FromMemalign);
----------------
cryptoad wrote:
> 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.
My take on it is that we have a flag allocator_may_return_null and when it is == 0, our allocator should do just that, never return null. If the client desires the up to the spec behavior of those memory functions, they should specify allocator_may_return_null=1.

That way our behavior is consistent.

I'm open to debate too, there's always some space for improvements!


================
Comment at: lib/scudo/scudo_allocator.cpp:676
+  }
   *MemPtr = Instance.allocate(Size, Alignment, FromMemalign);
   return 0;
----------------
To follow the spec, we should check the result too:

if (!*MemPtr)
  return errno_ENOMEM;


https://reviews.llvm.org/D34782