[PATCH] D28574: [scudo] Refactor of CRC32 and ARM runtime CRC32 detection

[Michał Górny via Phabricator via llvm-commits]

mgorny added inline comments.


================
Comment at: lib/scudo/scudo_allocator.cpp:82
+  // the hardware version of the CRC32.
+#if defined(__SSE4_2__)
+  return computeHardwareCRC32(Crc, Data);
----------------
rengolin wrote:
> cryptoad wrote:
> > mgorny wrote:
> > > If I'm not mistaken, you could add `|| defined(__ARM_FEATURE_CRC32)` to cover the same idea for ARM.
> > I think for ARM, since the feature could be enabled via -mcrc (specifically for CRC32, unlike the whole instruction set for SSE 4.2), the runtime check is still required, as no other potentially illegal instructions would be emitted anywhere else.
> Right, this is actually a good point. There are basically two ideas here:
> 
> 1. You compile to force one implementation or the other, via flags. In this case, if you compile to have CRC, it will fail with `sigill` on cores that don't have it. The most likely scenario is that all builds will end up not having it, for safety, so you'll never use it.
> 
> 2. You have a run time check, as it is now, so that it's not a compilation option and you'll always use whenever you can. The problem now is that you end up paying the comparison price on every call.
> 
> The third, and best (but more complicated) way is to use IFUNC, but that's certainly for another patch.
> 
> FWIW, right now, the AARch64 sanitizers are getting the VMA configuration via run time calls, just like this patch, because of the problems outlines above, so this is not without prior art. :)
However, the instruction can be enabled implicitly via `-march`. If you build purely for ARM target that supports CRC32, I don't see a point not to use it.


================
Comment at: lib/scudo/scudo_allocator.cpp:85
+#else
+  if (computeHardwareCRC32 && HashType == CRC32Hardware)
+    return computeHardwareCRC32(Crc, Data);
----------------
cryptoad wrote:
> mgorny wrote:
> > I'm sorry if I'm a bit ignorant here but why are you checking the function address? Is there a potential case for it being null?
> Given that it's a weak symbol, if not implemented anywhere the pointer will be null indeed (some details can be found at https://en.wikipedia.org/wiki/Weak_symbol) after linking. That way, if -msse42 is not available for scudo_crc32.cpp, computeHardwareCRC32 remains unimplemented, and we fallback to the software implementation.
Oh, that was the case I was missing. I don't mind using this then. However, personally I would do it the more obvious way and just `#ifdef` for the case when hardware CRC32 would not be implemented instead of checking at runtime ;-).


https://reviews.llvm.org/D28574