> From: David Marchand [mailto:david.march...@redhat.com] > Sent: Tuesday, 5 November 2024 10.28 > > On Tue, Nov 5, 2024 at 10:09 AM Morten Brørup > <m...@smartsharesystems.com> wrote: > > > diff --git a/lib/net/rte_ip4.h b/lib/net/rte_ip4.h > > > index 4dd0058cc5..f9b8333332 100644 > > > --- a/lib/net/rte_ip4.h > > > +++ b/lib/net/rte_ip4.h > > > @@ -39,7 +39,7 @@ extern "C" { > > > /** > > > * IPv4 Header > > > */ > > > -struct rte_ipv4_hdr { > > > +struct __rte_aligned(2) rte_ipv4_hdr { > > > > <unrelated> > > I wonder why we have a convention of putting __rte_packed after the > struct, and not between the "struct" tag and the name of the struct. > > It would make the code much more readable having it here, like > __rte_aligned(). > > </unrelated> > > I agree that the previous convention was not great, as it has resulted > in some funny jokes, like getting some __rte_XXX variables (in the > absence of the right header inclusion defining __rte_XXX attribute > macro). > > __rte_aligned() "conventional" location has been changed recently by > Tyler. > __rte_packed is still conventionnally placed in a "legacy" position > around the dpdk tree. > It could probably be moved in the same way. > > But there is still the question of packed structures with MSVC. > Tyler proposal seemed to rely on the current __rte_packed conventional > position. > https://patchwork.dpdk.org/project/dpdk/patch/1713225913-20792-2-git- > send-email-roret...@linux.microsoft.com/ > Note that I am not a fan of this push/pop stuff. > > Maybe Andre will find a better solution.
If we cannot come up with a clean solution that looks like an attribute (like GCC), we should accept MSVC's style with push/pop and learn to live with it. Perhaps something like: #ifdef RTE_TOOLCHAIN_MSVC #define __RTE_PACKED(...) \ __pragma(pack(push, 1)) \ __VA_ARGS__ \ __pragma(pack(pop)) #else #define __RTE_PACKED(...) __VA_ARGS__ __attribute__((__packed__)) #endif This would also move the "packed" information to the top of the struct, making the code easier to read - i.e. easier to notice that the structure is packed when not hidden away at the end of the structure. > > In any case, I prefer we keep __rte_packed position as is until this > question is resolved. Agree. Better to change it in one pass, instead of two.
