[PATCH] D48031: [scudo] Add C++17 aligned new/delete operators support

[Kostya Kortchinsky via Phabricator via llvm-commits]

cryptoad added inline comments.


================
Comment at: lib/scudo/scudo_allocator.cpp:633
+void *scudoAllocate(uptr Size, uptr Alignment, AllocType Type) {
+  if (Alignment && UNLIKELY(!IsPowerOfTwo(Alignment))) {
+    errno = EINVAL;
----------------
alekseyshl wrote:
> IsPowerOfTwo(0) is true, which is not correct, but you can use this feature to drop Alignment check.
> 
> We mostly use IsPowerOfTwo in places where we know x != 0, so no need to check for 0 and here we just don't care.
> 
> I just submitted D47924, which tightened alignment check for posix_memalign and aligned_alloc. I am curious whether it will break anyone, according to spec, aligned_alloc should not accept non-power-of-two alignments, although my current libc implementation happily accepts any value, 0 or not, power of two or not. posix_memalign returns EINVAL as expected. Anyway, it was just a side note.
Ack. I was sort of thinking that checking for 0 had the least performance cost since it would be the path taken the most. WDYT?


================
Comment at: lib/scudo/scudo_new_delete.cpp:38
+  if (!NoThrow && UNLIKELY(!Ptr)) DieOnFailure::OnOOM();           \
+  return Ptr;
+
----------------
alekseyshl wrote:
> I was thinking of merging OPERATOR_NEW_BODY/OPERATOR_NEW_BODY_ALIGN defines for other allocators, so maybe you can start the trend :)
> 
>   #define OPERATOR_NEW_BODY_ALIGN(Type, Align, NoThrow) \
>     ...
>   #define OPERATOR_NEW_BODY(Type, Align, NoThrow) \
>     OPERATOR_NEW_BODY_ALIGN(Type, 0, NoThrow)
> 
> or pass the zero explicitly in the operators.
> 
> 
Ok will do. Are you doing it for delete as well?


Repository:
  rCRT Compiler Runtime

https://reviews.llvm.org/D48031