Re: [PATCH v8 01/29] devtools: check packed attributes

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

Hello Andre,

On Tue, Dec 31, 2024 at 7:39 PM Andre Muezerie
<andremue@linux.microsoft.com> wrote:
>
> Ensure __rte_packed_begin and __rte_packed_end show up in pairs
> when checking patches.
>
> Signed-off-by: Andre Muezerie <andremue@linux.microsoft.com>
> Acked-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> ---
>  devtools/checkpatches.sh | 43 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 43 insertions(+)
>
> diff --git a/devtools/checkpatches.sh b/devtools/checkpatches.sh
> index 4a8591be22..7868f5e522 100755
> --- a/devtools/checkpatches.sh
> +++ b/devtools/checkpatches.sh
> @@ -202,6 +202,14 @@ check_forbidden_additions() { # <patch>
>                 -f $(dirname $(readlink -f $0))/check-forbidden-tokens.awk \
>                 "$1" || res=1
>
> +       # forbid use of __rte_packed_begin with enums
> +       awk -v FOLDERS='lib drivers app examples' \
> +               -v EXPRESSIONS='enum.*__rte_packed_begin' \
> +               -v RET_ON_FAIL=1 \
> +               -v MESSAGE='Using __rte_packed_begin with enum is not allowed' \
> +               -f $(dirname $(readlink -f $0))/check-forbidden-tokens.awk \
> +               "$1" || res=1
> +
>         # forbid use of experimental build flag except in examples
>         awk -v FOLDERS='lib drivers app' \
>                 -v EXPRESSIONS='-DALLOW_EXPERIMENTAL_API allow_experimental_apis' \
> @@ -362,6 +370,33 @@ check_aligned_attributes() { # <patch>
>         return $res
>  }
>
> +check_packed_attributes() { # <patch>
> +       res=0
> +
> +       if [ $(grep -E '^\+.*__rte_packed_begin' "$1" | \
> +                       grep -vE '\<struct[[:space:]]*__rte_packed_begin\>' | \
> +                       grep -vE '\<union[[:space:]]*__rte_packed_begin\>' | \
> +                       grep -vE '\<__rte_cache_aligned[[:space:]]*__rte_packed_begin\>' | \
> +                       grep -vE '\<__rte_cache_min_aligned[[:space:]]*__rte_packed_begin\>' | \
> +                       grep -vE '\<__rte_aligned\(.*\)[[:space:]]*__rte_packed_begin\>' | \
> +                       wc -l) != 0 ]; then
> +               echo "Please use __rte_packed_begin only after struct, union," \
> +                        " __rte_cache_aligned, __rte_cache_min_aligned or __rte_aligned."
> +               res=1
> +       fi

This part lgtm.


> +
> +       begin_count=$(grep '__rte_packed_begin' "$1" | \
> +                       wc -l)
> +       end_count=$(grep '__rte_packed_end' "$1" | \
> +                       wc -l)
> +       if [ $begin_count != $end_count ]; then
> +               echo "__rte_packed_begin and __rte_packed_end mismatch. They should always be used in pairs."
> +               res=1
> +       fi

This part is problematic.
The check is applied on a patch: a __rte_packed_* token (let's imagine
__rte_packed_begin) may be involved without an associated change on
its counterpart token (__rte_packed_end).

Let me show an example:

$ git show
commit 655cfe433ab6f4fb8c92fec44ea5e1f689055201 (HEAD -> pack_v8_dma)
Author: David Marchand <david.marchand@redhat.com>
Date:   Tue Jan 7 15:13:26 2025 +0100

    plop

    plop

    Signed-off-by: David Marchand <david.marchand@redhat.com>

diff --git a/lib/net/rte_ip6.h b/lib/net/rte_ip6.h
index 92558a124a..686c12220b 100644
--- a/lib/net/rte_ip6.h
+++ b/lib/net/rte_ip6.h
@@ -461,7 +461,7 @@ rte_ether_mcast_from_ipv6(struct rte_ether_addr
*mac, const struct rte_ipv6_addr
 /**
  * IPv6 Header
  */
-struct __rte_aligned(2) __rte_packed_begin rte_ipv6_hdr {
+struct __rte_aligned(2) __rte_packed_begin rte_ipv6_hdr /* A useful
comment, isn't it? */ {
        union {
                rte_be32_t vtc_flow;        /**< IP version, traffic
class & flow label. */
                __extension__

$ ./devtools/checkpatches.sh -n1

### [PATCH] plop

__rte_packed_begin and __rte_packed_end should always be used in pairs.

0/1 valid patch


I don't think there is a way to accurately catch wrongly paired tokens.
The only good way is having most extensive Windows builds in the CI, isn't it?


-- 
David Marchand