Re: [PATCH v1 2/4] perf tools: Add sha1 utils
From: Ian Rogers
Date: Fri Jun 06 2025 - 14:28:05 EST
On Wed, Jun 4, 2025 at 11:17 AM Yuzhuo Jing <yuzhuo@xxxxxxxxxx> wrote:
>
> Hi Arnaldo,
>
> Thanks for testing the patches!
>
> > In file included from util/sha1_generic.c:18:
> > util/sha1_base.h: In function ‘sha1_base_do_finalize’:
> > util/sha1_base.h:77:21: error: comparison of integer expressions of different signedness: ‘unsigned int’ and ‘int’ [-Werror=sign-compare]
> > 77 | if (partial > bit_offset) {
> > | ^
> > cc1: all warnings being treated as errors
>
> Oh, I didn't see that on my GCC 14.2. A quick fix would work:
>
> --- /dev/fd/63 2025-06-04 09:54:42.344516115 -0700
> +++ tools/perf/util/sha1_base.h 2025-06-03 15:43:39.194036707 -0700
> @@ -71,7 +69,7 @@
> static inline int sha1_base_do_finalize(struct sha1_state *sctx,
> sha1_block_fn *block_fn)
> {
> - const int bit_offset = SHA1_BLOCK_SIZE - sizeof(__be64);
> + const unsigned int bit_offset = SHA1_BLOCK_SIZE - sizeof(__be64);
> __be64 *bits = (__be64 *)(sctx->buffer + bit_offset);
> unsigned int partial = sctx->count % SHA1_BLOCK_SIZE;
>
>
> To test it, I added -Werror=sign-compare to my local Makefile.config to
> force the error.
>
> diff --git a/tools/perf/Makefile.config b/tools/perf/Makefile.config
> index d19d1f132726..9909611be301 100644
> --- a/tools/perf/Makefile.config
> +++ b/tools/perf/Makefile.config
> @@ -225,9 +225,9 @@ endif
>
> # Treat warnings as errors unless directed not to
> ifneq ($(WERROR),0)
> - CORE_CFLAGS += -Werror
> - CXXFLAGS += -Werror
> - HOSTCFLAGS += -Werror
> + CORE_CFLAGS += -Werror=sign-compare -Werror
> + CXXFLAGS += -Werror=sign-compare -Werror
> + HOSTCFLAGS += -Werror=sign-compare -Werror
> endif
>
> ifndef DEBUG
>
>
> While testing with "make -C tools/perf -f tests/make make_debug", I saw
> similar compile errors in libjvmti.c:
>
> jvmti/libjvmti.c: In function ‘copy_class_filename’:
> jvmti/libjvmti.c:148:39: error: comparison of integer expressions of
> different signedness: ‘int’ and ‘long unsigned int’
> [-Werror=sign-compare]
> 148 | for (i = 0; i < (size_t)(p - class_sign); i++)
> | ^
> jvmti/libjvmti.c:155:31: error: comparison of integer expressions of
> different signedness: ‘int’ and ‘size_t’ {aka ‘long unsigned int’}
> [-Werror=sign-compare]
> 155 | for (j = 0; i < (max_length - 1) && file_name
> && j < strlen(file_name); j++, i++)
> | ^
> jvmti/libjvmti.c:155:68: error: comparison of integer expressions of
> different signedness: ‘int’ and ‘size_t’ {aka ‘long unsigned int’}
> [-Werror=sign-compare]
> 155 | for (j = 0; i < (max_length - 1) && file_name
> && j < strlen(file_name); j++, i++)
> | ^
>
> I've just sent a separate patch to the mailing list:
> https://lore.kernel.org/lkml/20250604173632.2362759-1-yuzhuo@xxxxxxxxxx/T/
Thanks Yuzhuo! I guess this happened because jvmti.h is missing in the
test container. It seems to make sense to add -Wsign-compare to the
standard CFLAGS to lower the chance of breaking this container again.
> > Humm that part is the same as in the kernel...
> >
> > ⬢ [acme@toolbx perf-tools-next]$ line=$(ctags -x --c-kinds=f include/crypto/sha1_base.h | awk '$1 == "sha1_base_do_finalize" {print $3}')
> > ⬢ [acme@toolbx perf-tools-next]$ sed -n $line,\$p include/crypto/sha1_base.h | awk '{print} /\{/ {c++} /\}/ {c--; if (c==0) exit}' > /tmp/original
> > ⬢ [acme@toolbx perf-tools-next]$ line=$(ctags -x --c-kinds=f tools/perf/util/sha1_base.h | awk '$1 == "sha1_base_do_finalize" {print $3}')
> > ⬢ [acme@toolbx perf-tools-next]$ sed -n $line,\$p include/crypto/sha1_base.h | awk '{print} /\{/ {c++} /\}/ {c--; if (c==0) exit}' > /tmp/copy
> > ⬢ [acme@toolbx perf-tools-next]$ diff -u /tmp/original /tmp/copy
> > --- /tmp/original 2025-05-22 14:48:31.338406860 -0300
> > +++ /tmp/copy 2025-05-22 14:48:58.401603439 -0300
> > @@ -1,3 +1,7 @@
> > +
> > + return 0;
> > +}
> > +
> > static inline int sha1_base_do_finalize(struct shash_desc *desc,
> > sha1_block_fn *block_fn)
> > {
> > @@ -13,10 +17,3 @@
> >
> > block_fn(sctx, sctx->buffer, 1);
> > }
> > -
> > - memset(sctx->buffer + partial, 0x0, bit_offset - partial);
> > - *bits = cpu_to_be64(sctx->count << 3);
> > - block_fn(sctx, sctx->buffer, 1);
> > -
> > - return 0;
> > -}
> > ⬢ [acme@toolbx perf-tools-next]$
>
> There were some other fixes that I made only to the perf tree version,
> while maintaining verbatim for other parts. Here's a script that
> retains and compares only the copied parts.
>
> # Ignore everything after sha1_transform
> diff -u -B -I "^#include " <(sed -n
> '/EXPORT_SYMBOL(sha1_transform)/q;p' lib/crypto/sha1.c)
> tools/perf/util/sha1.c
> diff -u -B -I "^#include " -I "sha1_zero_message_hash" -I "^struct
> sha1_state;$" -I "^void sha1_init(__u32 \*buf);$" \
> <(sed 's/shash_desc/sha1_state/g;' include/crypto/sha1.h)
> tools/perf/util/sha1.h
> diff -u -B -I "^EXPORT_SYMBOL" -I "^#include " \
> <(sed 's/shash_desc \*desc/sha1_state *sctx/g;
> /shash_desc_ctx(desc)/d' include/crypto/sha1_base.h)
> tools/perf/util/sha1_base.h
> # Ignore everything after crypto_sha1_finup
> diff -u -B -I "^EXPORT_SYMBOL" -I "^#include " \
> <(sed -n -e '/const u8
> sha1_zero_message_hash/,/EXPORT_SYMBOL_GPL(sha1_zero_message_hash)/d'
> \
> -e 's/shash_desc/sha1_state/g;
> /EXPORT_SYMBOL(crypto_sha1_finup)/q;p' crypto/sha1_generic.c) \
> tools/perf/util/sha1_generic.c
>
> And the changes are as below (including the quick fix above), with one
> changing the sign and integer type and another fixing type mismatch from
> const u8 * to const char *.
>
> Should we send another patch to the kernel tree version to fix the sign
> error, or we add rules to allow perf tree only changes?
I believe there will need to be a set of patches for the kernel sha1
code (fixing -Wsign-compare, any other typing issues) and a set of
patches for perf with the check-headers.sh updated as your scripts
check above. The perf patches shouldn't assume kernel patches have
landed. The perf check-headers.sh produces a warning but doesn't stop
the perf build. When the kernel changes land we can update the perf
check-headers.sh expectations.
Thanks,
Ian
> --- /dev/fd/63 2025-06-04 09:54:42.344516115 -0700
> +++ tools/perf/util/sha1_base.h 2025-06-03 15:43:39.194036707 -0700
> @@ -71,7 +69,7 @@
> static inline int sha1_base_do_finalize(struct sha1_state *sctx,
> sha1_block_fn *block_fn)
> {
> - const int bit_offset = SHA1_BLOCK_SIZE - sizeof(__be64);
> + const unsigned int bit_offset = SHA1_BLOCK_SIZE - sizeof(__be64);
> __be64 *bits = (__be64 *)(sctx->buffer + bit_offset);
> unsigned int partial = sctx->count % SHA1_BLOCK_SIZE;
>
> @@ -95,7 +93,7 @@
> __be32 *digest = (__be32 *)out;
> int i;
>
> - for (i = 0; i < SHA1_DIGEST_SIZE / sizeof(__be32); i++)
> + for (i = 0; i < SHA1_DIGEST_SIZE / (int)sizeof(__be32); i++)
> put_unaligned_be32(sctx->state[i], digest++);
>
> memzero_explicit(sctx, sizeof(*sctx));
> --- /dev/fd/63 2025-06-04 09:54:42.344516115 -0700
> +++ tools/perf/util/sha1_generic.c 2025-05-16 10:52:59.219531034 -0700
> @@ -27,7 +23,7 @@
> u32 temp[SHA1_WORKSPACE_WORDS];
>
> while (blocks--) {
> - sha1_transform(sst->state, src, temp);
> + sha1_transform(sst->state, (const char *)src, temp);
> src += SHA1_BLOCK_SIZE;
> }
> memzero_explicit(temp, sizeof(temp));
>
> Thanks!
>
> Best regards,
> Yuzhuo