[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

271

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.

274

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

251

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

254

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

257

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

271

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_;
+  uptr os_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

 
 sanitizer_shadow_bounds_t ShadowBounds;
 
 uptr GetMaxUserVirtualAddress() {
   ShadowBounds = __sanitizer_shadow_bounds();
   return ShadowBounds.memory_limit - 1;
 }
 
-uptr GetMaxVirtualAddress() {
-  return GetMaxUserVirtualAddress();
-}
+uptr GetMaxVirtualAddress() { return GetMaxUserVirtualAddress(); }
 
 static void *DoAnonymousMmapOrDie(uptr size, const char *mem_type,
                                   bool raw_report, bool die_for_nomem) {
   size = RoundUpTo(size, PAGE_SIZE);
 
   zx_handle_t vmo;
   zx_status_t status = _zx_vmo_create(size, 0, &vmo);
   if (status != ZX_OK) {

 void *MmapNoReserveOrDie(uptr size, const char *mem_type) {
   return MmapOrDie(size, mem_type);
 }
 
 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 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(os_handle_, ZX_HANDLE_INVALID);
   uintptr_t base;
+  zx_handle_t vmar;
   zx_status_t status =
-      _zx_vmar_allocate(_zx_vmar_root_self(), 0, size,
+      _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);
+                        &vmar, &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;
+  os_handle_ = vmar;
 
-  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 0;
   }
-  _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,
+      vmar, offset, vmo, 0, map_size,
       ZX_VM_FLAG_PERM_READ | ZX_VM_FLAG_PERM_WRITE | ZX_VM_FLAG_SPECIFIC,
       &addr);
   _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 0;
+  }
+  IncreaseTotalMmap(map_size);
+  return addr;
 
-  IncreaseTotalMmap(size);
+uptr ReservedAddressRange::Map(uptr fixed_addr, uptr map_size) {
+  return DoMmapFixedOrDie(os_handle_, fixed_addr, map_size, base_,
+                          name_, false);
+}
 
-  return reinterpret_cast<void *>(addr);
+uptr ReservedAddressRange::MapOrDie(uptr fixed_addr, uptr map_size) {
+  return DoMmapFixedOrDie(os_handle_, fixed_addr, map_size, base_,
+                          name_, true);
 }
 
-void *MmapFixedOrDie(uptr fixed_addr, uptr size) {
-  return DoMmapFixedOrDie(fixed_addr, size, true);
+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(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);
   }
 
-void *MmapFixedOrDieOnFatalError(uptr fixed_addr, uptr size) {
-  return DoMmapFixedOrDie(fixed_addr, size, false);
+  DecreaseTotalMmap(size);
+}
+
+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>(os_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,
void *MmapAlignedOrDieOnFatalError(uptr size, uptr alignment,
   }
 
   IncreaseTotalMmap(size);
 
   return reinterpret_cast<void *>(addr);
 }
 
 void UnmapOrDie(void *addr, uptr size) {
-  if (!addr || !size) return;
-  size = RoundUpTo(size, PAGE_SIZE);
-
-  zx_status_t status = _zx_vmar_unmap(_zx_vmar_root_self(),
-                                      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);
+  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_EQ(size, length);
   return true;
 }
 
 u32 GetNumberOfCPUs() {
   return zx_system_get_num_cpus();
 }
 
+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;

lib/sanitizer_common/sanitizer_posix_libcdep.cc

uptr ReservedAddressRange::Init(uptr size, const char *name, uptr fixed_addr) {
   // this problem.
   if (fixed_addr) {
     base_ = MmapFixedNoAccess(fixed_addr, size);
   } else {
     base_ = MmapNoAccess(size);
   }
   size_ = size;
   name_ = name;
+  (void)os_handle_;  // unsupported
   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 size) {
   return reinterpret_cast<uptr>(MmapFixedOrDieOnFatalError(fixed_addr, size));
 }

lib/sanitizer_common/sanitizer_win.cc

 uptr ReservedAddressRange::Init(uptr size, const char *name, uptr fixed_addr) {
   if (fixed_addr) {
     base_ = MmapFixedNoAccess(fixed_addr, size, name);
   } else {
     base_ = MmapNoAccess(size);
   }
   size_ = size;
   name_ = name;
+  (void)os_handle_;  // unsupported
   return reinterpret_cast<uptr>(base_);
 }
 
 
 void *MmapFixedNoAccess(uptr fixed_addr, uptr size, const char *name) {
   (void)name; // unsupported
   void *res = VirtualAlloc((LPVOID)fixed_addr, size,
                            MEM_RESERVE, PAGE_NOACCESS);