[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:
> cryptoad wrote:
> > 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?
> Perf data and looking at the generated code would be the only way to pick the right way to do it.
If that's ok with you I'd rather stick with checking for Alignment rather than expecting IsPowerOfTwo(0) to be true.


Repository:
  rCRT Compiler Runtime

https://reviews.llvm.org/D48031