[2/2] devtools: forbid the use of ffs compiler builtins
Checks
Commit Message
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>
---
devtools/checkpatches.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
Comments
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.
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?
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.
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.
On 2024-10-16 17:04, Stephen Hemminger 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.
>
Aren̈́'t GCC builtins pretty much standard? So any driver not targeting
Windows should be fine, although it would be better to use a DPDK wrapper.
> There are a lot of drivers still doing this.
I would suggest we fix this when someone has taken the time to
improve/modernize/extend <rte_bitops.h> further (e.g., with _Generic
versions of all bit fiddling and count functions and "all" __builtins
are covered).
I guess other APIs also may need to be extended (for non-bitops builtins).
@@ -179,7 +179,7 @@ check_forbidden_additions() { # <patch>
# forbid use of non abstracted bit count operations
awk -v FOLDERS="lib drivers app examples" \
- -v EXPRESSIONS='\\<__builtin_(clz|clzll|ctz|ctzll|popcount|popcountll)\\>' \
+ -v EXPRESSIONS='\\<__builtin_(clz|clzll|ctz|ctzll|popcount|popcountll|ffs|ffsll)\\>' \
-v RET_ON_FAIL=1 \
-v MESSAGE='Using __builtin helpers for bit count operations' \
-f $(dirname $(readlink -f $0))/check-forbidden-tokens.awk \