On Tue, Nov 19, 2024 at 09:32:07AM +0100, Morten Brørup wrote: > > 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 > > 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.
so just some additional info here. for the compiler that cares about the end (i.e. msvc) msvc actually emits a warning (treated as error) if a push pack is not closed with a pop pack within a header or a tu. that's one of the benefits of the macros in original proposal. > > -Morten
