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

[Vitaly Buka via Phabricator via llvm-commits]

vitalybuka accepted this revision.
vitalybuka added inline comments.
This revision is now accepted and ready to land.


================
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:
> 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?


================
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:
> 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 


================
Comment at: lib/msan/msan_allocator.cc:231
 
+inline void *check_ptr(void *ptr) {
+  if (UNLIKELY(!ptr))
----------------
maybe move to some common header?


https://reviews.llvm.org/D35275