[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);
----------------
rengolin wrote:
> mgorny wrote:
> > 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.
> With ARM, the probability that you build in one machine (type) and run in another is higher than in x86. The number of variations and hardware support is also much much higher.
> 
> All x86_64 chips of family X have feature Y. A mix bag of ARM chips on family X, which support feature Y, has it in a way that is actually usable (ie. some cores report the feature but you really rather not use it).
> 
> All I'm saying is that, using -march=aarch64+crc will not work on any core that doesn't have it, so everyone will *not* use it in case the binary has to run on hardware that doesn't have it. Any Linux distribution (including Android) will not enable it for that reason.
> 
> If you're advocating for hard-coding if the feature IS there in mattr (either +crc or +nocrc), but leaving run-time if it's not, than I think that'll be ok, as it's a platform choice.
IIUC, the option suggested by Michal is the counterpart to SSE 4.2: test for -mcrc support in the compiler, add it to scudo_crc32.cpp only if detected and keep the runtime check in that case. In the event of -march with CRC support, scudo_allocator.cpp can bypass the runtime check and just do hardware.


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


https://reviews.llvm.org/D28574