This is an archive of the discontinued LLVM Phabricator instance.

[scudo] Ensure proper allocator alignment in TSD test
ClosedPublic

Authored by cryptoad on Jun 16 2021, 10:52 AM.

Details

Summary

The MockAllocator used in ScudoTSDTest wasn't allocated
properly aligned, which resulted in the TSDs of the shared
registry not being aligned either. This lead to some failures
like: https://reviews.llvm.org/D103119#2822008

This changes how the MockAllocator is allocated, same as
Vitaly did in the combined tests, properly aligning it, which
results in the TSDs being aligned as well.

Add a DCHECK in the shared registry to check that it is.

Diff Detail

Event Timeline

cryptoad requested review of this revision.Jun 16 2021, 10:52 AM
cryptoad created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptJun 16 2021, 10:52 AM
Herald added a subscriber: Restricted Project. · View Herald Transcript
cryptoad updated this revision to Diff 352483.Jun 16 2021, 10:56 AM

Add stdlib.h to the test for posix_memalign. My builds don't
complain but I assume something will eventually.

yroux accepted this revision.Jun 16 2021, 11:22 AM

LGTM, I confirm that it fixes the alignement issue observed on Thumbv7 bots

This revision is now accepted and ready to land.Jun 16 2021, 11:22 AM
vitalybuka added inline comments.Jun 16 2021, 11:41 AM
compiler-rt/lib/scudo/standalone/tsd_shared.h
157 ↗(On Diff #352483)

Should we just check isAligned(this, alignof(*this)) ?

vitalybuka added inline comments.Jun 16 2021, 11:47 AM
compiler-rt/lib/scudo/standalone/tsd_shared.h
157 ↗(On Diff #352483)

I guess alignas in allocator promises alignment which is not delivered buy "new"
So maybe more consistent solution is just to put DCHECK(isAligned) into every class with alignas statement.

vitalybuka added inline comments.Jun 16 2021, 11:50 AM
compiler-rt/lib/scudo/standalone/tsd_shared.h
157 ↗(On Diff #352483)

e.g.
GlobalQuarantine::Init
SizeClassAllocator64::Init
SizeClassAllocator32::SizeClassAllocator32
TSD::Init

yroux added inline comments.Jun 16 2021, 12:36 PM
compiler-rt/lib/scudo/standalone/tsd_shared.h
157 ↗(On Diff #352483)

Ah yes, it seems better indeed.
I verified it on ARM and it fixes the issue as well

cryptoad added inline comments.Jun 16 2021, 1:50 PM
compiler-rt/lib/scudo/standalone/tsd_shared.h
157 ↗(On Diff #352483)

ack, working on it.

cryptoad updated this revision to Diff 352552.Jun 16 2021, 2:07 PM

Adding more isAligned DCHECK as Vitaly suggested.

alignof(*this) was triggering a warning in my build:

'alignof' applied to an expression is a GNU extension [-Wgnu-alignof-expression]

so I decided to use the ThisT construct instead which doesn't
exhibit the same issue.

vitalybuka accepted this revision.Jun 16 2021, 2:12 PM
vitalybuka added inline comments.
compiler-rt/lib/scudo/standalone/quarantine.h
173

this is the modern way do to typedefs
using ThisT = GlobalQuarantine<Callback, Node> ;

cryptoad updated this revision to Diff 352555.Jun 16 2021, 2:18 PM

Using using instead of typedef on the newly added ones.

This revision was landed with ongoing or failed builds.Jun 16 2021, 2:22 PM
This revision was automatically updated to reflect the committed changes.