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

[delcypher]

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

[delcypher]

@kcc ping.

[vitalybuka]

lib/sanitizer_common/sanitizer_local_address_space_view.h

43 ↗

(On Diff #175179)

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

[delcypher]

lib/sanitizer_common/sanitizer_local_address_space_view.h

43 ↗

(On Diff #175179)

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

lib/sanitizer_common/sanitizer_local_address_space_view.h

43 ↗

(On Diff #175179)

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?

[vitalybuka]

lib/sanitizer_common/sanitizer_local_address_space_view.h

43 ↗

(On Diff #175179)

Could you please respond to one above?

43 ↗

(On Diff #175179)

input arg should be const?

static const T *Load(const T *target_address, uptr num_elements = 1)

[delcypher]

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

(On Diff #175179)

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]

Make Load's target_address parameter be const.

compiler-rt/trunk/lib/sanitizer_common/sanitizer_allocator_secondary.h

void *GetBlockBegin(const void *ptr) {
     CHECK_LE(nearest_chunk, p);
     if (h->map_beg + h->map_size <= p)
       return nullptr;
     return GetUser(h);
   }
 
   void EnsureSortedChunks() {
     if (chunks_sorted_) return;
-    Header **chunks = AddressSpaceView::Load(chunks_, n_chunks_);
+    Header **chunks = AddressSpaceView::LoadWritable(chunks_, n_chunks_);
     Sort(reinterpret_cast<uptr *>(chunks), n_chunks_);
     for (uptr i = 0; i < n_chunks_; i++)
-      AddressSpaceView::Load(chunks[i])->chunk_idx = i;
+      AddressSpaceView::LoadWritable(chunks[i])->chunk_idx = i;
     chunks_sorted_ = true;
   }
 
   // This function does the same as GetBlockBegin, but is much faster.
   // Must be called with the allocator locked.
   void *GetBlockBeginFastLocked(void *ptr) {
     mutex_.CheckLocked();
     uptr p = reinterpret_cast<uptr>(ptr);
public:
   void ForceUnlock() {
     mutex_.Unlock();
   }
 
   // Iterate over all existing chunks.
   // The allocator must be locked when calling this function.
   void ForEachChunk(ForEachChunkCallback callback, void *arg) {
     EnsureSortedChunks();  // Avoid doing the sort while iterating.
-    Header **chunks = AddressSpaceView::Load(chunks_, n_chunks_);
+    const Header *const *chunks = AddressSpaceView::Load(chunks_, n_chunks_);
     for (uptr i = 0; i < n_chunks_; i++) {
-      Header *t = chunks[i];
+      const Header *t = chunks[i];
       callback(reinterpret_cast<uptr>(GetUser(t)), arg);
       // Consistency check: verify that the array did not change.
       CHECK_EQ(chunks[i], t);
       CHECK_EQ(AddressSpaceView::Load(chunks[i])->chunk_idx, i);
     }
   }
 
  private:
   struct Header {
     uptr map_beg;
     uptr map_size;
     uptr size;
     uptr chunk_idx;
   };
 
   Header *GetHeader(uptr p) {
     CHECK(IsAligned(p, page_size_));
     return reinterpret_cast<Header*>(p - page_size_);
   }
   Header *GetHeader(const void *p) {
     return GetHeader(reinterpret_cast<uptr>(p));
   }
 
-  void *GetUser(Header *h) {
+  void *GetUser(const Header *h) {
     CHECK(IsAligned((uptr)h, page_size_));
     return reinterpret_cast<void*>(reinterpret_cast<uptr>(h) + page_size_);
   }
 
   uptr RoundUpMapSize(uptr size) {
     return RoundUpTo(size, page_size_) + page_size_;
   }
 

compiler-rt/trunk/lib/sanitizer_common/sanitizer_local_address_space_view.h

 // code duplication.
 //
 //===----------------------------------------------------------------------===//
 #ifndef SANITIZER_LOCAL_ADDRES_SPACE_VIEW_H
 #define SANITIZER_LOCAL_ADDRES_SPACE_VIEW_H
 
 namespace __sanitizer {
 struct LocalAddressSpaceView {
-  // Load memory `sizeof(T) * num_elements` bytes of memory
-  // from the target process (always local for this implementation)
-  // starting at address `target_address`. The local copy of
-  // this memory is returned as a pointer. It is guaranteed that
-  //
-  // * That the function will always return the same value
-  //   for a given set of arguments.
-  // * That the memory returned is writable and that writes will persist.
+  // Load memory `sizeof(T) * num_elements` bytes of memory from the target
+  // process (always local for this implementation) starting at address
+  // `target_address`. The local copy of this memory is returned as a pointer.
+  // The caller should not write to this memory. The behaviour when doing so is
+  // undefined. Callers should use `LoadWritable()` to get access to memory
+  // that is writable.
   //
   // The lifetime of loaded memory is implementation defined.
   template <typename T>
-  static T *Load(T *target_address, uptr num_elements = 1) {
+  static const T *Load(const T *target_address, uptr num_elements = 1) {
+    // The target address space is the local address space so
+    // nothing needs to be copied. Just return the pointer.
+    return target_address;
+  }
+
+  // Load memory `sizeof(T) * num_elements` bytes of memory from the target
+  // process (always local for this implementation) starting at address
+  // `target_address`. The local copy of this memory is returned as a pointer.
+  // The memory returned may be written to.
+  //
+  // Writes made to the returned memory will be visible in the memory returned
+  // by subsequent `Load()` or `LoadWritable()` calls provided the
+  // `target_address` parameter is the same. It is not guaranteed that the
+  // memory returned by previous calls to `Load()` will contain any performed
+  // writes.  If two or more overlapping regions of memory are loaded via
+  // separate calls to `LoadWritable()`, it is implementation defined whether
+  // writes made to the region returned by one call are visible in the regions
+  // returned by other calls.
+  //
+  // Given the above it is recommended to load the largest possible object
+  // that requires modification (e.g. a class) rather than individual fields
+  // from a class to avoid issues with overlapping writable regions.
+  //
+  // The lifetime of loaded memory is implementation defined.
+  template <typename T>
+  static T *LoadWritable(T *target_address, uptr num_elements = 1) {
     // The target address space is the local address space so
     // nothing needs to be copied. Just return the pointer.
     return target_address;
   }
 };
 }  // namespace __sanitizer
 
 #endif