[compiler-rt] [scudo] Use -mcrc32 on x86 when available

[mgorny]

Summary

Update the hardware CRC32 logic in scudo to support using -mcrc32
instead of -msse4.2. The CRC32 intrinsics use the former flag
in the newer compiler versions, e.g. in clang since 12fa608af44a.
With these versions of clang, passing -msse4.2 is insufficient
to enable the instructions and causes build failures when -march does
not enable CRC32 implicitly:

/var/tmp/portage/sys-libs/compiler-rt-sanitizers-14.0.0/work/compiler-rt/lib/scudo/scudo_crc32.cpp:20:10: error: always_inline function '_mm_crc32_u32' requires target feature 'crc32', but would be inlined into function 'computeHardwareCRC32' that is compiled without support for 'crc32'
  return CRC32_INTRINSIC(Crc, Data);
         ^
/var/tmp/portage/sys-libs/compiler-rt-sanitizers-14.0.0/work/compiler-rt/lib/scudo/scudo_crc32.h:27:27: note: expanded from macro 'CRC32_INTRINSIC'
#  define CRC32_INTRINSIC FIRST_32_SECOND_64(_mm_crc32_u32, _mm_crc32_u64)
                          ^
/var/tmp/portage/sys-libs/compiler-rt-sanitizers-14.0.0/work/compiler-rt/lib/scudo/../sanitizer_common/sanitizer_platform.h:132:36: note: expanded from macro 'FIRST_32_SECOND_64'
#  define FIRST_32_SECOND_64(a, b) (a)
                                   ^
1 error generated.

For backwards compatibility, use -mcrc32 when available and fall back
to -msse4.2. The <smmintrin.h> header remains in use as it still
works and is compatible with GCC, while clang's <crc32intrin.h>
is not.

Use builtin_ia32*() rather than _mm_crc32*() when using -mcrc32
to preserve compatibility with GCC. _mm_crc32*() are aliases
to builtin_ia32*() in both compilers but GCC requires -msse4.2
for the former, while both use -mcrc32 for the latter.

Originally reported in https://bugs.gentoo.org/835870.

[pengfei]

LGTM, thanks!

[mgorny]

Used __builtin_ia32_crc32* along with -mcrc32 to fix compatibility with GCC.

[nikic]

Not familiar with this code, but the newest version does resolve the build failure with GCC, so I guess this is fine.

[MaskRay]

Is this a problem with D105462? Should -msse4.2 imply -mcrc32?

[pengfei]

In D122789#3423846, @MaskRay wrote:

Is this a problem with D105462? Should -msse4.2 imply -mcrc32?

-msse4.2 implies -mcrc32: https://godbolt.org/z/xaPccrKx3

[pengfei]

So it's interesting, it shouldn't fail that way https://godbolt.org/z/jcqx5x9j7

[MaskRay]

In D122789#3424213, @pengfei wrote:

In D122789#3423846, @MaskRay wrote:

Is this a problem with D105462? Should -msse4.2 imply -mcrc32?

-msse4.2 implies -mcrc32: https://godbolt.org/z/xaPccrKx3

That is my understanding of clang/lib/Basic/Targets/X86.cpp:159 , but the error suggested that D105462 might change something in an interesting way.
I am on a trip so do not spend time investigating the root cause.

[pengfei]

In D122789#3424226, @MaskRay wrote:

In D122789#3424213, @pengfei wrote:

In D122789#3423846, @MaskRay wrote:

Is this a problem with D105462? Should -msse4.2 imply -mcrc32?

-msse4.2 implies -mcrc32: https://godbolt.org/z/xaPccrKx3

That is my understanding of clang/lib/Basic/Targets/X86.cpp:159 , but the error suggested that D105462 might change something in an interesting way.
I am on a trip so do not spend time investigating the root cause.

I a bit suspect the -msse4.2 doesn't specified (correctly). The message in https://bugs.gentoo.org/835870 is a bit confusing. The options in first comment contain -msse4.2, but the following ones say "-msse4.2 [disabled]" or "-mno-sse4 [enabled]". If sse4.2 is disable, no doubt we should add -mcrc32 instead.

[MaskRay]

To kurly (original Gentoo reporter):

printf '#include <smmintrin.h>\n#include <stdint.h>\nuint32_t computeHardwareCRC32(uint32_t Crc, uint32_t Data) { return _mm_crc32_u32(Crc, Data); }' > a.c

