Re: [PATCH v6 00/30] fix packing of structs when building with MSVC

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

Hello Andre,

On Wed, Nov 27, 2024 at 1:53 AM Andre Muezerie
<andremue@linux.microsoft.com> wrote:
>
> MSVC struct packing is not compatible with GCC. Provide a macro
> (__rte_packed_begin) that can be used to push existing pack value
> and sets packing to 1-byte. The existing __rte_packed macro is
> replaced with __rte_packed_end and restores the pack value
> prior to the push.
>
> Instead of providing macros exclusively for MSVC and for GCC,
> 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 is removed and the two new macros represent the
> new way to enable packing in the DPDK code.
>
> Script checkpatches.sh was enhanced to ensure __rte_packed_begin and
> __rte_packed_end show up in pairs when checking patches.
>
> If as a part of review maintainers identify structs they believe
> don't require packing so long as they are explicitly identified
> I'll remove the __rte_packed as a part of this series.
>
> v6:
>   * replace __rte_msvc_pack with __rte_packed_begin
>   * replace __rte_packed with __rte_packed_end
>   * update checkpatches.sh to ensure __rte_packed_begin and
>     __rte_packed_end are used in pairs

I had mentionned this in a separate thread.
Why not do like OVS and have a RTE_PACKED() macro?

#ifdef RTE_TOOLCHAIN_MSVC
#define RTE_PACKED(...) __pragma(pack(push, 1)) __VA_ARGS__ __pragma(pack(pop))
#else
#define RTE_PACKED(...) __VA_ARGS__ __attribute__((__packed__))
#endif

This removes the need for updating checkpatch.
Plus, builds on Linux will catch issues (hopefully by the author of
the change, before submitting).


>   * remove __rte_packed

Please mark it deprecated for now (see RTE_DEPRECATED / add a
deprecation notice) and we will remove it in 25.11.


-- 
David Marchand