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

[Renato Golin via Phabricator via llvm-commits]

rengolin 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);
----------------
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. :)


https://reviews.llvm.org/D28574