This is an archive of the discontinued LLVM Phabricator instance.

[fuchsia] Update Fuchsia with a new mmap implementation.
ClosedPublic

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

Details

Summary
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 https://reviews.llvm.org/D38593
* part 2 https://reviews.llvm.org/D38592
* part 3 https://reviews.llvm.org/D38593

Diff Detail

Event Timeline

flowerhack created this revision.Oct 5 2017, 12:39 PM
mcgrathr added inline comments.
compiler-rt/lib/sanitizer_common/sanitizer_fuchsia.cc
242

Could be DCHECK_EQ, since it should be impossible.

248

Pass name, not a constant string.

272

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

300

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.

lib/sanitizer_common/sanitizer_common.h
145 ↗(On Diff #123441)

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?

lib/sanitizer_common/sanitizer_fuchsia.cc
273 ↗(On Diff #123441)

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.

276 ↗(On Diff #123441)

Doesn't that fit on 1 line?

mcgrathr added inline comments.Nov 20 2017, 1:13 PM
lib/sanitizer_common/sanitizer_common.h
145 ↗(On Diff #123441)

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

lib/sanitizer_common/sanitizer_fuchsia.cc
248 ↗(On Diff #123441)

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'?

255 ↗(On Diff #123441)

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

258 ↗(On Diff #123441)

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

273 ↗(On Diff #123441)

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
lib/sanitizer_common/sanitizer_common.h
145 ↗(On Diff #123441)

done.

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

cryptoad added inline comments.
lib/sanitizer_common/sanitizer_common.h
145 ↗(On Diff #123441)

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
lib/sanitizer_common/sanitizer_common.h
145 ↗(On Diff #123441)

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

flowerhack marked 3 inline comments as done.

updated

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