[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:667
+  if (UNLIKELY(!IsPowerOfTwo(Alignment)))
+    return nullptr;
+  return Instance.allocate(Size, Alignment, FromMemalign);
----------------
alekseyshl wrote:
> 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!
I'll revisit that portion tomorrow and see how it checks out.


================
Comment at: lib/scudo/scudo_allocator.cpp:676
+  }
   *MemPtr = Instance.allocate(Size, Alignment, FromMemalign);
   return 0;
----------------
alekseyshl wrote:
> To follow the spec, we should check the result too:
> 
> if (!*MemPtr)
>   return errno_ENOMEM;
Yeah I wanted to talk to you about that.
Following what's in https://linux.die.net/man/3/malloc (look for ENOMEM) , we should probably set the errno in ReturnNullOrDieOnFailure::OnOOM (or whichever works better).


https://reviews.llvm.org/D34782