RE: [PATCH v7 02/29] eal/include: add new packing macros

["Morten Brørup" <mb@smartsharesystems.com>]

> From: Andre Muezerie [mailto:andremue@linux.microsoft.com]
> Sent: Monday, 23 December 2024 20.12
> 
> MSVC struct packing is not compatible with GCC. Add macro
> __rte_packed_begin which can be used to push existing pack value
> and set packing to 1-byte. Add macro __rte_packed_end to restore
> the pack value prior to the push.

Nice to have:
Some alternative solutions were discussed on the mailing list.
Please consider adding a brief summary of them with their benefits/disadvantages to the patch description, either here or in the cover letter (0/NN) of the patch series.

> 
> Macro __rte_packed_end is deliberately utilized to trigger a
> MSVC compiler warning if no existing packing has been pushed allowing
> easy identification of locations where the __rte_packed_begin is
> missing.
> 
> Macro __rte_packed was marked as deprecated.
> 
> Signed-off-by: Andre Muezerie <andremue@linux.microsoft.com>
> Acked-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> ---
>  lib/eal/include/rte_common.h | 19 +++++++++++++++++--
>  1 file changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/eal/include/rte_common.h
> b/lib/eal/include/rte_common.h
> index 4d299f2b36..e40910fa20 100644
> --- a/lib/eal/include/rte_common.h
> +++ b/lib/eal/include/rte_common.h
> @@ -99,13 +99,28 @@ typedef uint32_t unaligned_uint32_t;
>  typedef uint16_t unaligned_uint16_t;
>  #endif
> 
> +/**
> + * @deprecated
> + * @see __rte_packed_begin
> + * @see __rte_packed_end
> + *
> + * Force a structure to be packed
> + */
> +#ifdef RTE_TOOLCHAIN_MSVC
> +#define __rte_packed RTE_DEPRECATED(__rte_packed)
> +#else
> +#define __rte_packed (RTE_DEPRECATED(__rte_packed)
> __attribute__((__packed__)))
> +#endif
> +
>  /**
>   * Force a structure to be packed
>   */
>  #ifdef RTE_TOOLCHAIN_MSVC
> -#define __rte_packed
> +#define __rte_packed_begin __pragma(pack(push, 1))
> +#define __rte_packed_end __pragma(pack(pop))
>  #else
> -#define __rte_packed __attribute__((__packed__))
> +#define __rte_packed_begin
> +#define __rte_packed_end __attribute__((__packed__))
>  #endif
> 
>  /**
> --
> 2.47.0.vfs.0.3

In the following patches in this series, the placement of the __rte_packed_begin attribute is wrong for GCC.
Like the __rte_aligned() attribute, the __rte_packed_begin attribute needs to be after the "struct" keyword, not before.

The GCC documentation [1] says:
You may specify type attributes in an enum, struct or union type declaration or definition by placing them immediately after the struct, union or enum keyword.
You can also place them just past the closing curly brace of the definition, but this is less preferred because logically the type should be fully defined at the closing brace.
You can also include type attributes in a typedef declaration.

[1] https://gcc.gnu.org/onlinedocs/gcc/Type-Attributes.html

<feature creep>
The documentation comment preceding each macro in rte_common.h should include a brief description of how to use the macro.
This goes for most macros in rte_common.h, not only the macros for packing.
</feature creep>