[PATCH] D90483: [GWP-ASan] Fuchsia specific mapping & utilities functions

[Roland McGrath via Phabricator via llvm-commits]

mcgrathr accepted this revision.
mcgrathr added a comment.

LGTM with a few minor changes.
Thanks!



================
Comment at: compiler-rt/lib/gwp_asan/platform_specific/common_fuchsia.cpp:12
+namespace gwp_asan {
+uint64_t getThreadID() { return kInvalidThreadID; }
+} // namespace gwp_asan
----------------
Perhaps comment that on Fuchsia this is only used for AllocationTrace.ThreadID and allocation traces are not yet supported on Fuchsia.


================
Comment at: compiler-rt/lib/gwp_asan/platform_specific/guarded_pool_allocator_fuchsia.cpp:23
+  _zx_cprng_draw(&RandomState, sizeof(RandomState));
+  getThreadLocals()->RandomState = RandomState;
+}
----------------
This is fine but I don't see why it's using the separate local and copying instead of passing `&getThreadLocals()->RandomState`.


================
Comment at: compiler-rt/lib/gwp_asan/platform_specific/guarded_pool_allocator_fuchsia.cpp:29
+  zx_handle_t Vmo;
+  zx_status_t Status = _zx_vmo_create(Size, ZX_VMO_RESIZABLE, &Vmo);
+  Check(Status == ZX_OK, "Failed to create Vmo");
----------------
This VMO does not need to be resizable.
It can just use 0 flags here.


================
Comment at: compiler-rt/lib/gwp_asan/platform_specific/guarded_pool_allocator_fuchsia.cpp:36
+                        0, Vmo, 0, Size, &Addr);
+  Check(Status == ZX_OK, "Vmo mapping failed");
+  _zx_handle_close(Vmo);
----------------
Is it possible for the failure message to include `Status` or `zx_status_get_string(Status)`?


================
Comment at: compiler-rt/lib/gwp_asan/platform_specific/guarded_pool_allocator_fuchsia.cpp:74
+  zx_handle_t Vmo;
+  zx_status_t Status = _zx_vmo_create(Size, ZX_VMO_RESIZABLE, &Vmo);
+  Check(Status == ZX_OK, "Failed to create vmo");
----------------
Doesn't need to be resizable.


================
Comment at: compiler-rt/lib/gwp_asan/platform_specific/guarded_pool_allocator_fuchsia.cpp:102
+
+size_t GuardedPoolAllocator::getPlatformPageSize() { return PAGE_SIZE; }
+
----------------
Use ZX_PAGE_SIZE from <zircon/limits.h> instead.


================
Comment at: compiler-rt/lib/gwp_asan/platform_specific/guarded_pool_allocator_fuchsia.h:9
+
+#if defined(__Fuchsia__)
+#ifndef GWP_ASAN_GUARDED_POOL_ALLOCATOR_FUCHSIA_H_
----------------
It seems weird to have any `#if` outside the header guard.  Obviously inverting the order makes no practical difference but I think it makes the code more readable to have it inside and separated by a blank line.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90483