From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from outbound.mail.eo.outlook.com (outbound.mail.eo.outlook.com [40.93.3.7]) by mx.groups.io with SMTP id smtpd.web11.10281.1624982389362546051 for ; Tue, 29 Jun 2021 08:59:49 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@microsoft.com header.s=selector2 header.b=ODAtvlzH; spf=pass (domain: microsoft.com, ip: 40.93.3.7, mailfrom: bret.barkelew@microsoft.com) ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=J8sdM6mA3PiIw9yGrG8v9aSD7t9W2lqZv4cq2OjElbXiQCx10Dc+IswuuYifY1wF02K1EudPKNqaIBaa/82HahRNjUEigZsxJxnXPsKl9a4xyowea9Uv3AL9n4+EyQcr8GOUGMHJiITNeNJuxnVVQ0eWq7wGFPyd3rqpz1PP2fiTdxDa6yZebjJSql6nGJMuTDlA9kh+bDn3zPad8U5knzSZddB3ZbpHs7JC9fZJQwp6hx31GLNA45TrLb2m3iVYGCz0tidnen8kpQaNqXfRJAYsiUw0N0UH/GqdtIkpk6l3gn79NRhEJFKd3TazUwJ8c+402lhddebeUsyrXwzZZw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=R63Sm0LbmlS6wY+3yZ7EZDVWKYSzzPQOt59h2pbOlEs=; b=IOHCwOYp52hb0CN7wVoQoD8DQsmpoLdDD65dLBFms2zM3OcOYcmTkN1KI9XRyS8ZXAbGrYPM78JFoJeRvGvUwUGt8Uhn4V994W0lF2VgtK/9w36AS6AiWeJcuEGkEGtcSiVclnGi2uDzrZUHKZFC3HfU/4TdGA4ArFat2KApLslxhemsiRGecO8eexhrl5ntgoxbD83awhOU1Wn93fX8bW5hkqRXFBZVsQiIF7DkVly7kcpMWvz5uSErwq1Ze+N5WuuYoEAwe6cNUyn57FAatXLKa9+jDX6YA0phBHD/oVWukLZ/VETrwIe0eGVlU7MhpVr4y0QA6hlpw8T8ePMGug== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=microsoft.com; dmarc=pass action=none header.from=microsoft.com; dkim=pass header.d=microsoft.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=selector2; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=R63Sm0LbmlS6wY+3yZ7EZDVWKYSzzPQOt59h2pbOlEs=; b=ODAtvlzHgTP6e26lrh28wrb6SvwjoxUBJP70YIe2iLY/80hKsEdLacmcYfEsZYHDobYYZamPiRREnOJCWey0O8npHDvld+gI1wpSEmkteM+jsaeVmNJXBDITAdNl0hGG7Vka4dnSaeM4mac7rwzUdhPNvepuxUeQr/Oj43XXsgk= Received: from MW4PR21MB1907.namprd21.prod.outlook.com (2603:10b6:303:71::8) by MWHPR21MB0509.namprd21.prod.outlook.com (2603:10b6:300:df::11) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.4308.3; Tue, 29 Jun 2021 15:59:45 +0000 Received: from MW4PR21MB1907.namprd21.prod.outlook.com ([fe80::b198:f3a0:8cfc:c11c]) by MW4PR21MB1907.namprd21.prod.outlook.com ([fe80::b198:f3a0:8cfc:c11c%6]) with mapi id 15.20.4308.004; Tue, 29 Jun 2021 15:59:45 +0000 From: "Bret Barkelew" To: =?Windows-1252?Q?Marvin_H=E4user?= , Laszlo Ersek , Kun Qin , "Kinney, Michael D" , "devel@edk2.groups.io" CC: "Wang, Jian J" , "Wu, Hao A" , "Dong, Eric" , "Ni, Ray" , Liming Gao , "Liu, Zhiguang" , Andrew Fish , "Lindholm, Leif" , Michael Kubacki Subject: Re: [EXTERNAL] Re: [edk2-devel] [PATCH v1 0/5] EDK2 Code First: PI Specification: Update EFI_MM_COMMUNICATE_HEADER Thread-Topic: [EXTERNAL] Re: [edk2-devel] [PATCH v1 0/5] EDK2 Code First: PI Specification: Update EFI_MM_COMMUNICATE_HEADER Thread-Index: AQHXaQ02qiv47ByQH0uDTsMrgOLbTqslExIAgAR2x4CAAAyuAIAA/RRvgAAkCYCAAHXKSQ== Date: Tue, 29 Jun 2021 15:59:45 +0000 Message-ID: References: <20210610014259.1151-1-kuqin12@gmail.com> <93fd191e-e62f-f02e-11d0-403173fcdf42@posteo.de> <817ab349-b7a2-528b-9b78-aa72cefcd25a@posteo.de> <40bffd17-28a6-d280-02b1-628f1b2daa09@posteo.de> <1525bdb4-abfa-d89a-d7d9-7f8745640bff@gmail.com> <3f3ce50a-4ff0-d80f-c635-528751a0ab78@posteo.de> <0c931dbd-8a8d-fd61-b4ad-bd5ff16812d9@gmail.com> <127b368d-ba8e-6cb1-c8d4-179034be9d11@redhat.com> <5dd6ce4c-a3ba-8d56-dce9-0e699c919c5b@posteo.de> ,<09dd9917-a262-39d6-5611-e668cca3e168@posteo.de> In-Reply-To: <09dd9917-a262-39d6-5611-e668cca3e168@posteo.de> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: msip_labels: MSIP_Label_f42aa342-8706-4288-bd11-ebb85995028c_Enabled=True;MSIP_Label_f42aa342-8706-4288-bd11-ebb85995028c_SiteId=72f988bf-86f1-41af-91ab-2d7cd011db47;MSIP_Label_f42aa342-8706-4288-bd11-ebb85995028c_SetDate=2021-06-29T15:59:39.2979533Z;MSIP_Label_f42aa342-8706-4288-bd11-ebb85995028c_ContentBits=0;MSIP_Label_f42aa342-8706-4288-bd11-ebb85995028c_Method=Standard authentication-results: posteo.de; dkim=none (message not signed) header.d=none;posteo.de; dmarc=none action=none header.from=microsoft.com; x-ms-publictraffictype: Email x-ms-office365-filtering-correlation-id: 7d33aa7b-0861-4d48-a0a9-08d93b16ea37 x-ms-traffictypediagnostic: MWHPR21MB0509: x-ms-exchange-transport-forked: True x-ld-processed: 72f988bf-86f1-41af-91ab-2d7cd011db47,ExtAddr x-microsoft-antispam-prvs: x-ms-oob-tlc-oobclassifiers: OLM:3826; x-ms-exchange-senderadcheck: 1 x-microsoft-antispam: BCL:0; x-microsoft-antispam-message-info: x5D5lAnqi59Gmu1AxQ7IxzBug0/GbNUwW9eVAQKDKtjf+ZYJgrOCCXnGqVIvAR10al6TNWQgvjHrz+58hs59PNDGvn39JJJejBeZ0GHtA5qflDwnm/ds5X/QW7KfH2BRcmwv4ODRBLwoUf3XMor+6TW7V5kOFn2Gy0OEuA9Irj0Sb+5j6iDb8ntK5uyZBmLWeNUgwilecDmGoLcTJDO2qJDZc617V7vqSPjPVpqLRubg82/X1Il3vVIAUG0bRE2WhrGu+Kp7eGWL7wnV5b3I3jXYOtH4W5rIBY5lRNx1nR7WCCE217qY3xmAHb7KuRvf5/P8AN+b3knXc1+mRtIGkqjzG7vZbuaBVCDP9dLDlTkiHWk7TvOLAXDXFoB94dwZntfQt5/trRMwFNndpaWIfef8zU6Ynda9OYDkcTGstKG0EeEVVfiUp0/d4BPX7rivehct+ZovTcrvkfiMVMUbqo7y8ZGFV9nArSVElPVayrGqBmeNf3lE/DOmWvLCnhtZU4IN1iOhpDiL1CtR9k48sNAs/7PtxyKu1YQO7QW3N6vgabVMdB5c/Xba4AlpKMoUope1yGpoJvaCLMOuluyLKa3HHbgRHWABuONHSCZpRRMFxm/zhTcEO+2mxOkyeZHLZGJNaJpA+NHCzMThaXgZPg== x-forefront-antispam-report: CIP:255.255.255.255;CTRY:;LANG:en;SCL:1;SRV:;IPV:NLI;SFV:NSPM;H:MW4PR21MB1907.namprd21.prod.outlook.com;PTR:;CAT:NONE;SFS:(4636009)(366004)(76116006)(66574015)(6506007)(186003)(9686003)(86362001)(64756008)(7696005)(107886003)(10290500003)(66446008)(66946007)(8936002)(54906003)(8676002)(4326008)(2906002)(38100700002)(82950400001)(53546011)(110136005)(7416002)(122000001)(82960400001)(30864003)(55016002)(8990500004)(26005)(316002)(83380400001)(66556008)(66476007)(15650500001)(33656002)(52536014)(71200400001)(5660300002)(478600001);DIR:OUT;SFP:1102; x-ms-exchange-antispam-messagedata-chunkcount: 1 x-ms-exchange-antispam-messagedata-0: =?Windows-1252?Q?9U0pXR9x1oylx1N6MsX2E+3F6a4mu4qkcUiYvY/1tuyeKMQ6Fa/mypao?= =?Windows-1252?Q?r69fpob2f3UAf8mPgvG8jXfOn8XjIYk9x2VEyL5jPHtoP6aaBPKeyWiE?= =?Windows-1252?Q?PrqgBMkJvB+V4tQ9H4N7Vcpi7r4rtDVVrB3vBy8o7FlW2EBDiJG7TcAt?= =?Windows-1252?Q?DFvZqUcFMwkQqs6Av0r47hWt0imWb5kgrCDlWO21AadWW25g+gkIg0fQ?= =?Windows-1252?Q?pr2q5ZddtpWnzAF/JFPwP4VAt/H2YOSZFq8MGvgEu0dde3zXcmk7m+ZL?= =?Windows-1252?Q?nAJbLp6RsvqXpGEWrvVjqDswQSD9a6M23Kk7sxlGY7xOiGBqholu7/Oi?= =?Windows-1252?Q?ZdLnqDlcXdacojnWIc2CI8AM//3N9tEiDSUPRVdKxQRyM4N1xvdm5nw/?= =?Windows-1252?Q?sdC0+6scKM4QKVt+j8w+Bcyf/N+QZtlRzSvpJm5hvEc+t+f8ZW0rkuCx?= =?Windows-1252?Q?DVQBkNy9Cx7E0wZNaBlXKB4E9nHbDp5yFpP8I2H/oMww2dtXbcGOAZRo?= =?Windows-1252?Q?3m+EZorKpiZt6gWDZLdbT8Kvw/Ed6730uEcL4ymoHvEEx1ED4F4u1SMA?= =?Windows-1252?Q?BhgzPT42n+RWyZOWUzRM/z6MszgInhwF+tPOtzD7aX89U8wOHUDoxCTl?= =?Windows-1252?Q?ztVNbpuimHhFkPT9N/Akoqt0OxtmVM1Y0UyjKI3O605B3Tgp8d9Jyuo/?= =?Windows-1252?Q?1j6zBQNZSQCmAyLyE4NkmyhieBX4aK7gvXBb+UdI2rOS+LrTGSb4M0TR?= =?Windows-1252?Q?GI/rZ36SwaGkAVib4jpGFbLcX7ZMw/eJU8juV6Hq0A5HXDha2ghi/QbM?= =?Windows-1252?Q?9YPGJ2LI/hxaLuP7XqIHGk0tzarcsKgBDgfvFU5OTs9Wxv7055Byqvzc?= =?Windows-1252?Q?WWSRg2ODwUQfTCpZFwpydR/nGS7z8eEIoftMMJqx3A/lWam6imldi1Po?= =?Windows-1252?Q?ziqvkAKUbweZc26ES9GBbTwTPAAi98DqZW6mjibi9Eg7MwarPBYNo7hV?= =?Windows-1252?Q?rJ9P1ZVVK3XPiP5k6O7elTJlyMMXol1foUSfsjjCTA90YyIa6sYMfZwC?= =?Windows-1252?Q?lDZLEMrxp3waVvaO8MxaR9rM+7yTpCQrbc6aMrxgt9AohBO0vdROEJAb?= =?Windows-1252?Q?8xujqCfPaWvz+utl58qGr282jER28l26Q9Oz7RRHG+nBegWpgTjmwfxi?= =?Windows-1252?Q?6c8Mh4Dmq7Ko0gQkFoSi/hUnTP0V9mESoXqCYDY30KrCkc/zGPX0sBvj?= =?Windows-1252?Q?rT1J26HDMTIXJlIVoH0EitfHevcaEZy38H2TtOlKAG0hAtf+93ZapZXO?= =?Windows-1252?Q?hi49S4wMgg9zZtuvM2E1Fg605boMDHNCXNPuzzgfazYuOP/6czDTmoPI?= =?Windows-1252?Q?PJbwVDspvwPNoB/1yvuEAJRt/dy4FgFE9drh3ZCfmtNrOJrPMKbzkfCV?= =?Windows-1252?Q?1ky1kTnvHiXfdLMhkgerNQ=3D=3D?= MIME-Version: 1.0 X-OriginatorOrg: microsoft.com X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-AuthSource: MW4PR21MB1907.namprd21.prod.outlook.com X-MS-Exchange-CrossTenant-Network-Message-Id: 7d33aa7b-0861-4d48-a0a9-08d93b16ea37 X-MS-Exchange-CrossTenant-originalarrivaltime: 29 Jun 2021 15:59:45.1771 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 72f988bf-86f1-41af-91ab-2d7cd011db47 X-MS-Exchange-CrossTenant-mailboxtype: HOSTED X-MS-Exchange-CrossTenant-userprincipalname: rpd+OUrdeQfVirzy0jYsc9YWuQSt72Ypt9888hgWU5Q/5x2TLiV/k9LwZ/lL7Hgm69miJm+iBhtivyx0HatSiA== X-MS-Exchange-Transport-CrossTenantHeadersStamped: MWHPR21MB0509 Content-Language: en-US Content-Type: multipart/alternative; boundary="_000_MW4PR21MB190775DD7144B23C262B13EAEF029MW4PR21MB1907namp_" --_000_MW4PR21MB190775DD7144B23C262B13EAEF029MW4PR21MB1907namp_ Content-Type: text/plain; charset="Windows-1252" Content-Transfer-Encoding: quoted-printable Good note. Thanks! - Bret From: Marvin H=E4user Sent: Tuesday, June 29, 2021 1:58 AM To: Bret Barkelew; Laszlo Ersek; Kun Qin; Kinney, Michael D; devel@edk2.groups.io Cc: Wang, Jian J; Wu, Hao A; Dong, Eric; Ni, Ray; Liming Gao; Liu, Zhiguang; Andrew Fish; Lindholm, Le= if; Michael Kubacki Subject: Re: [EXTERNAL] Re: [edk2-devel] [PATCH v1 0/5] EDK2 Code First: PI= Specification: Update EFI_MM_COMMUNICATE_HEADER Generally yes, but gladly not for EDK II. Default GNU ABI uses 32-bit alignment for 64-bit integers on IA32 (which led to a (non-critical) mistake in our PE paper :( ) for example, but UEFI / EDK II (seem to) successfully dictate natural alignment consistently. Possibly we could introduce some STATIC_ASSERTs around important cases (e.g. UINT64 on 32-bit platforms) to ensure compilers keep it that way, once the ALIGNOF macro is introduced. Best regards, Marvin On 29.06.21 08:49, Bret Barkelew wrote: > > The fact that it may vary per ABI seems like a pretty big gotcha if > the SMM/MM Core was compiled at a different time or on a different > system than the module that=92s invoking the communication. > > - Bret > > *From: *Marvin H=E4user > *Sent: *Monday, June 28, 2021 8:43 AM > *To: *Laszlo Ersek ; Kun Qin > ; Kinney, Michael D > ; devel@edk2.groups.io > > *Cc: *Wang, Jian J ; Wu, Hao A > ; Dong, Eric ; > Ni, Ray ; Liming Gao > ; Liu, Zhiguang > ; Andrew Fish ; > Lindholm, Leif ; Bret Barkelew > ; Michael Kubacki > > *Subject: *[EXTERNAL] Re: [edk2-devel] [PATCH v1 0/5] EDK2 Code First: > PI Specification: Update EFI_MM_COMMUNICATE_HEADER > > On 28.06.21 16:57, Laszlo Ersek wrote: > > On 06/25/21 20:47, Kun Qin wrote: > >> Hi Mike, > >> > >> Thanks for the information. I can update the patch and proposed spec > >> change to use flexible array in v-next if there is no other concerns. > >> > >> After switching to flexible array, OFFSET_OF (Data) should lead to the > >> same result as sizeof. > > This may be true on specific compilers, but it is *not* guaranteed by > > the C language standard. > > Sorry, for completeness sake... :) > > I don't think it really depends on the compiler (but can vary per ABI), > but it's a conceptual issue with alignment requirements. Assuming > natural alignment and the usual stuff, for "struct s { uint64_t a; > uint32_t b; uint8_t c[]; }" the alignment requirement is 8 Bytes, where > there are 4 Bytes of padding after "b" (as "c" may as well be empty). > "c" however is guaranteed to start after b in the same fashion as if it > was declared with the maximum amount of elements that fit the memory. So > if we take 4 elements for "c", and note that "c" has an alignment > requirement of 1 Byte, c[0 .. 3] will alias the padding after "b". For > "sizeof" this means that the padding is included, whereas for "offsetof" > it is not, yielding "sizeof(struct s) =3D=3D offsetof(struct s, c) + 4". > That is what I meant by "wasted space" earlier, but this could possibly > be made nicer with macros as necessary. > > As for this specific struct, the values should be identical as it is > padded. > > Best regards, > Marvin > > > > > Quoting C99 6.7.2.1 "Structure and union specifiers", paragraph 16: > > > > "In most situations, the flexible array member is ignored. In > > particular, the size of the structure is as if the flexible array membe= r > > were omitted except that it may have more trailing padding than the > > omission would imply." > > > > Quoting footnotes 17 and 19, > > > > (17) [...] > > struct s { int n; double d[]; }; > > [...] > > > > (19) [...] > > the expressions: > > [...] > > sizeof (struct s) >=3D offsetof(struct s, d) > > > > are always equal to 1. > > > > Thanks > > Laszlo > > > > > > > >> While sizeof would be a preferred way to move > >> forward. > >> > >> Regards, > >> Kun > >> > >> On 06/24/2021 08:25, Kinney, Michael D wrote: > >>> Hello, > >>> > >>> Flexible array members are supported and should be used. The old > style > >>> of adding an array of size [1] at the end of a structure was used at = a > >>> time > >>> flexible array members were not supported by all compilers (late > 1990's). > >>> The workarounds used to handle the array of size [1] are very > >>> confusing when > >>> reading the C code and the fact that sizeof() does not produce the > >>> expected > >>> result make it even worse. > >>> > >>> If we use flexible array members in this proposed change then there i= s > >>> no need to use OFFSET_OF(). Correct? > >>> > >>> Mike > >>> > >>> > >>>> -----Original Message----- > >>>> From: Marvin H=E4user > >>>> Sent: Thursday, June 24, 2021 1:00 AM > >>>> To: Kun Qin ; Laszlo Ersek ; > >>>> devel@edk2.groups.io > >>>> Cc: Wang, Jian J ; Wu, Hao A > >>>> ; Dong, Eric ; Ni, Ray > >>>> ; Kinney, Michael D ; > >>>> Liming Gao ; Liu, Zhiguang > >>>> ; Andrew Fish ; Leif > >>>> Lindholm ; Bret Barkelew > >>>> ; michael.kubacki@microsoft.com > >>>> Subject: Re: [edk2-devel] [PATCH v1 0/5] EDK2 Code First: PI > >>>> Specification: Update EFI_MM_COMMUNICATE_HEADER > >>>> > >>>> Hey Kun, > >>>> > >>>> Why would you rely on undefined behaviours? The OFFSET_OF macro is > >>>> well-defined for GCC and Clang as it's implemented by an > intrinsic, and > >>>> while the expression for the MSVC compiler is undefined behaviour > as per > >>>> the C standard, it is well-defined for MSVC due to their own > >>>> implementation being identical. From my standpoint, all supported > >>>> compilers will yield well-defined behaviour even this way. > OFFSET_OF on > >>>> flexible arrays is not UB in any case to my knowledge. > >>>> > >>>> However, the same way as your new suggestion, you can replace > OFFSET_OF > >>>> with sizeof. While this *can* lead to wasted space with certain > >>>> structure layouts (e.g. when the flexible array overlays padding > bytes), > >>>> this is not the case here, and otherwise just loses you a few > bytes. I > >>>> think this comes down to preference. > >>>> > >>>> The pattern you mentioned arguably is less nice syntax when used > >>>> (involves address calculation and casting), but the biggest > problem here > >>>> is alignment constraints. For packed structures, you lose the > ability of > >>>> automatic unaligned accesses (irrelevant here because the > structure is > >>>> manually padded anyway). For non-packed structures, you still need t= o > >>>> ensure the alignment requirement of the trailing array data is met > >>>> manually. With flexible array members, the compiler takes care of > both > >>>> cases automatically. > >>>> > >>>> Best regards, > >>>> Marvin > >>>> > >>>> On 24.06.21 02:24, Kun Qin wrote: > >>>>> Hi Marvin, > >>>>> > >>>>> I would prefer not to rely on undefined behaviors from different > >>>>> compilers. Instead of using flexible arrays, is it better to remove > >>>>> the `Data` field, pack the structure and follow > >>>>> "VARIABLE_LOCK_ON_VAR_STATE_POLICY" pattern? > >>>>> > >>>>> In that case, OFFSET_OF will be forced to change to sizeof, and > >>>>> read/write to `Data` will follow the range indicated by > MessageLength. > >>>>> But yes, that will enforce developers to update their platform leve= l > >>>>> implementations accordingly. > >>>>> > >>>>> Regards, > >>>>> Kun > >>>>> > >>>>> On 06/23/2021 08:26, Laszlo Ersek wrote: > >>>>>> On 06/23/21 08:54, Marvin H=E4user wrote: > >>>>>>> On 22.06.21 17:34, Laszlo Ersek wrote: > >>>>>>>> On 06/18/21 11:37, Marvin H=E4user wrote: > >>>>>>>>> On 16.06.21 22:58, Kun Qin wrote: > >>>>>>>>>> On 06/16/2021 00:02, Marvin H=E4user wrote: > >>>>>>>>>>> 2) Is it feasible yet with the current set of supported > >>>>>>>>>>> compilers to > >>>>>>>>>>> support flexible arrays? > >>>>>>>>>> My impression is that flexible arrays are already supported (a= s > >>>>>>>>>> seen > >>>>>>>>>> in > UnitTestFrameworkPkg/PrivateInclude/UnitTestFrameworkTypes.h). > >>>>>>>>>> Please correct me if I am wrong. > >>>>>>>>>> > >>>>>>>>>> Would you mind letting me know why this is applicable here? > We are > >>>>>>>>>> trying to seek ideas on how to catch developer mistakes > caused by > >>>>>>>>>> this > >>>>>>>>>> change. So any input is appreciated. > >>>>>>>>> Huh, interesting. Last time I tried I was told about > >>>>>>>>> incompatibilities > >>>>>>>>> with MSVC, but I know some have been dropped since then > (2005 and > >>>>>>>>> 2008 > >>>>>>>>> if I recall correctly?), so that'd be great to allow globally. > >>>>>>>> I too am surprised to see > >>>>>>>> > "UnitTestFrameworkPkg/PrivateInclude/UnitTestFrameworkTypes.h". The > >>>>>>>> flexible array member is a C99 feature, and I didn't even know > >>>>>>>> that we > >>>>>>>> disallowed it for the sake of particular VS toolchains -- I > >>>>>>>> thought we > >>>>>>>> had a more general reason than just "not supported by VS > versions X > >>>>>>>> and Y". > >>>>>>>> > >>>>>>>> The behavior of OFFSET_OF() would be interesting -- the > OFFSET_OF() > >>>>>>>> macro definition for non-gcc / non-clang: > >>>>>>>> > >>>>>>>> #define OFFSET_OF(TYPE, Field) ((UINTN) &(((TYPE *)0)->Field)) > >>>>>>>> > >>>>>>>> borders on undefined behavior as far as I can tell, so its > >>>>>>>> behavior is > >>>>>>>> totally up to the compiler. It works thus far okay on Visual > >>>>>>>> Studio, but > >>>>>>>> I couldn't say if it extended correctly to flexible array > members. > >>>>>>> Yes, it's UB by the standard, but this is actually how MS > implements > >>>>>>> them (or used to anyway?). I don't see why it'd cause issues with > >>>>>>> flexible arrays, as only the start of the array is relevant > (which is > >>>>>>> constant for all instances of the structure no matter the > amount of > >>>>>>> elements actually stored). Any specific concern? If so, they > could be > >>>>>>> addressed by appropriate STATIC_ASSERTs. > >>>>>> No specific concern; my point was that two aspects of the same > "class" > >>>>>> of undefined behavior didn't need to be consistent with each other= . > >>>>>> > >>>>>> Thanks > >>>>>> Laszlo > >>>>>> > --_000_MW4PR21MB190775DD7144B23C262B13EAEF029MW4PR21MB1907namp_ Content-Type: text/html; charset="Windows-1252" Content-Transfer-Encoding: quoted-printable

