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

[Kostya Kortchinsky via llvm-commits]

cryptoad marked an inline comment as done.
cryptoad added a comment.

Thanks Filipe, I appreciate you having a look at this. I fixed some of raised issues in my working copy, I have some comments/questions in return for you for clarification before I change more things.



================
Comment at: lib/scudo/scudo_allocator.cpp:39
   static const uptr kMetadataSize = 0;
-  typedef DefaultSizeClassMap SizeClassMap;
+  typedef __scudo::SizeClassMap SizeClassMap;
   typedef NoOpMapUnmapCallback MapUnmapCallback;
----------------
filcab wrote:
> Maybe just use `__scudo::DefaultSizeClassMap` and get rid of the above typedef?
SizeClassMap::kMaxSize is used below to compute the maximum offset for a chunk.
In order to be able to use the same code construct for 32 & 64-bit, I used the first typedef, which led to the second one in the AP structure.


================
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))
----------------
filcab wrote:
> 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.
To clarify this, would it boil down to moving the loadHeader logic outside of the sizing function?


================
Comment at: lib/scudo/scudo_allocator.cpp:461
   uptr getUsableSize(const void *Ptr, UnpackedHeader *Header) {
     if (UNLIKELY(!ThreadInited))
       initThread();
----------------
filcab wrote:
> Do we need this if? What goes wrong if we emit it?
The reasoning behind this was that malloc_usable_size could potentially be the first allocator related function called in a program which would lead to bad things if the initialization wasn't done. I agree that this should probably not happen, so it becomes a matter of CHECK vs doing it.


================
Comment at: lib/scudo/scudo_utils.cpp:89
     if (isSupportedCPU() == true)
-      getCPUID(&CPUInfo, 1, 0);
-    else
-      UNIMPLEMENTED();
+      getCPUID(&CPUInfo, 1);
     InfoInitialized = true;
----------------
filcab wrote:
> 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).
This is only called from the global initialization routine call via pthread_once, hence me not making it thread safe.
I am happy to change it if you feel it could become a problem, let me know.


https://reviews.llvm.org/D26358