[PATCH] D54879: Introduce `LocalAddressSpaceView::LoadWritable(...)` and make the `Load(...)` method return a const pointer.

[Dan Liew via Phabricator via llvm-commits]

delcypher marked an inline comment as done.
delcypher added a comment.

@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.



================
Comment at: lib/sanitizer_common/sanitizer_local_address_space_view.h:43
+  template <typename T>
+  static const T *Load(T *target_address, uptr num_elements = 1) {
+    // The target address space is the local address space so
----------------
vitalybuka wrote:
> vitalybuka wrote:
> > vitalybuka wrote:
> > > delcypher wrote:
> > > > vitalybuka wrote:
> > > > > const T *target_addres?
> > > > > and then you maybe can remove Writable from the another name
> > > > @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?
> > > 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?
> > Could you please respond to one above?
> input arg should be const?
> ```
> static const T *Load(const T *target_address, uptr num_elements = 1)
> ```
> 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


Repository:
  rCRT Compiler Runtime

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54879/new/

https://reviews.llvm.org/D54879