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 719F245E33; Thu, 5 Dec 2024 01:20:03 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 05169402AF; Thu, 5 Dec 2024 01:20:03 +0100 (CET) Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by mails.dpdk.org (Postfix) with ESMTP id 13CB34029B; Thu, 5 Dec 2024 01:20:01 +0100 (CET) Received: by linux.microsoft.com (Postfix, from userid 1086) id 5B23F20BCAFF; Wed, 4 Dec 2024 16:20:00 -0800 (PST) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com 5B23F20BCAFF DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1733358000; bh=Dze8HXSyn+W489EERJUiYUr4aUVdTsMsKQHJe7BG9nY=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=UmkTMvrQdbBba4jKDf8KdOiH53k9yiOL/J5GJ9yQoCPpET/XuPWZV2u9+RxWJeyZj m0/4RIEOjOuBPaenSLp5fpe1YRopszzFIh//3fKFW/feahhMTekpe2yAwKRAYEIULE M4gDKRxyZK9liqYxHIea1b7/Hrc3/j0cfyGkQ/s8= Date: Wed, 4 Dec 2024 16:20:00 -0800 From: Tyler Retzlaff To: Morten =?iso-8859-1?Q?Br=F8rup?= Cc: Andre Muezerie , techboard@dpdk.org, Yuying.Zhang@intel.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 v5 01/16] eal: provide pack start macro for MSVC Message-ID: <20241205002000.GE2333@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> References: <1710968771-16435-1-git-send-email-roretzla@linux.microsoft.com> <1731990941-10001-1-git-send-email-andremue@linux.microsoft.com> <1731990941-10001-2-git-send-email-andremue@linux.microsoft.com> <98CBD80474FA8B44BF855DF32C47DC35E9F8D2@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: <98CBD80474FA8B44BF855DF32C47DC35E9F8D2@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 Tue, Nov 19, 2024 at 09:32:07AM +0100, Morten Brørup wrote: > > From: Andre Muezerie [mailto:andremue@linux.microsoft.com] > > Sent: Tuesday, 19 November 2024 05.35 > > > > From: Tyler Retzlaff > > > > MSVC struct packing is not compatible with GCC. Provide a macro that > > can be used to push existing pack value and sets packing to 1-byte. > > The existing __rte_packed macro is then used to restore the pack value > > prior to the push. > > > > Instead of providing macros exclusively for MSVC and for GCC the > > existing macro is deliberately utilized to trigger a warning if no > > existing packing has been pushed allowing easy identification of > > locations where the __rte_msvc_pack is missing. > > > > Signed-off-by: Tyler Retzlaff > > --- > > lib/eal/include/rte_common.h | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/lib/eal/include/rte_common.h > > b/lib/eal/include/rte_common.h > > index 4d299f2b36..409890863e 100644 > > --- a/lib/eal/include/rte_common.h > > +++ b/lib/eal/include/rte_common.h > > @@ -103,8 +103,10 @@ typedef uint16_t unaligned_uint16_t; > > * Force a structure to be packed > > */ > > #ifdef RTE_TOOLCHAIN_MSVC > > -#define __rte_packed > > +#define __rte_msvc_pack __pragma(pack(push, 1)) > > +#define __rte_packed __pragma(pack(pop)) > > #else > > +#define __rte_msvc_pack > > #define __rte_packed __attribute__((__packed__)) > > #endif > > > > -- > > 2.47.0.vfs.0.3 > > Before proceeding with this, can we please discuss the alternative, proposed here: > https://inbox.dpdk.org/dev/CAJFAV8yStgiBbe+Nkt9mC30r0+ZP64_kGuRHOzqd90RD2HXZyw@mail.gmail.com/ > > The definition of the packing macro in OVS, for reference: > https://github.com/openvswitch/ovs/blob/main/include/openvswitch/compiler.h#L209 > > The current solution requires __rte_packed to be placed at the end of a structure, although __attribute__((packed)) is normally allowed at the beginning (between the "struct" tag and the name of the structure), which introduces a high risk of contributors placing it "incorrectly", thus causing errors. > > I have a strong preference for an __RTE_PACKED(decl) variant. > > Here's a third alternative: > #ifdef RTE_TOOLCHAIN_MSVC > #define __rte_msvc_pack_begin __pragma(pack(push, 1)) > #define __rte_msvc_pack_end __pragma(pack(pop)) > #else > #define __rte_msvc_pack_begin > #define __rte_msvc_pack_end > #endif > > The third alternative is also problematic, e.g. if a contributor forgets the _end after the structure declaration, or adds another structure declaration before the _end. so just some additional info here. for the compiler that cares about the end (i.e. msvc) msvc actually emits a warning (treated as error) if a push pack is not closed with a pop pack within a header or a tu. that's one of the benefits of the macros in original proposal. > > -Morten