[PATCH] D35275: [Sanitizers] MSan allocator set errno on failure.

[Aleksey Shlyapnikov via Phabricator via llvm-commits]

alekseyshl marked 3 inline comments as done.
alekseyshl added inline comments.


================
Comment at: lib/msan/msan_allocator.cc:269
+void *msan_aligned_alloc(uptr alignment, uptr size, StackTrace *stack) {
+  if (UNLIKELY(!IsPowerOfTwo(alignment) || (size & (alignment - 1)) != 0)) {
+    errno = errno_EINVAL;
----------------
vitalybuka wrote:
> vitalybuka wrote:
> > alekseyshl wrote:
> > > alekseyshl wrote:
> > > > vitalybuka wrote:
> > > > > morehouse wrote:
> > > > > > vitalybuka wrote:
> > > > > > > why not just (size % alignmet == 0) and ignore "power of two" check?
> > > > > > From memalign and aligned_alloc man page:
> > > > > > > The  obsolete  function  memalign()  allocates size bytes and returns a
> > > > > > > pointer to the allocated memory.  The memory address will be a multiple
> > > > > > > of alignment, which must be a power of two.
> > > > > > 
> > > > > > > The  function aligned_alloc() is the same as memalign(), except for the
> > > > > > > added restriction that size should be a multiple of alignment.
> > > > > power of two is posix specific requirement
> > > > > http://en.cppreference.com/w/c/memory/aligned_alloc
> > > > > 
> > > > > Anyway  (size & (alignment - 1)) != 0) can be replaced with simple (size % alignmet)
> > > > My man memalign says exactly as quoted earlier, the power of two requirement affects both memalign and aligned_alloc.
> > > > 
> > > > 
> > > (size & (alignment - 1)) != 0) leads to appreciably more compact and faster assembly generated than (size % alignment), because div required for the latter touches a lot of registers, which forces compiler to push other vars around.
> > > 
> > > The difference is 0x41 vs 0x51 bytes of the function size.
> > then why msan_posix_memalign uses % when you do the same?
> your man is for posix 
Cause it % with compile time constant (which is a power of 2), the compiler was capable to optimize % part of the condition to one bitmask operation there.


================
Comment at: lib/msan/msan_allocator.cc:269
+void *msan_aligned_alloc(uptr alignment, uptr size, StackTrace *stack) {
+  if (UNLIKELY(!IsPowerOfTwo(alignment) || (size & (alignment - 1)) != 0)) {
+    errno = errno_EINVAL;
----------------
alekseyshl wrote:
> vitalybuka wrote:
> > vitalybuka wrote:
> > > alekseyshl wrote:
> > > > alekseyshl wrote:
> > > > > vitalybuka wrote:
> > > > > > morehouse wrote:
> > > > > > > vitalybuka wrote:
> > > > > > > > why not just (size % alignmet == 0) and ignore "power of two" check?
> > > > > > > From memalign and aligned_alloc man page:
> > > > > > > > The  obsolete  function  memalign()  allocates size bytes and returns a
> > > > > > > > pointer to the allocated memory.  The memory address will be a multiple
> > > > > > > > of alignment, which must be a power of two.
> > > > > > > 
> > > > > > > > The  function aligned_alloc() is the same as memalign(), except for the
> > > > > > > > added restriction that size should be a multiple of alignment.
> > > > > > power of two is posix specific requirement
> > > > > > http://en.cppreference.com/w/c/memory/aligned_alloc
> > > > > > 
> > > > > > Anyway  (size & (alignment - 1)) != 0) can be replaced with simple (size % alignmet)
> > > > > My man memalign says exactly as quoted earlier, the power of two requirement affects both memalign and aligned_alloc.
> > > > > 
> > > > > 
> > > > (size & (alignment - 1)) != 0) leads to appreciably more compact and faster assembly generated than (size % alignment), because div required for the latter touches a lot of registers, which forces compiler to push other vars around.
> > > > 
> > > > The difference is 0x41 vs 0x51 bytes of the function size.
> > > then why msan_posix_memalign uses % when you do the same?
> > your man is for posix 
> Cause it % with compile time constant (which is a power of 2), the compiler was capable to optimize % part of the condition to one bitmask operation there.
Not sure how we can check for "valid alignment supported by the implementation" condition, but I can definitely relax it for non-POSIX platforms.


================
Comment at: lib/msan/msan_allocator.cc:231
 
+inline void *check_ptr(void *ptr) {
+  if (UNLIKELY(!ptr))
----------------
vitalybuka wrote:
> maybe move to some common header?
Ok, let's try to common-ise some of the logc here.


https://reviews.llvm.org/D35275