Re: [PATCH v11 00/30] fix packing of structs when building with MSVC

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

On Wed, Jan 15, 2025 at 05:13:22PM +0100, David Marchand wrote:
> Hello Andre,
> 
> On Fri, Jan 10, 2025 at 11:17 PM Andre Muezerie
> <andremue@linux.microsoft.com> wrote:
> >
> > MSVC struct packing is not compatible with GCC. Different alternatives
> > were considered:
> >
> > 1) Have a macro __RTE_PACKED(decl) to which the struct/union is passed
> >    and the macro would define the struct/union with the appropriate
> >    packing attribute for the compiler in use.
> >
> > Advantages:
> >  * Can be placed in front of a struct, or even in the middle. Good
> >    for readability.
> >  * Does not require a different macro to be placed at the end of the
> >    structure.
> >
> > However, problems can arise when compiler directives are present in the
> > struct, as they become arguments for __RTE_PACKED macro. This is not
> > portable. Two problematic situations observed in the DPDK code:
> >
> >   a) #defines mentioned in the struct. In this situation we could just
> >      move the #define out of the struct.
> >
> >   b) #if/#ifdef/#elif mentioned in the struct.
> >      This is a somewhat common pattern in structs where fields change
> >      based on endianness and would require code duplication to be
> >      handled, which makes the code hard to read and maintain.
> >
> > 2) Have macros __rte_msvc_pack_begin and __rte_msvc_pack_end which
> >    would be placed at the beginning and end of the struct/union
> >    respectively. Concerns were raised about having macros for
> >    specific compilers, or even having compiler names mentioned in
> >    the macros' names.
> >
> > 3) Instead of providing macros exclusively for MSVC and for GCC,
> >    have a macro __rte_packed_begin and __rte_packed_end which would
> >    be placed at the beginning and end of the struct/union respectively.
> >    With MSVC both macros end up having a purpose. With GCC and Clang
> >    only __rte_packed_end has a purpose, as can be seen below.
> >    This makes the solution generic and is the approach taken in this
> >    patchset.
> >
> >    #ifdef RTE_TOOLCHAIN_MSVC
> >    #define __rte_packed_begin __pragma(pack(push, 1))
> >    #define __rte_packed_end __pragma(pack(pop))
> >    #else
> >    #define __rte_packed_begin
> >    #define __rte_packed_end __attribute__((__packed__))
> >    #endif
> >
> >    Macro __rte_packed_end is deliberately utilized to trigger a
> >    MSVC compiler warning if no existing packing has been pushed allowing
> >    easy identification of locations where the __rte_packed_begin is
> >    missing.
> >
> > Macro __rte_packed is marked deprecated and the two new macros represent
> > the new way to enable packing in the DPDK code.
> >
> > Script checkpatches.sh was enhanced to ensure that:
> >  * __rte_packed_begin and __rte_packed_end show up in pairs.
> >  * __rte_packed_begin is not used with enums.
> >  * __rte_packed_begin is only used after struct, union,
> >    __rte_cache_aligned, __rte_cache_min_aligned or __rte_aligned
> >
> 
> I did a couple of small changes:
> - the __rte_packed_macro is kept in the first patch and removed in the
> last one, to avoid breaking patch per patch compilation,
> - fixed some small coding style issues (like double space before
> __rte_packed_end),
> - moved dropping __rte_packed into separate commits for better
> visibility of those changes,
> - squashed all libraries, drivers and examples into one separate
> commit each. Splitting into many patches did not help get more
> reviews, and for such global changes I see no advantage to keep those
> many commits.
> - shortened a bit the checkpatch warnings,
> - dropped the change on gve base driver, to be on the safe side, since
> GVE maintainers did not reply.
>   Other changes on base drivers have been kept when the code was
> already using __rte_* macros/attributes,
> - added a RN entry,
> 
> I did not touch the check on counting __rte_packed_begin/end.
> I think we will get various false positives as I described in
> https://inbox.dpdk.org/dev/CAJFAV8w=s1L-WYk+Qv-B+Mn6eAwKrB=GTz6hU--ZoLrJsz7=DQ@mail.gmail.com/.
> Future will tell us.
> 
> Series applied, thanks Andre.
> 
> 
> -- 
> David Marchand

That's great news. Thanks for merging this David!