[PATCH] D58184: [scudo][standalone] Introduce platform specific code & mutexes

[Roland McGrath via Phabricator via llvm-commits]

mcgrathr added a comment.

I only really looked at the Fuchsia code.



================
Comment at: lib/scudo/standalone/common.h:140
+
+#define MAP_ERROR (void *)-1
+
----------------
Parens?


================
Comment at: lib/scudo/standalone/common.h:149
+// platform specific data to the function.
+// Returns MAP_ERROR on error or dies if MAP_ALLOWNOMEM is not specified.
+void *map(void *Addr, uptr Size, const char *Name, uptr Flags = 0,
----------------
Is it really useful to have MAP_ERROR instead of just using nullptr here?
In the traditional mmap interface, MAP_FAILED is not 0 because it's sometimes possible to actually mmap address 0.
But that's not relevant here, so why not just stick the standard C++?


================
Comment at: lib/scudo/standalone/fuchsia.cc:31
+
+void NORETURN die() { abort(); }
+
----------------
We mostly just use __builtin_trap() directly.


================
Comment at: lib/scudo/standalone/fuchsia.cc:50
+  const zx_status_t Status =
+      _zx_vmar_allocate_old(_zx_vmar_root_self(), 0, Size,
+                            ZX_VM_FLAG_CAN_MAP_READ | ZX_VM_FLAG_CAN_MAP_WRITE |
----------------
Use _zx_vmar_allocate, the new API.  Parameters are in a different order, and flags are spelled ZX_VM_* rather than ZX_VM_FLAG_*.


================
Comment at: lib/scudo/standalone/fuchsia.cc:66
+  zx_info_vmar_t Info;
+  const zx_status_t Status = _zx_object_get_info(
+      Vmar, ZX_INFO_VMAR, &Info, sizeof(Info), nullptr, nullptr);
----------------
You're only using VMARs you created yourself, so why use a syscall to get the bounds rather than keeping track locally?


================
Comment at: lib/scudo/standalone/fuchsia.cc:176
+
+const char *getEnv(const char *Name) {
+  if (StoredEnviron) {
----------------
Why doesn't this just call libc's getenv?


================
Comment at: lib/scudo/standalone/fuchsia.cc:185
+  const zx_status_t Status =
+      _zx_futex_wait_deprecated(reinterpret_cast<zx_futex_t *>(OpaqueStorage),
+                                MtxSleeping, ZX_TIME_INFINITE);
----------------
Use _zx_futex_wait.  It just needs an extra ZX_HANDLE_INVALID parameter (second to last).


================
Comment at: lib/scudo/standalone/fuchsia.cc:199
+
+u32 getNumberOfCPUs() { return zx_system_get_num_cpus(); }
+
----------------
_zx


================
Comment at: lib/scudo/standalone/fuchsia.cc:202
+bool getRandom(void *Buffer, uptr Length, bool Blocking) {
+  if (!Buffer || !Length || Length > 256U)
+    return false;
----------------
Where does the magic 256 constant come from?
If it's part of the getRandom API then it should be a macro or constexpr in the OS-independent headers.


================
Comment at: lib/scudo/standalone/fuchsia.cc:211
+  static StaticSpinMutex Mutex;
+  SpinMutexLock L(&Mutex);
+  uptr N = 0;
----------------
There no need for locking around this.


================
Comment at: lib/scudo/standalone/internal_defs.h:36
+// variable declaration is not portable.
 #define ALIGNED(x) __attribute__((aligned(x)))
 #define FORMAT(f, a) __attribute__((format(printf, f, a)))
----------------
Why not just use C++ standard alignas?


================
Comment at: lib/scudo/standalone/internal_defs.h:126
+
+// Various integer constants.
+
----------------
Why not use <cstdint>?


Repository:
  rCRT Compiler Runtime

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

https://reviews.llvm.org/D58184