21/11/2024 20:39, Andre Muezerie: > 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. > > > > -Morten > > I looked at the suggestions made and I liked the one having a __RTE_PACKED > macro > the most. > > Advantages: > - Can be placed in front of the struct, or even in the middle. Good for > readability. > - Does not require a different macro to be placed at the end of the structure > as was > proposed in V5 series. > - Works well in 99% of the cases. > > Problems can arise when compiler directives are present in the struct, as they > become arguments for __RTE_PACKED macro. This is not portable. > I've seen two situations in the DPDK code: > > 1) #defines mentioned in the struct. In this situation we can just move the > #define out of the struct. > > 2) #if/#ifdef/#elif mentioned in the struct. > This is a somewhat common pattern in structs where fields change based on > endianness. > Example: > > /** > * IPv4 Header > */ > struct __rte_aligned(2) rte_ipv4_hdr { > __extension__ > union { > uint8_t version_ihl; /**< version and header length */ > struct { > #if RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN > uint8_t ihl:4; /**< header length */ > uint8_t version:4; /**< version */ > #elif RTE_BYTE_ORDER == RTE_BIG_ENDIAN > uint8_t version:4; /**< version */ > uint8_t ihl:4; /**< header length */ > #endif > }; > }; > uint8_t type_of_service; /**< type of service */ > rte_be16_t total_length; /**< length of packet */ > ... > } __rte_packed; > > One way to solve this is to move the #if to the outside. But that involves > defining the struct twice (once for each endianness). It's less than > ideal because common parts would be duplicated. I'm not sure how popular > this would be. > It's not so common though (about 1% of the structs?). I think it's an > acceptable trade-off to get portable code, but I would like to hear your > thoughts.
This code would be portable if Microsoft would align with other compilers. Also I'm not sure we really need __rte_packed for most network protocols.
