[PATCH] D27428: [sanitizer] Do not use the alignment-rounded-up size when using the secondary

[Aleksey Shlyapnikov via Phabricator via llvm-commits]

alekseyshl added inline comments.


================
Comment at: lib/sanitizer_common/sanitizer_allocator_combined.h:62
     if (alignment > 8)
       CHECK_EQ(reinterpret_cast<uptr>(res) & (alignment - 1), 0);
     if (cleared && res && from_primary)
----------------
cryptoad wrote:
> alekseyshl wrote:
> > Is it safe to run this CHECK now, when secondary is using non-adjusted allocation size? 
> sanitizer_allocator_secondary.h [[ https://github.com/llvm-mirror/compiler-rt/blob/master/lib/sanitizer_common/sanitizer_allocator_secondary.h#L37 | adds ]] alignment to the size prior to allocation and align the result properly.
> It also does a similar CHECK there.
> Same with the Scudo allocator, but the job is split between the frontend and the secondary.
> So the CHECK is still safe.
Understood, but this check feels a bit arbitrary now (to me, that is). Before your change it matched well with the similar if statement adjusting 'size', now it is puzzling why, while both allocators accept the alignment as a parameter, the resulting pointer is expected to be aligned only when alignment is greater than 8?
I bet there's a reason, but it's not clear from the code, which just become more obscure.


================
Comment at: lib/sanitizer_common/sanitizer_allocator_combined.h:64
     if (cleared && res && from_primary)
       internal_bzero_aligned16(res, RoundUpTo(size, 16));
     return res;
----------------
cryptoad wrote:
> alekseyshl wrote:
> > Same here, it tries to zero out the result up to the rounded up size, not the original size.
> If the allocation comes from the secondary, it's mmap backed and it's zero'd out, and we don't need to clear the memory again.
> We need to zero out only when it comes from the primary, which uses the rounded size rather than the original size for its allocation.
Ah, right, there's '&& from_primary' check... Then it seems to make sense to push 'cleared' flag down to allocators, since the particular allocator knows better the current state of the memory block. Anyway, not for this patch.


https://reviews.llvm.org/D27428