% clang -m32 -march=i686 -msse4.2 -c a.c  # seems to compile fine
% gcc -m32 -march=i686 -msse4.2 -c a.c
% gcc -m32 -march=i686 -mcrc32 -c a.c
In file included from a.c:1:
a.c: In function ‘computeHardwareCRC32’:
/usr/lib/gcc/x86_64-linux-gnu/11/include/smmintrin.h:839:1: error: inlining failed in call to ‘always_inline’ ‘_mm_crc32_u32’: target specific option mismatch
  839 | _mm_crc32_u32 (unsigned int __C, unsigned int __V)
      | ^~~~~~~~~~~~~
a.c:3:69: note: called from here
    3 | uint32_t computeHardwareCRC32(uint32_t Crc, uint32_t Data) { return _mm_crc32_u32(Crc, Data); }
      |                                                                     ^~~~~~~~~~~~~~~~~~~~~~~~

I have some older GCC and latest GCC (2022-04, multilib), gcc -m32 -march=i686 -msse4.2 -c a.c builds while -mcrc32 doesn't.

I suspect we should revert the -mcrc32 change. The __CRC32__ macro may be fine.

[pengfei]

In D122789#3446865, @MaskRay wrote:

To kurly (original Gentoo reporter):

printf '#include <smmintrin.h>\n#include <stdint.h>\nuint32_t computeHardwareCRC32(uint32_t Crc, uint32_t Data) { return _mm_crc32_u32(Crc, Data); }' > a.c

% clang -m32 -march=i686 -msse4.2 -c a.c  # seems to compile fine
% gcc -m32 -march=i686 -msse4.2 -c a.c
% gcc -m32 -march=i686 -mcrc32 -c a.c
In file included from a.c:1:
a.c: In function ‘computeHardwareCRC32’:
/usr/lib/gcc/x86_64-linux-gnu/11/include/smmintrin.h:839:1: error: inlining failed in call to ‘always_inline’ ‘_mm_crc32_u32’: target specific option mismatch
  839 | _mm_crc32_u32 (unsigned int __C, unsigned int __V)
      | ^~~~~~~~~~~~~
a.c:3:69: note: called from here
    3 | uint32_t computeHardwareCRC32(uint32_t Crc, uint32_t Data) { return _mm_crc32_u32(Crc, Data); }
      |                                                                     ^~~~~~~~~~~~~~~~~~~~~~~~

I have some older GCC and latest GCC (2022-04, multilib), gcc -m32 -march=i686 -msse4.2 -c a.c builds while -mcrc32 doesn't.

I suspect we should revert the -mcrc32 change. The __CRC32__ macro may be fine.

Thanks for the infromation. I got the same result. That's weird. I believe at some point, GCC supported -mcrc32. @hjl.tools, did GCC intentionally remove the support for -mcrc32?

[pengfei]

GCC supports "-mcrc32", but seems only for built-in functions: https://godbolt.org/z/veeGMoY11

https://gcc.gnu.org/onlinedocs/gcc/x86-Options.html#x86-Options

[hjl.tools]

In D122789#3447413, @pengfei wrote:

In D122789#3446865, @MaskRay wrote:

To kurly (original Gentoo reporter):

printf '#include <smmintrin.h>\n#include <stdint.h>\nuint32_t computeHardwareCRC32(uint32_t Crc, uint32_t Data) { return _mm_crc32_u32(Crc, Data); }' > a.c

% clang -m32 -march=i686 -msse4.2 -c a.c  # seems to compile fine
% gcc -m32 -march=i686 -msse4.2 -c a.c
% gcc -m32 -march=i686 -mcrc32 -c a.c
In file included from a.c:1:
a.c: In function ‘computeHardwareCRC32’:
/usr/lib/gcc/x86_64-linux-gnu/11/include/smmintrin.h:839:1: error: inlining failed in call to ‘always_inline’ ‘_mm_crc32_u32’: target specific option mismatch
  839 | _mm_crc32_u32 (unsigned int __C, unsigned int __V)
      | ^~~~~~~~~~~~~
a.c:3:69: note: called from here
    3 | uint32_t computeHardwareCRC32(uint32_t Crc, uint32_t Data) { return _mm_crc32_u32(Crc, Data); }
      |                                                                     ^~~~~~~~~~~~~~~~~~~~~~~~

I have some older GCC and latest GCC (2022-04, multilib), gcc -m32 -march=i686 -msse4.2 -c a.c builds while -mcrc32 doesn't.

I suspect we should revert the -mcrc32 change. The __CRC32__ macro may be fine.

Thanks for the infromation. I got the same result. That's weird. I believe at some point, GCC supported -mcrc32. @hjl.tools, did GCC intentionally remove the support for -mcrc32?

