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

[Andre Muezerie <andremue@linux.microsoft.com>]

On Mon, Dec 23, 2024 at 12:46:34PM +0100, David Marchand wrote:
> On Mon, Dec 23, 2024 at 12:03 PM David Marchand
> <david.marchand@redhat.com> wrote:
> > > 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
> 
> Mm, in practice, this would be problematic with struct where
> endianness matters (for example).
> 
> In file included from ../../../git/pub/dpdk.org/main/lib/net/rte_ip.h:9,
>                  from ../../../git/pub/dpdk.org/main/lib/ethdev/rte_flow.h:25,
>                  from
> ../../../git/pub/dpdk.org/main/lib/ethdev/rte_eth_ctrl.h:11,
>                  from
> ../../../git/pub/dpdk.org/main/lib/ethdev/rte_ethdev.h:1472,
>                  from
> ../../../git/pub/dpdk.org/main/lib/ethdev/ethdev_driver.h:21,
>                  from
> ../../../git/pub/dpdk.org/main/drivers/net/failsafe/failsafe_ops.c:15:
> ../../../git/pub/dpdk.org/main/lib/net/rte_ip4.h:48:1: error:
> embedding a directive within macro arguments is not portable [-Werror]
>    48 | #if RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN
>       | ^
> ../../../git/pub/dpdk.org/main/lib/net/rte_ip4.h:51:1: error:
> embedding a directive within macro arguments is not portable [-Werror]
>    51 | #elif RTE_BYTE_ORDER == RTE_BIG_ENDIAN
>       | ^
> ../../../git/pub/dpdk.org/main/lib/net/rte_ip4.h:54:1: error:
> embedding a directive within macro arguments is not portable [-Werror]
>    54 | #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.
> 
> Still, please rework this part.
> 
> 
> -- 
> David Marchand

Yes, I'll send out an updated series shortly.
--
Andre Muezerie