[PATCH] D26358: [scudo] 32-bit and hardware agnostic support

[Filipe Cabecinhas via llvm-commits]

filcab added a comment.

Here's a few things I found on a quick read.
Thanks for doing this,
Filipe



================
Comment at: lib/scudo/scudo_allocator.cpp:39
   static const uptr kMetadataSize = 0;
-  typedef DefaultSizeClassMap SizeClassMap;
+  typedef __scudo::SizeClassMap SizeClassMap;
   typedef NoOpMapUnmapCallback MapUnmapCallback;
----------------
Maybe just use `__scudo::DefaultSizeClassMap` and get rid of the above typedef?


================
Comment at: lib/scudo/scudo_allocator.cpp:310
+    Header.Offset = MaxOffset;
+    if (Header.Offset != MaxOffset) {
       dieWithMessage("ERROR: the maximum possible offset doesn't fit in the "
----------------
It seems all these values are constant. Can you turn this into a static_assert near their definitions?


================
Comment at: lib/scudo/scudo_allocator.cpp:321
+    Header.UnusedBytes = MaxUnusedBytes;
+    if (Header.UnusedBytes != MaxUnusedBytes) {
+      dieWithMessage("ERROR: the maximum possible unused bytes doesn't fit in "
----------------
Same thing as above.


================
Comment at: lib/scudo/scudo_allocator.cpp:460
   // by the caller to perform additional processing.
   uptr getUsableSize(const void *Ptr, UnpackedHeader *Header) {
     if (UNLIKELY(!ThreadInited))
----------------
I'd rather have separate utility functions for this.
One "unpacks" the header onto some var on the stack, then you can getUsableSize on it. Maybe even have one which checks if the chunk is good (allocated), but that might be abstracting too much unless we need those verifications often.


================
Comment at: lib/scudo/scudo_allocator.cpp:461
   uptr getUsableSize(const void *Ptr, UnpackedHeader *Header) {
     if (UNLIKELY(!ThreadInited))
       initThread();
----------------
Do we need this if? What goes wrong if we emit it?


================
Comment at: lib/scudo/scudo_allocator.h:97
 
+// Scudo secondary allocator is defined in the following header.
 #include "scudo_allocator_secondary.h"
----------------
Do we need this comment?


================
Comment at: lib/scudo/scudo_utils.cpp:89
     if (isSupportedCPU() == true)
-      getCPUID(&CPUInfo, 1, 0);
-    else
-      UNIMPLEMENTED();
+      getCPUID(&CPUInfo, 1);
     InfoInitialized = true;
----------------
You'll get data races on `CPUInfo`.
Since those numbers never change, how about writing to the static one register by register, atomically?
Relaxed stores would be enough (not contributing to data races, at most you'd write more than once the exact same value).
You'll also need to load them with atomic loads (which on x86 is a simple mov).


================
Comment at: lib/scudo/scudo_utils.cpp:136
 
-} // namespace __scudo
+static u32 CRC32Table[] = {
+  0x00000000, 0x77073096, 0xee0e612c, 0x990951ba, 0x076dc419, 0x706af48f,
----------------
`const`


https://reviews.llvm.org/D26358