Re: [PATCH v5 01/16] eal: provide pack start macro for MSVC

[Thomas Monjalon <thomas@monjalon.net>]

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:andremue@linux.microsoft.com]
> > > Sent: Tuesday, 19 November 2024 05.35
> > > 
> > > From: Tyler Retzlaff <roretzla@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 <roretzla@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_kGuRHOzqd90RD2HXZyw@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.