This is an archive of the discontinued LLVM Phabricator instance.

[scudo] Add C++17 aligned new/delete operators support
ClosedPublic

Authored by cryptoad on Jun 11 2018, 9:21 AM.

Details

Summary

This CL adds support for aligned new/delete operators (C++17). Currently we
do not support alignment inconsistency detection on deallocation, as this
requires a header change, but the APIs are introduced and are functional.

Add a smoke test for the aligned version of the operators.

Event Timeline

cryptoad created this revision.Jun 11 2018, 9:21 AM
Herald added subscribers: Restricted Project, delcypher. · View Herald TranscriptJun 11 2018, 9:21 AM
alekseyshl added inline comments.Jun 11 2018, 11:12 AM
lib/scudo/scudo_allocator.cpp
633

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.

lib/scudo/scudo_new_delete.cpp
38

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.

cryptoad added inline comments.Jun 11 2018, 12:28 PM
lib/scudo/scudo_allocator.cpp
633

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?

lib/scudo/scudo_new_delete.cpp
38

Ok will do. Are you doing it for delete as well?

alekseyshl added inline comments.Jun 11 2018, 12:48 PM
lib/scudo/scudo_allocator.cpp
633

Perf data and looking at the generated code would be the only way to pick the right way to do it.

lib/scudo/scudo_new_delete.cpp
38

delete does not worth it, they are all one liners. New worries me because there's quite a lot of duplicated logic there, especially for ASan.

cryptoad updated this revision to Diff 150832.Jun 11 2018, 1:38 PM
cryptoad marked 3 inline comments as done.

OPERATOR_NEW_BODY now wraps OPERATOR_NEW_BODY_ALIGN.

cryptoad added inline comments.Jun 11 2018, 1:38 PM
lib/scudo/scudo_allocator.cpp
633

If that's ok with you I'd rather stick with checking for Alignment rather than expecting IsPowerOfTwo(0) to be true.

alekseyshl accepted this revision.Jun 11 2018, 4:56 PM
This revision is now accepted and ready to land.Jun 11 2018, 4:56 PM
This revision was automatically updated to reflect the committed changes.