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
