Re: [PATCH v1 2/4] perf tools: Add sha1 utils

[Arnaldo Carvalho de Melo <acme@kernel.org>]

On Fri, Jun 06, 2025 at 11:27:28AM -0700, Ian Rogers wrote:
> On Wed, Jun 4, 2025 at 11:17 AM Yuzhuo Jing <yuzhuo@google.com> wrote:
> > Thanks for testing the patches!

You're welcome!

> > > 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@google.com/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.

Right, we try to reuse bits from the kernel as there are more people
working there and its a more critical piece of software than tooling
like perf, so when we notice something in the copies we carry, we better
fix it first in the origin.

> 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.

Right,

Thanks,

- Arnaldo
 
> 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