[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

Could be DCHECK_EQ, since it should be impossible.

248

Pass name, not a constant string.

272

This should not exist here. The caller of Init supplies the name and that should be stored and used in all cases.

300

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 ↗

(On Diff #123441)

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 ↗

(On Diff #123441)

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 ↗

(On Diff #123441)

Doesn't that fit on 1 line?

[mcgrathr]

lib/sanitizer_common/sanitizer_common.h

145 ↗

(On Diff #123441)

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

lib/sanitizer_common/sanitizer_fuchsia.cc

248 ↗

(On Diff #123441)

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 ↗

(On Diff #123441)

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

258 ↗

(On Diff #123441)

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

273 ↗

(On Diff #123441)

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 ↗

(On Diff #123441)

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 ↗

(On Diff #123441)

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 ↗

(On Diff #123441)

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

[flowerhack]

updated

[flowerhack]

rebase

compiler-rt/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 = nullptr,
                                 uptr fixed_addr = uptr(0)) {
-  return reinterpret_cast<uptr>(MmapNoAccess(init_size));
+  init_size = RoundUpTo(init_size, PAGE_SIZE);
+  CHECK_EQ(os_cookie_, ZX_HANDLE_INVALID);
+  uintptr_t base;
+  mx_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, &os_cookie_, &base);
+  if (status != ZX_OK)
+    ReportMmapFailureAndDie(init_size, "sanitizer allocator address space", "zx_vmar_allocate", status);
+  base_ = base;
+  size_ = init_size;
+
+  return 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,
                                bool tolerate_enomem = true) {
   return reinterpret_cast<uptr>(MmapFixedOrDie(fixed_addr, size,
                                                tolerate_enomem));
 }
 
 // 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);
-  uintptr_t base;
-  zx_status_t status =
-      _zx_vmar_allocate(_zx_vmar_root_self(), 0, size,
-                        ZX_VM_FLAG_CAN_MAP_READ | ZX_VM_FLAG_CAN_MAP_WRITE |
-                            ZX_VM_FLAG_CAN_MAP_SPECIFIC,
-                        &allocator_vmar, &base);
-  if (status != ZX_OK)
-    ReportMmapFailureAndDie(size, "sanitizer allocator address space",
-                            "zx_vmar_allocate", status);
+constexpr const char kAllocatorVmoName[] = "sanitizer_allocator";
 
-  allocator_vmar_base = base;
-  allocator_vmar_size = size;
-  return reinterpret_cast<void *>(base);
+uptr ReservedAddressRange::Map(uptr offset, uptr map_size,
+                                bool tolerate_enomem = true) {
+  map_size = RoundUpTo(map_size, PAGE_SIZE);
+  zx_handle_t 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(map_size, kAllocatorVmoName,
+                              "zx_vmo_create", status());
+    return nullptr;
+  }
+  _zx_object_set_property(vmo, ZX_PROP_NAME, kAllocatorVmoName,
+                          sizeof(kAllocatorVmoName) - 1);
+  DCHECK_GE(base_ + size_, map_size + offset);
+  uintptr_t addr;
+  status = _zx_vmar_map(
+    static_cast<zx_handle_t>(os_cookie_), offset, vmo, 0, 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;
+  }
+  IncreaseTotalMmap(map_size);
+  return reinterpret_cast<uptr>(addr);
 }
-
-constexpr const char kAllocatorVmoName[] = "sanitizer_allocator";
 
 static void *DoMmapFixedOrDie(uptr fixed_addr, uptr size, 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) {
     if (status != ZX_ERR_NO_MEMORY || die_for_nomem)