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

[Kostya Kortchinsky via Phabricator via llvm-commits]

cryptoad marked 2 inline comments as done.
cryptoad 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)
----------------
alekseyshl wrote:
> 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.
I agree it's a bit obscure, all the more since the primary doesn't take the alignment as parameter, but is expected to produced a 2^x aligned chunked when requested 2^x bytes (https://github.com/llvm-mirror/compiler-rt/blob/master/lib/sanitizer_common/sanitizer_allocator_combined.h#L20).
It sort of messes up the logic as the secondary is being passed the alignment as a parameter, and it actually matters for the secondary.
I am opened to suggestion as to how it would make it clearer for you.


https://reviews.llvm.org/D27428