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

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

On Fri, Nov 22, 2024 at 09:13:38AM +0100, Morten Brørup wrote:
> > From: Andre Muezerie [mailto:andremue@linux.microsoft.com]
> > Sent: Friday, 22 November 2024 01.12
> > 
> > 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_kGuRHOzqd90R
> > D2HXZyw@mail.gmail.com/
> > > > >
> > > > > The definition of the packing macro in OVS, for reference:
> > > > >
> > https://github.com/openvswitch/ovs/blob/main/include/openvswitch/compil
> > er.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.
> 
> No problem.
> 
> > > >
> > > > 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.
> 
> Good catch.
> Although it is relatively rare, it is a standard design pattern, also in other projects than DPDK. We should either support it, or emit an error on occurrence when compiling.
> 
> > >
> > > 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.
> 
> Many network protocol structures contain uint32_t/rte_be32_t fields, which makes the structures 4-byte aligned; but the preceding 14 byte Ethernet header makes them non-4-byte aligned. So their alignment needs to be reduced from 4-byte to 2-byte.
> 
> > >
> > 
> > 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.
> 
> Agree.
> If we don't like "msvc" in the macro name, we could rename __rte_msvc_pack to __rte_packed_begin.
> We could also rename __rte_packed to __rte_packed_end, emphasizing that __rte_packed is no longer allowed at the beginning of structures.
> And for backwards compatibility add #define __rte_packed __rte_packed_end.
> 
> Structures would then look like this:
> 
> struct __rte_packed_begin __rte_aligned(2) rte_proto_hdr {
> 	rte_be32_t	proto_field;
> ...
> } __rte_packed_end;
> 
> If you prefer keeping __rte_packed instead of renaming it to __rte_packed_end, it would probably be easy for checkpatch to verify its location in the structure declaration, as it must always be directly preceded by whitespace and directly followed by a semicolon.
> (The same goes for __rte_packed_end.)
> 
> Checkpatch could also verify that each __rte_packed_end/__rte_packed is preceded by __rte_packed_begin, for MSVC compatibility.

Makes sense. I'll update the series accordingly.