This is an archive of the discontinued LLVM Phabricator instance.

[sanitizer] Update scudo to use new API
ClosedPublic

Authored by flowerhack on Oct 5 2017, 12:35 PM.

Details

Summary

The ScudoAllocator uses a SecondaryHeader to keep track of the size and base address of each mmap'd chunk.

This aligns well with what the ReservedAddressRange is trying to do. This changeset converts the scudo allocator from using the MmapNoAccess/MmapFixed APIs to the ReservedAddressRange::Init and ::Map APIs. In doing so, it replaces the SecondayHeader struct with the ReservedAddressRange object.

This is part 3 of a 4 part changeset; part 1 https://reviews.llvm.org/D39072 and part 2 https://reviews.llvm.org/D38592

Diff Detail

Event Timeline

flowerhack created this revision.Oct 5 2017, 12:35 PM

Some comments, even though it's not done yet.

compiler-rt/lib/sanitizer_common/sanitizer_common.h
87 ↗(On Diff #117866)

I think you can just pad the structure in the secondary or just declare it with ALIGNED there maybe.
Haven't tried either.

compiler-rt/lib/sanitizer_common/sanitizer_posix.cc
156 ↗(On Diff #117866)

That would be great to have that in D38437 in one form or another.

compiler-rt/lib/scudo/scudo_allocator_secondary.h
87 ↗(On Diff #117866)

I would avoid a placement new here and use a bit_cast or memcpy there.

126 ↗(On Diff #117866)

Might want to rename that API as to getReservedAddressRange.

flowerhack marked an inline comment as done.Oct 5 2017, 4:35 PM
flowerhack added inline comments.
compiler-rt/lib/sanitizer_common/sanitizer_posix.cc
156 ↗(On Diff #117866)

moved to D38437

flowerhack marked 3 inline comments as done.
flowerhack retitled this revision from DO NOT MERGE, WIP: Update scudo_allocator to use new API. to Update scudo_allocator to use new API..
flowerhack edited the summary of this revision. (Show Details)
flowerhack updated this revision to Diff 121821.Nov 6 2017, 5:18 PM
flowerhack marked an inline comment as done.

rebased against patch 2 of this series

feedback much appreciated!

flowerhack added inline comments.Nov 6 2017, 5:20 PM
compiler-rt/lib/sanitizer_common/sanitizer_common.h
87 ↗(On Diff #117866)

i switched this around at some point, but it turns out getting a specific alignment for an instance of a class, outside of the declaration of that class, is pretty much Not A Thing.

there's a way to do it with placement new, but, seemed more straightforward to subclass ReservedAddressRange, s.t. ScudoReservedAddressRange is properly-aligned but otherwise identical. lmk if this is a bad idea!

cryptoad edited edge metadata.Nov 7 2017, 9:07 AM

On the form:

On the content, I think we can get rid of ScudoReservedAddressRange and only work with a ReservedAddressRange.
We don't have to make the structure a multiple of MinAlignment itself, but just account for it in space computations. So Introduce something like:
uptr AlignedReservedAddressRangeSize = RoundUpTo(sizeof(ReservedAddressRange), MinAlignment);
Then compute the mapping size using that.
You can just store and load the ReservedAddressRange at Ptr-sizeof(sizeof(ReservedAddressRange)) , this will just result in potential empty space before the structure.
Also instead of using the Swap construct you should be able to memcpy the structure (or assign it, it should be POD), it will be inlined.

flowerhack updated this revision to Diff 121998.Nov 7 2017, 3:26 PM

Some style comments for the Scudo part.
And a few other changes.

lib/scudo/scudo_allocator_secondary.h
83–84

ALIGNED(MinAlignment) shouldn't be necessary?

105

This is a somewhat awkward new line, since it's aligned on the content of the cast as opposed to the parameter itself.

109

s/MapSize/ReservedAddressRange size. Or something like that.

111

Same alignment comment as above.

118–119

This can go away.

125–126

You can do something like:

const uptr AlignedReservedAddressRangeSize =
    RoundUpTo(sizeof(ReservedAddressRange), MinAlignment);

Same a line and the arithmetic expression is on the same line.

flowerhack marked 6 inline comments as done.
cryptoad retitled this revision from Update scudo_allocator to use new API. to [sanitizer] Update scudo to use new API.Nov 8 2017, 11:21 AM
cryptoad added a subscriber: llvm-commits.

LGTM pending tests. Could you rebase it please?

& tests pass + builds and works on Linux and Fuchsia

cryptoad added inline comments.Nov 8 2017, 1:26 PM
lib/scudo/scudo_allocator_secondary.h
104

Didn't catch that one before: but shouldn't that be StoredRange->Unmap()?

flowerhack marked an inline comment as done.
cryptoad added inline comments.Nov 10 2017, 9:43 AM
lib/scudo/scudo_allocator_secondary.h
98

You should be able to do directly ReservedAddressRange AddressRange = *getReservedAddressRange(Ptr); and then you don't have to use the StoredRange intermediate variable. Keep the comment though, but move it here.

flowerhack marked an inline comment as done.
cryptoad accepted this revision.Nov 10 2017, 12:06 PM

Tested Android & Linux locally, it's passing. LGTM.

This revision is now accepted and ready to land.Nov 10 2017, 12:06 PM
cryptoad updated this revision to Diff 122677.Nov 13 2017, 9:45 AM

Rebase & small style changes.

cryptoad updated this revision to Diff 122679.Nov 13 2017, 9:49 AM

Removing unneeded reinterpret_cast.

cryptoad closed this revision.Nov 13 2017, 12:38 PM