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

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

On Thu, Nov 21, 2024 at 09:51:36PM +0100, Thomas Monjalon wrote:
> 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.
> 

Indeed, it's is a shame that the way MSVC works differs so much, but
unfortunately that won't change.

Considering the implications of the __RTE_PACK macro approach on these
special cases, perhaps the initial proposal made in the series that was
submitted for review is better after all? At least it does not have
these "special" cases, so the approach required on the code would be
uniform in the entire code base.