Good note. Thanks!

 

- Bret

 

From: Marvin H=E4user
Sent: Tuesday, June 29, 2021 1:58 AM
To: Bret Barkelew= ; Laszlo Ersek; Kun Qin; Kinney, Michael D; devel@edk2.g= roups.io
Cc: Wang, Jian J; Wu, Hao A; Dong, Eric; Ni, Ray; Liming Gao; Liu, Zhiguang; Andrew Fish; Lindholm, Leif; Michae= l Kubacki
Subject: Re: [EXTERNAL] Re: [edk2-devel] [PATCH v1 0/5] EDK2 Code Fi= rst: PI Specification: Update EFI_MM_COMMUNICATE_HEADER

 

Generally yes, but gl= adly not for EDK II. Default GNU ABI uses 32-bit
alignment for 64-bit integers on IA32 (which led to a (non-critical)
mistake in our PE paper :( ) for example, but UEFI / EDK II (seem to)
successfully dictate natural alignment consistently. Possibly we could
introduce some STATIC_ASSERTs around important cases (e.g. UINT64 on
32-bit platforms) to ensure compilers keep it that way, once the ALIGNOF macro is introduced.

Best regards,
Marvin

On 29.06.21 08:49, Bret Barkelew wrote:
>
> The fact that it may vary per ABI seems like a pretty big gotcha if > the SMM/MM Core was compiled at a different time or on a different > system than the module that=92s invoking the communication.
>
> - Bret
>
> *From: *Marvin H=E4user <mail= to:mhaeuser@posteo.de>
> *Sent: *Monday, June 28, 2021 8:43 AM
> *To: *Laszlo Ersek <mailto:ler= sek@redhat.com>; Kun Qin
> <mailto:kuqin12@gmail.com&= gt;; Kinney, Michael D
> <mailto:michael.d.kin= ney@intel.com>; devel@edk2.groups.io
> <mailto:devel@edk2.groups.i= o>
> *Cc: *Wang, Jian J <mailto= :jian.j.wang@intel.com>; Wu, Hao A
> <mailto:hao.a.wu@intel.com>; Dong, Eric <mailto:eric.do= ng@intel.com>;
> Ni, Ray <mailto:ray.ni@intel.co= m>; Liming Gao
> <mailto:gaoliming@byoso= ft.com.cn>; Liu, Zhiguang
> <mailto:zhiguang.liu@inte= l.com>; Andrew Fish <mailto:af= ish@apple.com>;
> Lindholm, Leif <mailto:leif@nu= viainc.com>; Bret Barkelew
> <mailto:Bret.Barkele= w@microsoft.com>; Michael Kubacki
> <mailto:Michael.Ku= backi@microsoft.com>
> *Subject: *[EXTERNAL] Re: [edk2-devel] [PATCH v1 0/5] EDK2 Code First:=
> PI Specification: Update EFI_MM_COMMUNICATE_HEADER
>
> On 28.06.21 16:57, Laszlo Ersek wrote:
> > On 06/25/21 20:47, Kun Qin wrote:
> >> Hi Mike,
> >>
> >> Thanks for the information. I can update the patch and propos= ed spec
> >> change to use flexible array in v-next if there is no other c= oncerns.
> >>
> >> After switching to flexible array, OFFSET_OF (Data) should le= ad to the
> >> same result as sizeof.
> > This may be true on specific compilers, but it is *not* guarantee= d by
> > the C language standard.
>
> Sorry, for completeness sake... :)
>
> I don't think it really depends on the compiler (but can vary per ABI)= ,
> but it's a conceptual issue with alignment requirements. Assuming
> natural alignment and the usual stuff, for "struct s { uint64_t a= ;
> uint32_t b; uint8_t c[]; }" the alignment requirement is 8 Bytes,= where
> there are 4 Bytes of padding after "b" (as "c" may= as well be empty).
> "c" however is guaranteed to start after b in the same fashi= on as if it
> was declared with the maximum amount of elements that fit the memory. = So
> if we take 4 elements for "c", and note that "c" h= as an alignment
> requirement of 1 Byte, c[0 .. 3] will alias the padding after "b&= quot;. For
> "sizeof" this means that the padding is included, whereas fo= r "offsetof"
> it is not, yielding "sizeof(struct s) =3D=3D offsetof(struct s, c= ) + 4".
> That is what I meant by "wasted space" earlier, but this cou= ld possibly
> be made nicer with macros as necessary.
>
> As for this specific struct, the values should be identical as it is <= br> > padded.
>
> Best regards,
> Marvin
>
> >
> > Quoting C99 6.7.2.1 "Structure and union specifiers", p= aragraph 16:
> >
> > "In most situations, the flexible array member is ignored. I= n
> > particular, the size of the structure is as if the flexible array= member
> > were omitted except that it may have more trailing padding than t= he
> > omission would imply."
> >
> > Quoting footnotes 17 and 19,
> >
> > (17)  [...]
> >        struct s { int n; doubl= e d[]; };
> >        [...]
> >
> > (19)  [...]
> >        the expressions:
> >        [...]
> >        sizeof (struct s) >= =3D offsetof(struct s, d)
> >
> >        are always equal to 1.<= br> > >
> > Thanks
> > Laszlo
> >
> >
> >
> >> While sizeof would be a preferred way to move
> >> forward.
> >>
> >> Regards,
> >> Kun
> >>
> >> On 06/24/2021 08:25, Kinney, Michael D wrote:
> >>> Hello,
> >>>
> >>> Flexible array members are supported and should be used.&= nbsp; The old
> style
> >>> of adding an array of size [1] at the end of a structure = was used at a
> >>> time
> >>> flexible array members were not supported by all compiler= s (late
> 1990's).
> >>> The workarounds used to handle the array of size [1] are = very
> >>> confusing when
> >>> reading the C  code and the fact that sizeof() does = not produce the
> >>> expected
> >>> result make it even worse.
> >>>
> >>> If we use flexible array members in this proposed change = then there is
> >>> no need to use OFFSET_OF().  Correct?
> >>>
> >>> Mike
> >>>
> >>>
> >>>> -----Original Message-----
> >>>> From: Marvin H=E4user <mhaeuser@posteo.de>
> >>>> Sent: Thursday, June 24, 2021 1:00 AM
> >>>> To: Kun Qin <kuqin12@gmail.com>; Laszlo Ersek &= lt;lersek@redhat.com>;
> >>>> devel@edk2.groups.io
> >>>> Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, H= ao A
> >>>> <hao.a.wu@intel.com>; Dong, Eric <eric.dong@= intel.com>; Ni, Ray
> >>>> <ray.ni@intel.com>; Kinney, Michael D <micha= el.d.kinney@intel.com>;
> >>>> Liming Gao <gaoliming@byosoft.com.cn>; Liu, Zhi= guang
> >>>> <zhiguang.liu@intel.com>; Andrew Fish <afish= @apple.com>; Leif
> >>>> Lindholm <leif@nuviainc.com>; Bret Barkelew
> >>>> <Bret.Barkelew@microsoft.com>; michael.kubacki@= microsoft.com
> >>>> Subject: Re: [edk2-devel] [PATCH v1 0/5] EDK2 Code Fi= rst: PI
> >>>> Specification: Update EFI_MM_COMMUNICATE_HEADER
> >>>>
> >>>> Hey Kun,
> >>>>
> >>>> Why would you rely on undefined behaviours? The OFFSE= T_OF macro is
> >>>> well-defined for GCC and Clang as it's implemented by= an
> intrinsic, and
> >>>> while the expression for the MSVC compiler is undefin= ed behaviour
> as per
> >>>> the C standard, it is well-defined for MSVC due to th= eir own
> >>>> implementation being identical. From my standpoint, a= ll supported
> >>>> compilers will yield well-defined behaviour even this= way.
> OFFSET_OF on
> >>>> flexible arrays is not UB in any case to my knowledge= .
> >>>>
> >>>> However, the same way as your new suggestion, you can= replace
> OFFSET_OF
> >>>> with sizeof. While this *can* lead to wasted space wi= th certain
> >>>> structure layouts (e.g. when the flexible array overl= ays padding
> bytes),
> >>>> this is not the case here, and otherwise just loses y= ou a few
> bytes. I
> >>>> think this comes down to preference.
> >>>>
> >>>> The pattern you mentioned arguably is less nice synta= x when used
> >>>> (involves address calculation and casting), but the b= iggest
> problem here
> >>>> is alignment constraints. For packed structures, you = lose the
> ability of
> >>>> automatic unaligned accesses (irrelevant here because= the
> structure is
> >>>> manually padded anyway). For non-packed structures, y= ou still need to
> >>>> ensure the alignment requirement of the trailing arra= y data is met
> >>>> manually. With flexible array members, the compiler t= akes care of
> both
> >>>> cases automatically.
> >>>>
> >>>> Best regards,
> >>>> Marvin
> >>>>
> >>>> On 24.06.21 02:24, Kun Qin wrote:
> >>>>> Hi Marvin,
> >>>>>
> >>>>> I would prefer not to rely on undefined behaviors= from different
> >>>>> compilers. Instead of using flexible arrays, is i= t better to remove
> >>>>> the `Data` field, pack the structure and follow > >>>>> "VARIABLE_LOCK_ON_VAR_STATE_POLICY" pat= tern?
> >>>>>
> >>>>> In that case, OFFSET_OF will be forced to change = to sizeof, and
> >>>>> read/write to `Data` will follow the range indica= ted by
> MessageLength.
> >>>>> But yes, that will enforce developers to update t= heir platform level
> >>>>> implementations accordingly.
> >>>>>
> >>>>> Regards,
> >>>>> Kun
> >>>>>
> >>>>> On 06/23/2021 08:26, Laszlo Ersek wrote:
> >>>>>> On 06/23/21 08:54, Marvin H=E4user wrote:
> >>>>>>> On 22.06.21 17:34, Laszlo Ersek wrote: > >>>>>>>> On 06/18/21 11:37, Marvin H=E4user wr= ote:
> >>>>>>>>> On 16.06.21 22:58, Kun Qin wrote:=
> >>>>>>>>>> On 06/16/2021 00:02, Marvin H= =E4user wrote:
> >>>>>>>>>>> 2) Is it feasible yet wit= h the current set of supported
> >>>>>>>>>>> compilers to
> >>>>>>>>>>> support flexible arrays?<= br> > >>>>>>>>>> My impression is that flexibl= e arrays are already supported (as
> >>>>>>>>>> seen
> >>>>>>>>>> in
> UnitTestFrameworkPkg/PrivateInclude/UnitTestFrameworkTypes.h).
> >>>>>>>>>> Please correct me if I am wro= ng.
> >>>>>>>>>>
> >>>>>>>>>> Would you mind letting me kno= w why this is applicable here?
> We are
> >>>>>>>>>> trying to seek ideas on how t= o catch developer mistakes
> caused by
> >>>>>>>>>> this
> >>>>>>>>>> change. So any input is appre= ciated.
> >>>>>>>>> Huh, interesting. Last time I tri= ed I was told about
> >>>>>>>>> incompatibilities
> >>>>>>>>> with MSVC, but I know some have b= een dropped since then
> (2005 and
> >>>>>>>>> 2008
> >>>>>>>>> if I recall correctly?), so that'= d be great to allow globally.
> >>>>>>>> I too am surprised to see
> >>>>>>>>
> "UnitTestFrameworkPkg/PrivateInclude/UnitTestFrameworkTypes.h&quo= t;. The
> >>>>>>>> flexible array member is a C99 featur= e, and I didn't even know
> >>>>>>>> that we
> >>>>>>>> disallowed it for the sake of particu= lar VS toolchains -- I
> >>>>>>>> thought we
> >>>>>>>> had a more general reason than just &= quot;not supported by VS
> versions X
> >>>>>>>> and Y".
> >>>>>>>>
> >>>>>>>> The behavior of OFFSET_OF() would be = interesting -- the
> OFFSET_OF()
> >>>>>>>> macro definition for non-gcc / non-cl= ang:
> >>>>>>>>
> >>>>>>>> #define OFFSET_OF(TYPE, Field) ((UINT= N) &(((TYPE *)0)->Field))
> >>>>>>>>
> >>>>>>>> borders on undefined behavior as far = as I can tell, so its
> >>>>>>>> behavior is
> >>>>>>>> totally up to the compiler. It works = thus far okay on Visual
> >>>>>>>> Studio, but
> >>>>>>>> I couldn't say if it extended correct= ly to flexible array
> members.
> >>>>>>> Yes, it's UB by the standard, but this is= actually how MS
> implements
> >>>>>>> them (or used to anyway?). I don't see wh= y it'd cause issues with
> >>>>>>> flexible arrays, as only the start of the= array is relevant
> (which is
> >>>>>>> constant for all instances of the structu= re no matter the
> amount of
> >>>>>>> elements actually stored). Any specific c= oncern? If so, they
> could be
> >>>>>>> addressed by appropriate STATIC_ASSERTs.<= br> > >>>>>> No specific concern; my point was that two as= pects of the same
> "class"
> >>>>>> of undefined behavior didn't need to be consi= stent with each other.
> >>>>>>
> >>>>>> Thanks
> >>>>>> Laszlo
> >>>>>>
>

 

--_000_MW4PR21MB190775DD7144B23C262B13EAEF029MW4PR21MB1907namp_--