> -----Original Message-----
> From: Morten Brørup <m...@smartsharesystems.com>
> Sent: Tuesday, November 19, 2024 8:32 AM
> To: Andre Muezerie <andre...@linux.microsoft.com>;
> roret...@linux.microsoft.com; techbo...@dpdk.org
> Cc: yuying.zh...@intel.com; aman.deep.si...@intel.com;
> anatoly.bura...@intel.com; bruce.richard...@intel.com;
> byron.mar...@intel.com; conor.wa...@intel.com; cristian.dumitre...@intel.com;
> david.h...@intel.com; dev@dpdk.org;
> dsosnow...@nvidia.com; gak...@marvell.com; jer...@marvell.com;
> jingjing...@intel.com; kirill.rybalche...@intel.com;
> konstantin.v.anan...@yandex.ru; ma...@nvidia.com; or...@nvidia.com;
> radu.nico...@intel.com; ruifeng.w...@arm.com;
> sameh.gobr...@intel.com; sivaprasad.tumm...@amd.com; sk...@marvell.com;
> step...@networkplumber.org;
> suanmi...@nvidia.com; vattun...@marvell.com; viachesl...@nvidia.com;
> vladimir.medved...@intel.com;
> yipeng1.w...@intel.com
> Subject: RE: [PATCH v5 01/16] eal: provide pack start macro for MSVC
>
> > From: Andre Muezerie [mailto:andre...@linux.microsoft.com]
> > Sent: Tuesday, 19 November 2024 05.35
> >
> > From: Tyler Retzlaff <roret...@linux.microsoft.com>
> >
> > MSVC struct packing is not compatible with GCC. Provide a macro that
> > can be used to push existing pack value and sets packing to 1-byte.
> > The existing __rte_packed macro is then used to restore the pack value
> > prior to the push.
> >
> > Instead of providing macros exclusively for MSVC and for GCC the
> > existing macro is deliberately utilized to trigger a warning if no
> > existing packing has been pushed allowing easy identification of
> > locations where the __rte_msvc_pack is missing.
> >
> > Signed-off-by: Tyler Retzlaff <roret...@linux.microsoft.com>
> > ---
> > lib/eal/include/rte_common.h | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/lib/eal/include/rte_common.h
> > b/lib/eal/include/rte_common.h
> > index 4d299f2b36..409890863e 100644
> > --- a/lib/eal/include/rte_common.h
> > +++ b/lib/eal/include/rte_common.h
> > @@ -103,8 +103,10 @@ typedef uint16_t unaligned_uint16_t;
> > * Force a structure to be packed
> > */
> > #ifdef RTE_TOOLCHAIN_MSVC
> > -#define __rte_packed
> > +#define __rte_msvc_pack __pragma(pack(push, 1))
> > +#define __rte_packed __pragma(pack(pop))
> > #else
> > +#define __rte_msvc_pack
> > #define __rte_packed __attribute__((__packed__))
> > #endif
> >
> > --
> > 2.47.0.vfs.0.3
>
> Before proceeding with this, can we please discuss the alternative, proposed
> here:
> https://inbox.dpdk.org/dev/cajfav8ystgibbe+nkt9mc30r0+zp64_kgurhozqd90rd2hx...@mail.gmail.com/
>
> The definition of the packing macro in OVS, for reference:
> https://github.com/openvswitch/ovs/blob/main/include/openvswitch/compiler.h#L209
Yes, that one looks much nicer to me too.
Could I also ask to avoid creeping _msvc_ into DPDK namespace.
>
> The current solution requires __rte_packed to be placed at the end of a
> structure, although __attribute__((packed)) is normally
> allowed at the beginning (between the "struct" tag and the name of the
> structure), which introduces a high risk of contributors placing
> it "incorrectly", thus causing errors.
>
> I have a strong preference for an __RTE_PACKED(decl) variant.
>
> Here's a third alternative:
> #ifdef RTE_TOOLCHAIN_MSVC
> #define __rte_msvc_pack_begin __pragma(pack(push, 1))
> #define __rte_msvc_pack_end __pragma(pack(pop))
> #else
> #define __rte_msvc_pack_begin
> #define __rte_msvc_pack_end
> #endif
>
> The third alternative is also problematic, e.g. if a contributor forgets the
> _end after the structure declaration, or adds another
> structure declaration before the _end.
>
> -Morten
