On Sat, Dec 28, 2024 at 03:41:39PM +0100, Morten Brørup wrote: > > From: Andre Muezerie [mailto:andre...@linux.microsoft.com] > > Sent: Monday, 23 December 2024 20.12 > > > > MSVC struct packing is not compatible with GCC. Add macro > > __rte_packed_begin which can be used to push existing pack value > > and set packing to 1-byte. Add macro __rte_packed_end to restore > > the pack value prior to the push. > > Nice to have: > Some alternative solutions were discussed on the mailing list. > Please consider adding a brief summary of them with their > benefits/disadvantages to the patch description, either here or in the cover > letter (0/NN) of the patch series. >
Sure, I'll add a brief summary to the next patch series. > > > > 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 was marked as deprecated. > > > > Signed-off-by: Andre Muezerie <andre...@linux.microsoft.com> > > Acked-by: Tyler Retzlaff <roret...@linux.microsoft.com> > > --- > > lib/eal/include/rte_common.h | 19 +++++++++++++++++-- > > 1 file changed, 17 insertions(+), 2 deletions(-) > > > > diff --git a/lib/eal/include/rte_common.h > > b/lib/eal/include/rte_common.h > > index 4d299f2b36..e40910fa20 100644 > > --- a/lib/eal/include/rte_common.h > > +++ b/lib/eal/include/rte_common.h > > @@ -99,13 +99,28 @@ typedef uint32_t unaligned_uint32_t; > > typedef uint16_t unaligned_uint16_t; > > #endif > > > > +/** > > + * @deprecated > > + * @see __rte_packed_begin > > + * @see __rte_packed_end > > + * > > + * Force a structure to be packed > > + */ > > +#ifdef RTE_TOOLCHAIN_MSVC > > +#define __rte_packed RTE_DEPRECATED(__rte_packed) > > +#else > > +#define __rte_packed (RTE_DEPRECATED(__rte_packed) > > __attribute__((__packed__))) > > +#endif > > + > > /** > > * Force a structure to be packed > > */ > > #ifdef RTE_TOOLCHAIN_MSVC > > -#define __rte_packed > > +#define __rte_packed_begin __pragma(pack(push, 1)) > > +#define __rte_packed_end __pragma(pack(pop)) > > #else > > -#define __rte_packed __attribute__((__packed__)) > > +#define __rte_packed_begin > > +#define __rte_packed_end __attribute__((__packed__)) > > #endif > > > > /** > > -- > > 2.47.0.vfs.0.3 > > In the following patches in this series, the placement of the > __rte_packed_begin attribute is wrong for GCC. > Like the __rte_aligned() attribute, the __rte_packed_begin attribute needs to > be after the "struct" keyword, not before. > > The GCC documentation [1] says: > You may specify type attributes in an enum, struct or union type declaration > or definition by placing them immediately after the struct, union or enum > keyword. > You can also place them just past the closing curly brace of the definition, > but this is less preferred because logically the type should be fully defined > at the closing brace. > You can also include type attributes in a typedef declaration. Thanks for noticing this. Indeed the documentation is clear on this topic. It's curious that the compiler did not complain, and actually packed a structure when the attribute was (incorrectly) placed before the "struct" keyword. > > [1] https://gcc.gnu.org/onlinedocs/gcc/Type-Attributes.html > > <feature creep> > The documentation comment preceding each macro in rte_common.h should include > a brief description of how to use the macro. > This goes for most macros in rte_common.h, not only the macros for packing. > </feature creep> I'll add better comments to the new code being added in this series, and keep this in mind for future patches.
