[fuchsia] Update Fuchsia with a new mmap implementation.

[flowerhack]

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

[mcgrathr]

compiler-rt/lib/sanitizer_common/sanitizer_fuchsia.cc

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.

[flowerhack]

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

[cryptoad]

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

lib/sanitizer_common/sanitizer_common.h

145

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

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

Doesn't that fit on 1 line?

[mcgrathr]

lib/sanitizer_common/sanitizer_common.h

145

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

lib/sanitizer_common/sanitizer_fuchsia.cc

248

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

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

258

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

273

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]

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

[flowerhack]

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

[flowerhack]

lib/sanitizer_common/sanitizer_common.h

145

done.

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

[cryptoad]

lib/sanitizer_common/sanitizer_common.h

145

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]

lib/sanitizer_common/sanitizer_common.h

145

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

[flowerhack]

updated

[flowerhack]

rebase

lib/sanitizer_common/sanitizer_common.h

public:
   void Unmap(uptr addr, uptr size);
   void *base() const { return base_; }
   uptr size() const { return size_; }
 
  private:
   void* base_;
   uptr size_;
   const char* name_;
+  unsigned int address_space_handle;
 };
 
 typedef void (*fill_profile_f)(uptr start, uptr rss, bool file,
                                /*out*/uptr *stats, uptr stats_size);
 
 // Parse the contents of /proc/self/smaps and generate a memory profile.
 // |cb| is a tool-specific callback that fills the |stats| array containing
 // |stats_size| elements.

lib/sanitizer_common/sanitizer_fuchsia.cc

 }
 
 void *MmapOrDieOnFatalError(uptr size, const char *mem_type) {
   return DoAnonymousMmapOrDie(size, mem_type, false, false);
 }
 
 uptr ReservedAddressRange::Init(uptr init_size, const char* name,
                                 uptr fixed_addr) {
-  base_ = MmapNoAccess(init_size);
-  size_ = init_size;
-  name_ = name;
-  return reinterpret_cast<uptr>(base_);
-}
-
-// Uses fixed_addr for now.
-// Will use offset instead once we've implemented this function for real.
-uptr ReservedAddressRange::Map(uptr fixed_addr, uptr map_size) {
-  return reinterpret_cast<uptr>(MmapFixedOrDieOnFatalError(fixed_addr,
-                                                           map_size));
-}
-
-uptr ReservedAddressRange::MapOrDie(uptr fixed_addr, uptr map_size) {
-  return reinterpret_cast<uptr>(MmapFixedOrDie(fixed_addr, map_size));
-}
-
-void ReservedAddressRange::Unmap(uptr addr, uptr size) {
-  void* addr_as_void = reinterpret_cast<void*>(addr);
-  uptr base_as_uptr = reinterpret_cast<uptr>(base_);
-  // Only unmap at the beginning or end of the range.
-  CHECK((addr_as_void == base_) || (addr + size == base_as_uptr + size_));
-  CHECK_LE(size, size_);
-  UnmapOrDie(reinterpret_cast<void*>(addr), size);
-  if (addr_as_void == base_) {
-    base_ = reinterpret_cast<void*>(addr + size);
-  }
-  size_ = size_ - size;
-}
-
-// MmapNoAccess and MmapFixedOrDie are used only by sanitizer_allocator.
-// Instead of doing exactly what they say, we make MmapNoAccess actually
-// just allocate a VMAR to reserve the address space.  Then MmapFixedOrDie
-// uses that VMAR instead of the root.
-
-zx_handle_t allocator_vmar = ZX_HANDLE_INVALID;
-uintptr_t allocator_vmar_base;
-size_t allocator_vmar_size;
-
-void *MmapNoAccess(uptr size) {
-  size = RoundUpTo(size, PAGE_SIZE);
-  CHECK_EQ(allocator_vmar, ZX_HANDLE_INVALID);
+  init_size = RoundUpTo(init_size, PAGE_SIZE);
+  DCHECK_EQ(address_space_handle, ZX_HANDLE_INVALID);
   uintptr_t base;
-  zx_status_t status =
-      _zx_vmar_allocate(_zx_vmar_root_self(), 0, size,
+  zx_handle_t usable_address_space_handle = static_cast<zx_handle_t>(address_space_handle);
+  zx_status_t status = _zx_vmar_allocate(
+                         _zx_vmar_root_self(), 0, init_size,
                          ZX_VM_FLAG_CAN_MAP_READ | ZX_VM_FLAG_CAN_MAP_WRITE |
-                            ZX_VM_FLAG_CAN_MAP_SPECIFIC,
-                        &allocator_vmar, &base);
+                         ZX_VM_FLAG_CAN_MAP_SPECIFIC, &usable_address_space_handle, &base);
   if (status != ZX_OK)
-    ReportMmapFailureAndDie(size, "sanitizer allocator address space",
-                            "zx_vmar_allocate", status);
+    ReportMmapFailureAndDie(init_size, name, "zx_vmar_allocate", status);
+  base_ = reinterpret_cast<void*>(base);
+  size_ = init_size;
+  name_ = name;
+  address_space_handle = static_cast<unsigned int>(usable_address_space_handle);
 
-  allocator_vmar_base = base;
-  allocator_vmar_size = size;
-  return reinterpret_cast<void *>(base);
+  return reinterpret_cast<uptr>(base_);
 }
 
