From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: <dev-bounces@dpdk.org> Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 908E046093; Wed, 15 Jan 2025 17:51:51 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 254B4402B5; Wed, 15 Jan 2025 17:51:51 +0100 (CET) Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by mails.dpdk.org (Postfix) with ESMTP id 8D5B940298 for <dev@dpdk.org>; Wed, 15 Jan 2025 17:51:48 +0100 (CET) Received: by linux.microsoft.com (Postfix, from userid 1213) id D51E12043F18; Wed, 15 Jan 2025 08:51:47 -0800 (PST) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com D51E12043F18 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1736959907; bh=ND5sYr7+wZ643X+V0Cy4aJdhjbkW5uSUUA7vxKDT+Fs=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=bw2OUYbnkVWTBUEh12JU9TxJDxSIGK6VxHgPtk1qlxpUz17Qm+Flu7RtozL6xgxkO /AqR/y94POxL+RxMN3PQLdzAW+HArU5oRJyBtinDJ8EfDMR4+Gx3BzRgMqyycEHHiH XrnHNYLwPE4SUgAE/plh9gIf67uwyjVTbx9d7k+U= Date: Wed, 15 Jan 2025 08:51:47 -0800 From: Andre Muezerie <andremue@linux.microsoft.com> To: David Marchand <david.marchand@redhat.com> 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, mb@smartsharesystems.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 v11 00/30] fix packing of structs when building with MSVC Message-ID: <20250115165147.GA7424@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> References: <1710968771-16435-1-git-send-email-roretzla@linux.microsoft.com> <1736547411-5895-1-git-send-email-andremue@linux.microsoft.com> <CAJFAV8xmrcBBJs-58w4yz6CEp7dCNf5MMHAZpy2K8h+JNYATRg@mail.gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <CAJFAV8xmrcBBJs-58w4yz6CEp7dCNf5MMHAZpy2K8h+JNYATRg@mail.gmail.com> 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 <dev.dpdk.org> List-Unsubscribe: <https://mails.dpdk.org/options/dev>, <mailto:dev-request@dpdk.org?subject=unsubscribe> List-Archive: <http://mails.dpdk.org/archives/dev/> List-Post: <mailto:dev@dpdk.org> List-Help: <mailto:dev-request@dpdk.org?subject=help> List-Subscribe: <https://mails.dpdk.org/listinfo/dev>, <mailto:dev-request@dpdk.org?subject=subscribe> Errors-To: dev-bounces@dpdk.org On Wed, Jan 15, 2025 at 05:13:22PM +0100, David Marchand wrote: > Hello Andre, > > On Fri, Jan 10, 2025 at 11:17 PM Andre Muezerie > <andremue@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=DQ@mail.gmail.com/. > Future will tell us. > > Series applied, thanks Andre. > > > -- > David Marchand That's great news. Thanks for merging this David!