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

[Aleksey Shlyapnikov via Phabricator via llvm-commits]

alekseyshl 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;
----------------
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.


================
Comment at: lib/scudo/scudo_new_delete.cpp:38
+  if (!NoThrow && UNLIKELY(!Ptr)) DieOnFailure::OnOOM();           \
+  return Ptr;
+
----------------
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.




Repository:
  rCRT Compiler Runtime

https://reviews.llvm.org/D48031