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 AD35E4608D; Wed, 15 Jan 2025 17:13:41 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 92D9B402B5; Wed, 15 Jan 2025 17:13:41 +0100 (CET) Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by mails.dpdk.org (Postfix) with ESMTP id 4AB2A40298 for ; Wed, 15 Jan 2025 17:13:39 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1736957618; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=OAfaU4dwi1HRTA/npj8/QU2r8x9cfXZ3fvbKwJuOPoA=; b=XEaH+F73BtZRqsyoBTE/qkNvmDXkT7h8TAggmL09XXOBJa/is9Zs1r1WwtR5hlz/AO/FJx 2rX0ELpO2le+/9RvlPfA0XrUYSkaioE+kxj630NIQ/S9Nt9ATEVOR9MF8IJq1S96W/3Cx6 AG4m4VsHuZTUC1rAdl35XUDB/Iz9HQU= Received: from mail-lf1-f70.google.com (mail-lf1-f70.google.com [209.85.167.70]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-544-ko49VWvxPqqu4StlzX_M4g-1; Wed, 15 Jan 2025 11:13:35 -0500 X-MC-Unique: ko49VWvxPqqu4StlzX_M4g-1 X-Mimecast-MFC-AGG-ID: ko49VWvxPqqu4StlzX_M4g Received: by mail-lf1-f70.google.com with SMTP id 2adb3069b0e04-53f167e95e6so520829e87.1 for ; Wed, 15 Jan 2025 08:13:35 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1736957614; x=1737562414; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=OAfaU4dwi1HRTA/npj8/QU2r8x9cfXZ3fvbKwJuOPoA=; b=wOtKgftknj/kHXGLfpZRSiF+wlkRRJk2MvUeK4zQK1udSDCWGV+TDyDm+xwodbIfmD osKRKiZXoNqokPiiSJR1WnWoCsWhSvN8RjpXpYz7RH3/qsnIGOqjbWTkph94wtiWJKqX xIOgD42h9ncIT2emCN9AG2dzMuOrR/auzMTyAIcRFQlczN1s4w9E31X0mHodp0cnHJgt 2GGTB6eekXmP3BrEuIedSu32L6sRVGxWbGePuuVxmZ1VarGZ6hcJ73RNMVoO9UO0o0KU jcQroMgWzyUVpM1cbcpRK//n5ciUMVf4+3s1cVhplNuPUotmq0C90pCBh3sii0ZkyaIC CwVg== X-Forwarded-Encrypted: i=1; AJvYcCWm89A98fCXRpDqYpIvw7iEn9Tu6sP5FadNrUhfqxqvXbGUNCmrwJbg/fs/vcfIs3RsEVI=@dpdk.org X-Gm-Message-State: AOJu0YwkpQnzj5CD+aNvYIPjpp+FgfPaADBcS+FZQLx2KAl8A/empxnZ l11bqntrMyGN2R1/2W80+Oy4V53y01kbT5OzuTRQnzJAKKlPWhfd80sYpgBFCJvvE9W1PYaW+24 KG9iTbgnI8yrZgAMAMSUlLkBAPW1d7utIncrALSWT3bd/MMI04ZiN1c7cOaWsStx5JkhFoBRGGl wGtHUHgaJZDzJY+Q0= X-Gm-Gg: ASbGncvrv/C8QJzPgOnXeoftdyXWRY7b+jRAd4P+F59W8Hge7UZY69+X6TaeMNARrNh K+PyxYQflpvyaY3awnUyLRx4Ocvy43jHarDZ+3nvV X-Received: by 2002:a05:6512:6d3:b0:53f:67be:507 with SMTP id 2adb3069b0e04-542abfb56d7mr1051639e87.16.1736957613919; Wed, 15 Jan 2025 08:13:33 -0800 (PST) X-Google-Smtp-Source: AGHT+IGND/EH2QQXmTFuKdmVOvdE/aw8KtkB0TgchsCFQI+mEGmPlvEDLJp7fRFaZHoZ/5EGx/BKBANUkSY+FTCzIQs= X-Received: by 2002:a05:6512:6d3:b0:53f:67be:507 with SMTP id 2adb3069b0e04-542abfb56d7mr1051615e87.16.1736957613496; Wed, 15 Jan 2025 08:13:33 -0800 (PST) MIME-Version: 1.0 References: <1710968771-16435-1-git-send-email-roretzla@linux.microsoft.com> <1736547411-5895-1-git-send-email-andremue@linux.microsoft.com> In-Reply-To: <1736547411-5895-1-git-send-email-andremue@linux.microsoft.com> From: David Marchand Date: Wed, 15 Jan 2025 17:13:22 +0100 X-Gm-Features: AbW1kvZUjP5UwGf7zDFAjOu264ZllMTYxtZrAo3dfjr_NhqEuJgzjyekaS5RZf8 Message-ID: Subject: Re: [PATCH v11 00/30] fix packing of structs when building with MSVC To: Andre Muezerie 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 X-Mimecast-Spam-Score: 0 X-Mimecast-MFC-PROC-ID: yQVGayNocSZ-DvKlqZAGVW_6Zb_YXP9e7to5QJ1IRZs_1736957614 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable 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 Hello Andre, On Fri, Jan 10, 2025 at 11:17=E2=80=AFPM Andre Muezerie 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=3Ds1L-WYk+Qv-B+Mn6eAwKrB=3DGTz6hU--ZoLr= Jsz7=3DDQ@mail.gmail.com/. Future will tell us. Series applied, thanks Andre. --=20 David Marchand