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

[Kostya Kortchinsky via Phabricator via llvm-commits]

cryptoad marked an inline comment as done.
cryptoad 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);
----------------
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.


================
Comment at: lib/scudo/scudo_allocator.cpp:85
+#else
+  if (computeHardwareCRC32 && HashType == CRC32Hardware)
+    return computeHardwareCRC32(Crc, Data);
----------------
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.


https://reviews.llvm.org/D28574