Page MenuHomePhabricator

[fuchsia] Update Fuchsia with a new mmap implementation.

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


Now that the sanitizer_common interface for MmapNoAccess / MmapFixed
have been refactored to allow a more OO-esque access pattern, update the
Fuchsia mmap implementation to take advantage of this.

Previously MmapNoAccess / MmapFixed relied on a global allocator_vmar,
since the sanitizer_allocator only called MmapNoAccess once.  Now, we
create a new VMAR per ReservedAddressRange object.

This allows the sanitizer allocator to work in tandem with the Scudo
secondary allocator.

This is part 4 of a 4 part changeset:
* part 1
* part 2
* part 3

Diff Detail

Event Timeline

flowerhack created this revision.Oct 5 2017, 12:39 PM
mcgrathr added inline comments.
242 ↗(On Diff #117868)

Could be DCHECK_EQ, since it should be impossible.

248 ↗(On Diff #117868)

Pass name, not a constant string.

272 ↗(On Diff #117868)

This should not exist here. The caller of Init supplies the name and that should be stored and used in all cases.

300 ↗(On Diff #117868)

static_cast or no cast at all, since uptr and uintptr_t are always actually the same type.

Real patch; updated with changes from patches 1-3, and ofc responded to initial comments/feedback.

flowerhack retitled this revision from DO NOT MERGE, WIP: Update Fuchsia with new mmap API implementation. to [fuchsia] Update Fuchsia with a new mmap implementation..Nov 17 2017, 3:36 PM
flowerhack edited the summary of this revision. (Show Details)
flowerhack added reviewers: mcgrathr, cryptoad.
cryptoad edited edge metadata.Nov 20 2017, 8:23 AM

Few comments inline.
Might want to lint the source, it seems all over.


I am assuming the other platforms are going to complain about a private member not used?
Maybe an uptr would be better as it would keep the structure more consistent?


I don't think nullptr is a valid error return value here.
mmap failure returns (void *)-1, and the callers usually check against (uptr)~0ULL or equivalent.


Doesn't that fit on 1 line?

mcgrathr added inline comments.Nov 20 2017, 1:13 PM

Trailing _.
Probably should be called something like "os_handle_" to clarify that its use is OS-specific.


The initialization doesn't make sense. It's overwritten by the call. You've already asserted that the value is ZX_HANDLE_INVALID.

Why not call it 'vmar'?


This code uses space before *. Just put the whole file through clang-format.


I don't think there's really any need for this cast. Default integer conversions are fine.


The only caller that accepts any failure return value is MapWithCallback, and it checks for a zero result.
At any rate, reinterpret_case<uptr>(nullptr) is a pretty strange way to write 0. Just write 0.

flowerhack marked 8 inline comments as done.

clang format'd, responded to comments, tested on fuchsia & linux

oops, previous patch didn't pick up all my changes for some reason?

flowerhack added inline comments.Nov 21 2017, 12:18 PM


is it ok to use a pragma to suppress the "private member not used" complaint (as i've done here)?

cryptoad added inline comments.

Windows seems to be the only big consummer of pragmas in sanitizer_common.
I assume it would be better to initialize the field in each Init? @alekseyshl

alekseyshl added inline comments.Nov 21 2017, 3:16 PM

Yes, I think it's better to initialize it in all implementations than to add another pragma.

flowerhack marked 3 inline comments as done.


cryptoad accepted this revision.Nov 22 2017, 3:07 PM
This revision is now accepted and ready to land.Nov 22 2017, 3:07 PM
cryptoad closed this revision.Nov 27 2017, 11:54 AM