From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 208D245F86; Tue, 31 Dec 2024 16:07:45 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id A4A7A4026E; Tue, 31 Dec 2024 16:07:44 +0100 (CET) Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by mails.dpdk.org (Postfix) with ESMTP id C8F0F40263 for ; Tue, 31 Dec 2024 16:07:42 +0100 (CET) Received: by linux.microsoft.com (Postfix, from userid 1213) id CA8DB204675A; Tue, 31 Dec 2024 07:07:41 -0800 (PST) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com CA8DB204675A DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1735657661; bh=ihunBPI3q8lGAj27zJ+Q1L/kLc3ulBmquv+5+HDv5wU=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=IrmC9eYj+Y1nii8HK+qkd0wJqcIHIO7MvsmbOehK/d9Vl9bD+/7c66BTpC5OZEiBX 0ZHl+NU6P0Du/wbMDeRgwkPRNwmokoWncvh/aDXKQShOnsX0FKa2YwCNFsIGreKQg3 puoFK1U4VYcbOWvjf5DdQhIFsjsnzXNCFnjQcQnM= Date: Tue, 31 Dec 2024 07:07:41 -0800 From: Andre Muezerie To: Morten =?iso-8859-1?Q?Br=F8rup?= Cc: roretzla@linux.microsoft.com, aman.deep.singh@intel.com, anatoly.burakov@intel.com, bruce.richardson@intel.com, byron.marohn@intel.com, conor.walsh@intel.com, cristian.dumitrescu@intel.com, david.hunt@intel.com, dev@dpdk.org, dsosnowski@nvidia.com, gakhil@marvell.com, jerinj@marvell.com, jingjing.wu@intel.com, kirill.rybalchenko@intel.com, konstantin.v.ananyev@yandex.ru, matan@nvidia.com, orika@nvidia.com, radu.nicolau@intel.com, ruifeng.wang@arm.com, sameh.gobriel@intel.com, sivaprasad.tummala@amd.com, skori@marvell.com, stephen@networkplumber.org, suanmingm@nvidia.com, vattunuru@marvell.com, viacheslavo@nvidia.com, vladimir.medvedkin@intel.com, yipeng1.wang@intel.com Subject: Re: [PATCH v7 02/29] eal/include: add new packing macros Message-ID: <20241231150741.GA28057@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> References: <1710968771-16435-1-git-send-email-roretzla@linux.microsoft.com> <1734981122-4729-1-git-send-email-andremue@linux.microsoft.com> <1734981122-4729-3-git-send-email-andremue@linux.microsoft.com> <98CBD80474FA8B44BF855DF32C47DC35E9F977@smartserver.smartshare.dk> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <98CBD80474FA8B44BF855DF32C47DC35E9F977@smartserver.smartshare.dk> User-Agent: Mutt/1.5.21 (2010-09-15) X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org On Sat, Dec 28, 2024 at 03:41:39PM +0100, Morten Brørup wrote: > > From: Andre Muezerie [mailto:andremue@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 > > Acked-by: Tyler Retzlaff > > --- > > 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 > > > 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. > I'll add better comments to the new code being added in this series, and keep this in mind for future patches.