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

[Filipe Cabecinhas via llvm-commits]

filcab added a comment.

It looks like the patch is good, but I'll bail on giving the patch a go-ahead since I'm not too familiar with the allocator itself and some low-level problems you might be running into. I'll let Kostya or others decide on green lighting.



================
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))
----------------
cryptoad wrote:
> 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?
Yes. The way you changed it looks good.


================
Comment at: lib/scudo/scudo_allocator.cpp:461
   uptr getUsableSize(const void *Ptr, UnpackedHeader *Header) {
     if (UNLIKELY(!ThreadInited))
       initThread();
----------------
cryptoad wrote:
> 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.
Fair enough. I guess this can come from code which we don't control, so keeping this should be ok. Maybe a comment?


================
Comment at: lib/scudo/scudo_utils.cpp:89
     if (isSupportedCPU() == true)
-      getCPUID(&CPUInfo, 1, 0);
-    else
-      UNIMPLEMENTED();
+      getCPUID(&CPUInfo, 1);
     InfoInitialized = true;
----------------
cryptoad wrote:
> 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.
Indeed, all is good.


https://reviews.llvm.org/D26358