Re: [PATCH 2/2] devtools: forbid the use of ffs compiler builtins

[David Marchand <david.marchand@redhat.com>]

On Wed, Oct 16, 2024 at 5:39 PM Stephen Hemminger
<stephen@networkplumber.org> wrote:
>
> On Wed, 16 Oct 2024 17:32:27 +0200
> David Marchand <david.marchand@redhat.com> wrote:
>
> > On Wed, Oct 16, 2024 at 5:04 PM Stephen Hemminger
> > <stephen@networkplumber.org> wrote:
> > >
> > > On Wed, 16 Oct 2024 15:54:11 +0200
> > > Mattias Rönnblom <mattias.ronnblom@ericsson.com> wrote:
> > >
> > > > Extend checkpatches.sh to detect the use of __builtin_(ffs|ffsll).
> > > >
> > > > These intrinsics are not available in MSVC, and there are perfectly
> > > > serviceable alternatives in <rte_bitops.h>.
> > > >
> > > > Signed-off-by: Mattias Rönnblom <mattias.ronnblom@ericsson.com>
> > > > Suggested-by: David Marchand <david.marchand@redhat.com>
> > >
> > > Shouldn't this apply to all _builtin_ functions.
> > >
> > > There are a lot of drivers still doing this.
> >
> > - The updated check here is about use of __builtin_XXX instead of DPDK
> > bit count ops.
> > So when you refer to all builtin functions, there may be other
> > builtins related to bit count we are missing, but at least the current
> > patch is better than before.
> >
> >
> > - On the larger topic of refusing *any* __builtin_, some use of them
> > are "legit" under compiler checks.
> > So the check would have to skip headers where wrappers are provided to
> > the rest of DPDK.
> > This seems possible, but a larger and probably more tricky change for now.
> >
> > Are you ok if we go with this patch as is, in a first step?
> >
> >
>
> Examples of direct use of builtin
>         app/dumpcap/main.c:     log2 = sizeof(size) * 8 - __builtin_clzl(size - 1);
>         app/test-dma-perf/benchmark.c:                  __builtin_ia32_clflush(data + offset);
>         drivers/bus/fslmc/qbman/include/compat.h:#define likely(x)      __builtin_expect(!!(x), 1)
>         drivers/bus/fslmc/qbman/include/compat.h:#define unlikely(x)    __builtin_expect(!!(x), 0)
>
>         drivers/common/nfp/nfp_platform.h:#define __bf_shf(x) (__builtin_ffsll(x) - 1)
>         drivers/crypto/ccp/ccp_dev.c:   first_zero = __builtin_ffsl(~word);
>         app/test/test_memcpy_perf.c:    if (__builtin_constant_p(n))
>
>         drivers/net/bonding/rte_eth_bond_pmd.c:         if (__builtin_popcountl(link_speeds) != 2) {
>
>
> Think that direct use of __builitin_expect should be flagged.
> And the lots of __builtin_in mlx5 should also be flagged.

Yes, we should fix those, but that's beyond the bitops check.
I'll go ahead with current series for now.


-- 
David Marchand