"gcc -m32 -march=i686 -mcrc32" works with GCC 11.2: https://godbolt.org/z/nWzoofeKs

compiler-rt/cmake/config-ix.cmake

 check_cxx_compiler_flag("-Werror -fno-function-sections" COMPILER_RT_HAS_FNO_FUNCTION_SECTIONS_FLAG)
 check_cxx_compiler_flag(-std=c++14           COMPILER_RT_HAS_STD_CXX14_FLAG)
 check_cxx_compiler_flag(-ftls-model=initial-exec COMPILER_RT_HAS_FTLS_MODEL_INITIAL_EXEC)
 check_cxx_compiler_flag(-fno-lto             COMPILER_RT_HAS_FNO_LTO_FLAG)
 check_cxx_compiler_flag(-fno-profile-generate COMPILER_RT_HAS_FNO_PROFILE_GENERATE_FLAG)
 check_cxx_compiler_flag(-fno-profile-instr-generate COMPILER_RT_HAS_FNO_PROFILE_INSTR_GENERATE_FLAG)
 check_cxx_compiler_flag(-fno-profile-instr-use COMPILER_RT_HAS_FNO_PROFILE_INSTR_USE_FLAG)
 check_cxx_compiler_flag(-fno-coverage-mapping COMPILER_RT_HAS_FNO_COVERAGE_MAPPING_FLAG)
+check_cxx_compiler_flag("-Werror -mcrc32"    COMPILER_RT_HAS_MCRC32_FLAG)
 check_cxx_compiler_flag("-Werror -msse3" COMPILER_RT_HAS_MSSE3_FLAG)
 check_cxx_compiler_flag("-Werror -msse4.2"   COMPILER_RT_HAS_MSSE4_2_FLAG)
 check_cxx_compiler_flag(--sysroot=.          COMPILER_RT_HAS_SYSROOT_FLAG)
 check_cxx_compiler_flag("-Werror -mcrc"      COMPILER_RT_HAS_MCRC_FLAG)
 check_cxx_compiler_flag(-fno-partial-inlining COMPILER_RT_HAS_FNO_PARTIAL_INLINING_FLAG)
 
 if(NOT WIN32 AND NOT CYGWIN)
   # MinGW warns if -fvisibility-inlines-hidden is used.

compiler-rt/lib/scudo/CMakeLists.txt

set(SCUDO_HEADERS
   scudo_flags.inc
   scudo_interface_internal.h
   scudo_platform.h
   scudo_tsd.h
   scudo_tsd_exclusive.inc
   scudo_tsd_shared.inc
   scudo_utils.h)
 
-# Enable the SSE 4.2 instruction set for scudo_crc32.cpp, if available.
-if (COMPILER_RT_HAS_MSSE4_2_FLAG)
+# Enable the necessary instruction set for scudo_crc32.cpp, if available.
+# Newer compiler versions use -mcrc32 rather than -msse4.2.
+if (COMPILER_RT_HAS_MCRC32_FLAG)
+  set_source_files_properties(scudo_crc32.cpp PROPERTIES COMPILE_FLAGS -mcrc32)
+elseif (COMPILER_RT_HAS_MSSE4_2_FLAG)
   set_source_files_properties(scudo_crc32.cpp PROPERTIES COMPILE_FLAGS -msse4.2)
 endif()
 
 # Enable the AArch64 CRC32 feature for scudo_crc32.cpp, if available.
 # Note that it is enabled by default starting with armv8.1-a.
 if (COMPILER_RT_HAS_MCRC_FLAG)
   set_source_files_properties(scudo_crc32.cpp PROPERTIES COMPILE_FLAGS -mcrc)
 endif()