-constexpr const char kAllocatorVmoName[] = "sanitizer_allocator";
-
-static void *DoMmapFixedOrDie(uptr fixed_addr, uptr size, bool die_for_nomem) {
-  size = RoundUpTo(size, PAGE_SIZE);
-
+static uptr DoMmapFixedOrDie(zx_handle_t vmar, uptr fixed_addr, uptr map_size,
+                             void* base, const char* name, bool die_for_nomem) 
+{
+  uptr offset = fixed_addr - reinterpret_cast<uptr>(base);
+  map_size = RoundUpTo(map_size, PAGE_SIZE);
   zx_handle_t vmo;
-  zx_status_t status = _zx_vmo_create(size, 0, &vmo);
+  zx_status_t status = _zx_vmo_create(map_size, 0, &vmo);
   if (status != ZX_OK) {
     if (status != ZX_ERR_NO_MEMORY || die_for_nomem)
-      ReportMmapFailureAndDie(size, kAllocatorVmoName, "zx_vmo_create", status);
-    return nullptr;
+      ReportMmapFailureAndDie(map_size, name, "zx_vmo_create", status);
+    return reinterpret_cast<uptr>(nullptr);
   }
-  _zx_object_set_property(vmo, ZX_PROP_NAME, kAllocatorVmoName,
-                          sizeof(kAllocatorVmoName) - 1);
-
-  DCHECK_GE(fixed_addr, allocator_vmar_base);
-  uintptr_t offset = fixed_addr - allocator_vmar_base;
-  DCHECK_LE(size, allocator_vmar_size);
-  DCHECK_GE(allocator_vmar_size - offset, size);
-
+  _zx_object_set_property(vmo, ZX_PROP_NAME, name,
+                          sizeof(name) - 1);
+  DCHECK_GE(base + size_, map_size + offset);
   uintptr_t addr;
+
   status = _zx_vmar_map(
-      allocator_vmar, offset, vmo, 0, size,
-      ZX_VM_FLAG_PERM_READ | ZX_VM_FLAG_PERM_WRITE | ZX_VM_FLAG_SPECIFIC,
-      &addr);
+    vmar, offset, vmo, 0, map_size,
   _zx_handle_close(vmo);
   if (status != ZX_OK) {
-    if (status != ZX_ERR_NO_MEMORY || die_for_nomem)
-      ReportMmapFailureAndDie(size, kAllocatorVmoName, "zx_vmar_map", status);
-    return nullptr;
+    if (status != ZX_ERR_NO_MEMORY || die_for_nomem) {
+      ReportMmapFailureAndDie(map_size, name, "zx_vmar_map", status);
+    }
+    return reinterpret_cast<uptr>(nullptr);
   }
+  IncreaseTotalMmap(map_size);
+  return addr;
 
-  IncreaseTotalMmap(size);
+}
 
-  return reinterpret_cast<void *>(addr);
+uptr ReservedAddressRange::Map(uptr fixed_addr, uptr map_size) {
+  return DoMmapFixedOrDie(address_space_handle, fixed_addr, map_size, base_, name_, false);
 }
 
-void *MmapFixedOrDie(uptr fixed_addr, uptr size) {
-  return DoMmapFixedOrDie(fixed_addr, size, true);
+uptr ReservedAddressRange::MapOrDie(uptr offset, uptr map_size) {
+  return DoMmapFixedOrDie(address_space_handle, fixed_addr, map_size, base_, name_, true)
 }
 
-void *MmapFixedOrDieOnFatalError(uptr fixed_addr, uptr size) {
-  return DoMmapFixedOrDie(fixed_addr, size, false);
+void ReservedAddressRange::Unmap(uptr fixed_addr, uptr size) {
+  uptr offset = fixed_addr - reinterpret_cast<uptr>(base_);
+  uptr addr = reinterpret_cast<uptr>(base_) + offset;
+  void* addr_as_void = reinterpret_cast<void*>(addr);
+  uptr base_as_uptr = reinterpret_cast<uptr>(base_);
+  // Only unmap at the beginning or end of the range.
+  CHECK((addr_as_void == base_) || (addr + size == base_as_uptr + size_));
+  CHECK_LE(size, size_);
+  UnmapOrDieVmar(reinterpret_cast<void*>(addr), size, static_cast<zx_handle_t>(address_space_handle));
+  if (addr_as_void == base_) {
+    base_ = reinterpret_cast<void*>(addr + size);
+  }
+  size_ = size_ - size;
 }
 
 // This should never be called.
 void *MmapFixedNoAccess(uptr fixed_addr, uptr size, const char *name) {
   UNIMPLEMENTED();
 }
 
 void *MmapAlignedOrDieOnFatalError(uptr size, uptr alignment,
if (status != ZX_OK) {
     return nullptr;
   }
 
   IncreaseTotalMmap(size);
 
   return reinterpret_cast<void *>(addr);
 }
 
-void UnmapOrDie(void *addr, uptr size) {
+void UnmapOrDieVmar(void *addr, uptr size, zx_handle_t target_vmar) {
   if (!addr || !size) return;
   size = RoundUpTo(size, PAGE_SIZE);
 
-  zx_status_t status = _zx_vmar_unmap(_zx_vmar_root_self(),
+  zx_status_t status = _zx_vmar_unmap(target_vmar,
                                       reinterpret_cast<uintptr_t>(addr), size);
   if (status != ZX_OK) {
     Report("ERROR: %s failed to deallocate 0x%zx (%zd) bytes at address %p\n",
            SanitizerToolName, size, size, addr);
     CHECK("unable to unmap" && 0);
   }
 
   DecreaseTotalMmap(size);
 }
 
+void UnmapOrDie(void *addr, uptr size) {
+  UnmapOrDieVmar(addr, size, _zx_vmar_root_self());
+}
+
 // This is used on the shadow mapping, which cannot be changed.
 // Zircon doesn't have anything like MADV_DONTNEED.
 void ReleaseMemoryPagesToOS(uptr beg, uptr end) {}
 
 void DumpProcessMap() {
-  UNIMPLEMENTED();  // TODO(mcgrathr): write it
+  // TODO(mcgrathr): write it
+  return;
 }
 
 bool IsAccessibleMemoryRange(uptr beg, uptr size) {
   // TODO(mcgrathr): Figure out a better way.
   zx_handle_t vmo;
   zx_status_t status = _zx_vmo_create(size, 0, &vmo);
   if (status == ZX_OK) {
     while (size > 0) {

 bool GetRandom(void *buffer, uptr length, bool blocking) {
   CHECK_LE(length, ZX_CPRNG_DRAW_MAX_LEN);
   size_t size;
   CHECK_EQ(_zx_cprng_draw(buffer, length, &size), ZX_OK);
   CHECK_EQ(size, length);
   return true;
 }
 
+uptr GetRSS() {
+  UNIMPLEMENTED();
+}
+
 }  // namespace __sanitizer
 
 using namespace __sanitizer;  // NOLINT
 
 extern "C" {
 void __sanitizer_startup_hook(int argc, char **argv, char **envp,
                               void *stack_base, size_t stack_size) {
   __sanitizer::StoredArgv = argv;