This is an archive of the discontinued LLVM Phabricator instance.

Introduce `LocalAddressSpaceView::LoadWritable(...)` and make the `Load(...)` method return a const pointer.
ClosedPublic

Authored by delcypher on Nov 25 2018, 12:05 PM.

Details

Summary

This is a follow-up to r346956 (https://reviews.llvm.org/D53975).

The purpose of this change to allow implementers of the
AddressSpaceView to be able to distinguish between when a caller wants
read-only memory and when a caller wants writable memory. Being able
distinguish these cases allows implementations to optimize for the
different cases and also provides a way to workaround possible platform
restrictions (e.g. the low level platform interface for reading
out-of-process memory may place memory in read-only pages).

For allocator enumeration in almost all cases read-only is sufficient so
we make Load(...) take on this new requirement and introduce the
LoadWritable(...) variants for cases where memory needs to be
writable.

The behaviour of LoadWritable(...) documented in comments are
deliberately very restrictive so that it will be possible in the future
to implement a simple write-cache (i.e. just a map from target address
to a writable region of memory). These restrictions can be loosened in
the future if necessary by implementing a more sophisticated
write-cache.

rdar://problem/45284065

Event Timeline

delcypher created this revision.Nov 25 2018, 12:05 PM
Herald added a subscriber: Restricted Project. · View Herald TranscriptNov 25 2018, 12:05 PM
vitalybuka added inline comments.
lib/sanitizer_common/sanitizer_local_address_space_view.h
43

const T *target_addres?
and then you maybe can remove Writable from the another name

delcypher marked an inline comment as done.Dec 10 2018, 8:10 AM
delcypher added inline comments.
lib/sanitizer_common/sanitizer_local_address_space_view.h
43

@vitalybuka I considered this but this ends up putting the burden of giving Load(...) a const pointer on all callers.

This isn't good because it involves having to write a lot of reinterpret_cast<const Something*>(...) at the call sites. Given that getting a const pointer for enumeration of the allocator is the common case it makes more sense to force the caller to write the "longer" thing when we call site needs a non-const pointer (the uncommon case). So in that case the call-site uses LoadWritable(...) rather than Load(...). Does this make sense?

vitalybuka added inline comments.Dec 10 2018, 1:48 PM
lib/sanitizer_common/sanitizer_local_address_space_view.h
43

I am not sure what is going to be implementation of these.
Is there going to be any disadvantages of non-const vs const implementations?

delcypher marked an inline comment as not done.Dec 10 2018, 3:19 PM
vitalybuka added inline comments.Dec 13 2018, 5:39 PM
lib/sanitizer_common/sanitizer_local_address_space_view.h
43

Could you please respond to one above?

43

input arg should be const?

static const T *Load(const T *target_address, uptr num_elements = 1)
delcypher marked an inline comment as done.Dec 14 2018, 1:24 AM

@vitalybuka Sorry for the delay on responding to this. The reason for the delay is that I was testing whether this patch is actually needed. The original motivation was that in some cases the memory copied from another process on Darwin will be copied into read-only pages. Further testing actually suggests that this isn't the case when copying portions of memory from another process that were allocated by ASan's runtime (i.e. the memory copied is writable).

However this patch might be needed for optimization later on. When memory from another process (that was copied in as COW pages) gets written to this will trigger a copy of the page. On some Darwin platforms these pages are large (IIRC 16k) which is very wasteful given that writes are made to a very small portion of the page.

Given that I'm not sure if we need this patch yet, let's put landing this on hold and concentrate on other patches that definitely need to land.

lib/sanitizer_common/sanitizer_local_address_space_view.h
43

I am not sure what is going to be implementation of these.

Is there going to be any disadvantages of non-const vs const implementations?

Sorry the delay on this. For the implementations, the only disadvantages I can think right now is the fact that two different code paths need to be maintained.

For callers there are some disadvantages:

  • Callers of Load(...) is that they must be more careful about const-correctness otherwise the code won't compile.
  • When Load(...) is made and later a LoadWritable(...) to the same target_address the returned pointers might be different.
  • Callers need to avoid calling LoadWritable(...) on overlapping objects
delcypher updated this revision to Diff 178202.Dec 14 2018, 1:34 AM

Make Load's target_address parameter be const.

delcypher marked an inline comment as done.Dec 14 2018, 1:34 AM
vitalybuka accepted this revision.Dec 19 2018, 1:27 AM
This revision is now accepted and ready to land.Dec 19 2018, 1:27 AM
This revision was automatically updated to reflect the committed changes.