Re: [PATCH] cksum: Use pclmul hardware instruction for CRC32 calculation

[Pádraig Brady]

On 12/03/2021 15:33, Kristoffer Brånemyr wrote:
> Hi,
>
> I was just wondering if you are planning to merge the change, or if you decided 
> against it? :)
> I wanted to use the cpuid.h autoconf detection for another patch I'm working on.

We're still planning to include it.
I'm making a few adjustments.

The initial one is:

diff --git a/src/local.mk b/src/local.mk
index f54a96c9c..d4a00a1a4 100644
--- a/src/local.mk
+++ b/src/local.mk
@@ -358,10 +358,11 @@ src___SOURCES = src/lbracket.c
 nodist_src_coreutils_SOURCES = src/coreutils.h
 src_coreutils_SOURCES = src/coreutils.c

-src/cksum_pclmul.o: CFLAGS += -mavx -mpclmul
 src_cksum_SOURCES = src/cksum.c src/cksum.h
+src_cksum_CFLAGS = $(AM_CFLAGS)
 if USE_PCLMUL_CRC32
 src_cksum_SOURCES += src/cksum_pclmul.c
+src_cksum_CFLAGS += -mavx -mpclmul
 endif
 src_cp_SOURCES = src/cp.c $(copy_sources) $(selinux_sources)
 src_dir_SOURCES = src/ls.c src/ls-dir.c

The above changes from relying on make(1) to append to the variable,
to instead leveraging automake's append support for better portability.
The original used a "target-specific variable", which seems to be GNU specific:
https://www.gnu.org/software/make/manual/html_node/Target_002dspecific.html

To confirm I tried a trivial Makefile on Solaris and FreeBSD,
which gave the following errors respectively:
  "Macro assignment on dependency line"
  "don't know how to make CFLAGS"

Another issue with the original is that it modifies CFLAGS,
which is a user variable, and is advised against.
If the user overrides CFLAGS for a particular make invocation
(which is a common thing I do myself for debugging etc.),
then that would result in a compile failure due to
the necessary flags not being defined. More details at:
https://www.gnu.org/software/automake/manual/html_node/Flag-Variables-Ordering.html

A caveat of the new approach is that the main cksum module
also has these flags applied, but that should be fine.
For completeness if you really wanted to restrict the flags
to a particular module, you could define a noinst_LIBRARY as described at:
https://www.gnu.org/software/automake/manual/html_node/Per_002dObject-Flags.html

The other changes are mainly scope changes etc.
as identified with `make syntax-check`.

cheers,
Pádraig