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

[Kostya Kortchinsky via Phabricator via llvm-commits]

cryptoad added inline comments.


================
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,
----------------
mcgrathr wrote:
> 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++?
Personal preference here. I thought that sanitizer_common was bad at mixing null & (void *)-1 for errors, and it did try to map 0 in some places.
So I went with all MAP_FAILED type error for map functions.
Though I am not opposed to all nullptr.


================
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);
----------------
mcgrathr wrote:
> You're only using VMARs you created yourself, so why use a syscall to get the bounds rather than keeping track locally?
It was mostly for storage reasons.
The vmar+vmo fits in a u64 which works well. Keeping the base & size would require 2 extra uptr, which to be fair is not the end of the world.
The extra 16 bytes would be for every secondary backed allocation & for every region of the primary.
I am open to doing either.



================
Comment at: lib/scudo/standalone/fuchsia.cc:176
+
+const char *getEnv(const char *Name) {
+  if (StoredEnviron) {
----------------
mcgrathr wrote:
> Why doesn't this just call libc's getenv?
I went the way it was done in sanitizer_common.
Part of the reason being that on some platform (Android), getenv would not work when called either too early or from the init process for example.
If this is not case for Fuchsia, happy to switch.


================
Comment at: lib/scudo/standalone/fuchsia.cc:202
+bool getRandom(void *Buffer, uptr Length, bool Blocking) {
+  if (!Buffer || !Length || Length > 256U)
+    return false;
----------------
mcgrathr wrote:
> 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.
256 is a magic Linux related constant.
getrandom doesn't get interrupted for sizes < 256.
And when reading /dev/urandom, reads of up to 256 bytes will return as many bytes as are requested and will not be interrupted by a signal handler.
Since we don't use large random streams of bytes, I enforced 256.
I will put that in the common header.


================
Comment at: lib/scudo/standalone/internal_defs.h:126
+
+// Various integer constants.
+
----------------
mcgrathr wrote:
> Why not use <cstdint>?
Right, it's mostly a sanitizer_common artifact, I haven't revisited it.
cstdint might not work with -nostdinc++ but stdint.h might.


Repository:
  rCRT Compiler Runtime

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

https://reviews.llvm.org/D58184