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

[Kostya Kortchinsky via Phabricator via llvm-commits]

cryptoad added inline comments.


================
Comment at: lib/scudo/scudo_allocator.cpp:85
+#else
+  if (computeHardwareCRC32 && HashType == CRC32Hardware)
+    return computeHardwareCRC32(Crc, Data);
----------------
mgorny wrote:
> cryptoad wrote:
> > mgorny wrote:
> > > 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 ;-).
> > I had issues with `#ifdef` as only the .cpp is compiled with CRC32, leaving me without any symbol I could check for existence.
> > If you have a suggestion around that, I'll take it :)
> I meant making CMake explicitly define some value after determining that SSE4.2/CRC32 support is available. But that's a task for a separate patch, I guess.
Understood!


================
Comment at: lib/scudo/scudo_crc32.cpp:28
 # endif
 # ifdef __ARM_FEATURE_CRC32
 #  include <arm_acle.h>
----------------
mgorny wrote:
> One more thing: don't you want to enable `-mcrc` via CMake for this file?
I wasn't planning to with this, but I will update the patch to take this into account.


https://reviews.llvm.org/D28574