compiler-rt/lib/scudo/scudo_allocator.cpp

 // at compilation or at runtime.
 static atomic_uint8_t HashAlgorithm = { CRC32Software };
 
 inline u32 computeCRC32(u32 Crc, uptr Value, uptr *Array, uptr ArraySize) {
   // If the hardware CRC32 feature is defined here, it was enabled everywhere,
   // as opposed to only for scudo_crc32.cpp. This means that other hardware
   // specific instructions were likely emitted at other places, and as a
   // result there is no reason to not use it here.
-#if defined(__SSE4_2__) || defined(__ARM_FEATURE_CRC32)
+#if defined(__CRC32__) || defined(__SSE4_2__) || defined(__ARM_FEATURE_CRC32)
   Crc = CRC32_INTRINSIC(Crc, Value);
   for (uptr i = 0; i < ArraySize; i++)
     Crc = CRC32_INTRINSIC(Crc, Array[i]);
   return Crc;
 #else
   if (atomic_load_relaxed(&HashAlgorithm) == CRC32Hardware) {
     Crc = computeHardwareCRC32(Crc, Value);
     for (uptr i = 0; i < ArraySize; i++)
       Crc = computeHardwareCRC32(Crc, Array[i]);
     return Crc;
   }
   Crc = computeSoftwareCRC32(Crc, Value);
   for (uptr i = 0; i < ArraySize; i++)
     Crc = computeSoftwareCRC32(Crc, Array[i]);
   return Crc;
-#endif  // defined(__SSE4_2__) || defined(__ARM_FEATURE_CRC32)
+#endif  // defined(__CRC32__) || defined(__SSE4_2__) || defined(__ARM_FEATURE_CRC32)
 }
 
 static BackendT &getBackend();
 
 namespace Chunk {
   static inline AtomicPackedHeader *getAtomicHeader(void *Ptr) {
     return reinterpret_cast<AtomicPackedHeader *>(reinterpret_cast<uptr>(Ptr) -
         getHeaderSize());

compiler-rt/lib/scudo/scudo_crc32.h

 //===----------------------------------------------------------------------===//
 
 #ifndef SCUDO_CRC32_H_
 #define SCUDO_CRC32_H_
 
 #include "sanitizer_common/sanitizer_internal_defs.h"
 
 // Hardware CRC32 is supported at compilation via the following:
-// - for i386 & x86_64: -msse4.2
+// - for i386 & x86_64: -mcrc32 (earlier: -msse4.2)
 // - for ARM & AArch64: -march=armv8-a+crc or -mcrc
 // An additional check must be performed at runtime as well to make sure the
 // emitted instructions are valid on the target host.
 
-#if defined(__SSE4_2__) || defined(__ARM_FEATURE_CRC32)
-# ifdef __SSE4_2__
+#if defined(__CRC32__) || defined(__SSE4_2__) || defined(__ARM_FEATURE_CRC32)
+# if defined(__CRC32__) || defined(__SSE4_2__)
+// NB: clang has <crc32intrin.h> but GCC does not
 #  include <smmintrin.h>
 #  define CRC32_INTRINSIC FIRST_32_SECOND_64(_mm_crc32_u32, _mm_crc32_u64)
 # endif
 # ifdef __ARM_FEATURE_CRC32
 #  include <arm_acle.h>
 #  define CRC32_INTRINSIC FIRST_32_SECOND_64(__crc32cw, __crc32cd)
 # endif
-#endif  // defined(__SSE4_2__) || defined(__ARM_FEATURE_CRC32)
+#endif  // defined(__CRC32__) || defined(__SSE4_2__) || defined(__ARM_FEATURE_CRC32)
 
 namespace __scudo {
 
 enum : u8 {
   CRC32Software = 0,
   CRC32Hardware = 1,
 };
 

compiler-rt/lib/scudo/scudo_crc32.cpp

 /// kept separated to restrict the use of compiler specific flags to this file.
 ///
 //===----------------------------------------------------------------------===//
 
 #include "scudo_crc32.h"
 
 namespace __scudo {
 
-#if defined(__SSE4_2__) || defined(__ARM_FEATURE_CRC32)
+#if defined(__CRC32__) || defined(__SSE4_2__) || defined(__ARM_FEATURE_CRC32)
 u32 computeHardwareCRC32(u32 Crc, uptr Data) {
   return CRC32_INTRINSIC(Crc, Data);
 }
-#endif  // defined(__SSE4_2__) || defined(__ARM_FEATURE_CRC32)
+#endif  // defined(__CRC32__) || defined(__SSE4_2__) || defined(__ARM_FEATURE_CRC32)
 
 }  // namespace __scudo

compiler-rt/lib/scudo/standalone/CMakeLists.txt

set(SCUDO_SOURCES
   flags.cpp
   fuchsia.cpp
   linux.cpp
   release.cpp
   report.cpp
   string_utils.cpp
   )
 
-# Enable the SSE 4.2 instruction set for crc32_hw.cpp, if available.
-if (COMPILER_RT_HAS_MSSE4_2_FLAG)
+# Enable the necessary instruction set for scudo_crc32.cpp, if available.
+# Newer compiler versions use -mcrc32 rather than -msse4.2.
+if (COMPILER_RT_HAS_MCRC32_FLAG)
+  set_source_files_properties(crc32_hw.cpp PROPERTIES COMPILE_FLAGS -mcrc32)
+elseif (COMPILER_RT_HAS_MSSE4_2_FLAG)
   set_source_files_properties(crc32_hw.cpp PROPERTIES COMPILE_FLAGS -msse4.2)
 endif()
 
 # Enable the AArch64 CRC32 feature for crc32_hw.cpp, if available.
 # Note that it is enabled by default starting with armv8.1-a.
 if (COMPILER_RT_HAS_MCRC_FLAG)
   set_source_files_properties(crc32_hw.cpp PROPERTIES COMPILE_FLAGS -mcrc)
 endif()

compiler-rt/lib/scudo/standalone/checksum.h

 //===-- checksum.h ----------------------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef SCUDO_CHECKSUM_H_
 #define SCUDO_CHECKSUM_H_
 
 #include "internal_defs.h"
 
 // Hardware CRC32 is supported at compilation via the following:
-// - for i386 & x86_64: -msse4.2
+// - for i386 & x86_64: -mcrc32 (earlier: -msse4.2)
 // - for ARM & AArch64: -march=armv8-a+crc or -mcrc
 // An additional check must be performed at runtime as well to make sure the
 // emitted instructions are valid on the target host.
 
-#ifdef __SSE4_2__
+#if defined(__CRC32__) || defined(__SSE4_2__)
+// NB: clang has <crc32intrin.h> but GCC does not
 #include <smmintrin.h>
 #define CRC32_INTRINSIC FIRST_32_SECOND_64(_mm_crc32_u32, _mm_crc32_u64)
 #endif
 #ifdef __ARM_FEATURE_CRC32
 #include <arm_acle.h>
 #define CRC32_INTRINSIC FIRST_32_SECOND_64(__crc32cw, __crc32cd)
 #endif
 

compiler-rt/lib/scudo/standalone/chunk.h

 
 extern Checksum HashAlgorithm;
 
 inline u16 computeChecksum(u32 Seed, uptr Value, uptr *Array, uptr ArraySize) {
   // If the hardware CRC32 feature is defined here, it was enabled everywhere,
   // as opposed to only for crc32_hw.cpp. This means that other hardware
   // specific instructions were likely emitted at other places, and as a result
   // there is no reason to not use it here.
-#if defined(__SSE4_2__) || defined(__ARM_FEATURE_CRC32)
+#if defined(__CRC32__) || defined(__SSE4_2__) || defined(__ARM_FEATURE_CRC32)
   u32 Crc = static_cast<u32>(CRC32_INTRINSIC(Seed, Value));
   for (uptr I = 0; I < ArraySize; I++)
     Crc = static_cast<u32>(CRC32_INTRINSIC(Crc, Array[I]));
   return static_cast<u16>(Crc ^ (Crc >> 16));
 #else
   if (HashAlgorithm == Checksum::HardwareCRC32) {
     u32 Crc = computeHardwareCRC32(Seed, Value);
     for (uptr I = 0; I < ArraySize; I++)
       Crc = computeHardwareCRC32(Crc, Array[I]);
     return static_cast<u16>(Crc ^ (Crc >> 16));
   } else {
     u16 Checksum = computeBSDChecksum(static_cast<u16>(Seed), Value);
     for (uptr I = 0; I < ArraySize; I++)
       Checksum = computeBSDChecksum(Checksum, Array[I]);
     return Checksum;
   }
-#endif // defined(__SSE4_2__) || defined(__ARM_FEATURE_CRC32)
+#endif // defined(__CRC32__) || defined(__SSE4_2__) || defined(__ARM_FEATURE_CRC32)
 }
 
 namespace Chunk {
 
 // Note that in an ideal world, `State` and `Origin` should be `enum class`, and
 // the associated `UnpackedHeader` fields of their respective enum class type
 // but https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61414 prevents it from
 // happening, as it will error, complaining the number of bits is not enough.

compiler-rt/lib/scudo/standalone/crc32_hw.cpp

 //===-- crc32_hw.cpp --------------------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "checksum.h"
 
 namespace scudo {
 
-#if defined(__SSE4_2__) || defined(__ARM_FEATURE_CRC32)
+#if defined(__CRC32__) || defined(__SSE4_2__) || defined(__ARM_FEATURE_CRC32)
 u32 computeHardwareCRC32(u32 Crc, uptr Data) {
   return static_cast<u32>(CRC32_INTRINSIC(Crc, Data));
 }
-#endif // defined(__SSE4_2__) || defined(__ARM_FEATURE_CRC32)
+#endif // defined(__CRC32__) || defined(__SSE4_2__) || defined(__ARM_FEATURE_CRC32)
 
 } // namespace scudo