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 > <andre...@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=d...@mail.gmail.com/. > Future will tell us. > > Series applied, thanks Andre. > > > -- > David Marchand
That's great news. Thanks for merging this David!
