From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from outbound.mail.eo.outlook.com (outbound.mail.eo.outlook.com [40.93.8.9]) by mx.groups.io with SMTP id smtpd.web09.4761.1624949404260808394 for ; Mon, 28 Jun 2021 23:50:04 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@microsoft.com header.s=selector2 header.b=HBa0hI0E; spf=pass (domain: microsoft.com, ip: 40.93.8.9, mailfrom: bret.barkelew@microsoft.com) ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=R1yJtI2fSsFdgpLjgHvrMbOq/0vkUN2ceghOg/J/S6W8f5TlqsUaut2JIsNlWAZXxzwSnS16NbnmSPbRttjrL0GfK+d6ZqyHeL87Qjw43EcypIdavlhav261OyVbTEHbr1sXRYbi+TXAen4LnJ3m8YPdYxXTogXB2QPOvSyuIXkmFj+nOi+x5dvl45mGHMYObeNnXElsqpqQ/+/zAs17paYdRQTEVgpFdP81mplITPq26OpKdwLlfkokn6HYXfVf7QPE+f1ZCqH6WUEduUl0+WqPdODVYOPfw4P4H2azaMD8FFrvYzRhC9tbEsQbWgw7Nq7BqtIqmN71lKjjcnuzTw== 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=rzN4oaCXOhojhfDjzT1PUL7qztKDwjAQiETESMdnuV4=; b=ApUpa6GaxUfy6wbssdZtDUocOSC/zMMTeVfqi9RdQIs6r2z2WhIWEcc/8MJ7TSZZLtepkMl1IkGhdhZ7YSj86PhG0Fio38otovK19PQE9aSczkws+0bLJZggF7F721Tw8cUifO0GG2MF/lDfrhEWz22fP5+SIyk1W/SWLZIkKGnfZPHOzXVnSQZCsk5isBJDSkhLhJrxDmsALLUEQgICfQgofqhayk+ihNIWwk97yXKaTJE0pY5beIvNJBK2FkN5CUz54KcOUHsz51XHmsXedlgKPOURouhy9kL/NgCTRU+c63qIS5cLKsCyINVdsEcusp02Qhq0vfZ6n6qhG0yB5Q== 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=rzN4oaCXOhojhfDjzT1PUL7qztKDwjAQiETESMdnuV4=; b=HBa0hI0EBYLleeB16ZWvI+2o/QZTYgoryHHVnvDQE22j/4qRGIJ8TRzE1W3tMnMZa8yem3dGArKwPUxh5+aebVOJChDzqRqcAZ1NIVkDH6lTjnaoIbX3Sw4+B2/ni8Ta5towjJBMb7/YZeNZe2n86VDjC3rOovUN81tduAHWKNY= Received: from MW4PR21MB1907.namprd21.prod.outlook.com (2603:10b6:303:71::8) by MW4PR21MB2059.namprd21.prod.outlook.com (2603:10b6:303:11e::19) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.4308.2; Tue, 29 Jun 2021 06:50:00 +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 06:50:00 +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/RRv Date: Tue, 29 Jun 2021 06:49:59 +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> In-Reply-To: <5dd6ce4c-a3ba-8d56-dce9-0e699c919c5b@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-29T06:49:06.4313726Z;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: 4df528ca-35e5-4bff-d864-08d93aca1d5e x-ms-traffictypediagnostic: MW4PR21MB2059: 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: Pae7Z6pKD2Tv4QEbp7R0Mo1ZF4IbdwNztq93NvS7kdoTSETcFacWlRZob/0Wkejd1OgnDwG07y8OdfEE55iP4zeMF/kzoHHz/M421cvddrleDoeqYKUv/7JyJM+u+7LrbEgDu6oYVb782gYdbegq9PpSWJ4BszKVukkDv5+aeZBMB4SODCsuTNfz7pelMt4GC9rSQRO1ZdnBmGJOkRtgrqZzilhubocsxSTlDn0e4eMDwCBycb9Hf8xtW+bvZJxeXBERy+sUR7ZL/1Zjr2EUVfjnNbwB7CatRuyZXmDRGpCWu/rqSkhZEbSueyfrxUwSq9D8RiaE/26XbOXncR/FCj2/ROy6/IPYNs/gqe5a8uBjLw5A0eJXqAODxZ41+ZeTEj2s0MMRnb+owQD05CPae46OTUCW6w+FJ1w7PA0djtiOo658AUe4kPAgpOSuXazU77YAuoo1WCMP5VTDfmkso2/coZZS2jldEP6uXpJk3QGG66P/BK7Aa/PbMsc2sEXFobDgi+drSRUk1XXln2TISh9VPAY9ZGhKpLgbWkaZ9d87jF2FJJD37W/zQ+jWPk7m0QAzPcd5nO2VYMkGLwlTz1OaOtyIoTfh9+80sOY78GL0W5jzueG4hpUt50OkmSoRSyruS3keEWQrEk9ZkTqVLQ== 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)(2906002)(71200400001)(53546011)(6506007)(66574015)(38100700002)(83380400001)(10290500003)(478600001)(66446008)(66556008)(55016002)(66946007)(66476007)(76116006)(9686003)(26005)(5660300002)(186003)(122000001)(52536014)(64756008)(8676002)(107886003)(4326008)(8990500004)(7696005)(316002)(33656002)(86362001)(110136005)(54906003)(15650500001)(82950400001)(82960400001)(8936002)(7416002);DIR:OUT;SFP:1102; x-ms-exchange-antispam-messagedata-chunkcount: 1 x-ms-exchange-antispam-messagedata-0: =?Windows-1252?Q?+1Q8ODiwo5zEglDy8A09/rOs/wr+l2xzyPupIEJou/LJjdDQmp9ZvWZS?= =?Windows-1252?Q?K8kdZ8cs1VOtXljAEC47+eqng+QwIsD5DdKvzOaWFr/8Rj4CWrTRsyvD?= =?Windows-1252?Q?+OUUeDl5Dsp6jkDPo4CYqKDlYd5mf1ow7c6WnsUxj7oWzW4lpZBcyC+t?= =?Windows-1252?Q?FJdq4nLhq0OWhOPa8ss0GSJfJyc2IqxLxBpApzoOT2+Mx+RcBYNh9V0F?= =?Windows-1252?Q?DZ8fWSSlAaBb20qSIEoxKafBmy8yGpjcwiv6FWf2WQlHrDzNdsH6Hv68?= =?Windows-1252?Q?ZKGnmdIBwOY9N4WtfBHCzD4K2GeCKeK3vt2zRv0ptl6P5i82rrp+BahV?= =?Windows-1252?Q?m0R41mV3SodYpO++6yNebAV6PjGiGrN++E1Xx+/lNOyp+DLtEmkq4TcU?= =?Windows-1252?Q?rVsp8AT/LPCANXWoD2euvTUVVhJrWfmlngpsc2L0t1XOLvau6fUmx0mb?= =?Windows-1252?Q?BWEZHuB8d3bgUtzyJsjNqzrAzkliIygkDDghVw4U9uD5wdibL9s/Hhpb?= =?Windows-1252?Q?Y8YXz5Q5p9mDdQAMgdiHofdCm9qzw3wiFSWOxZcEFk65SfraoBgG0wHk?= =?Windows-1252?Q?wwXUjuzdOpnsYYM/1wbHdwwfjRgeSWlAsTOUrBoJ3WaueVjznf7QcdOz?= =?Windows-1252?Q?aEdG1+O8Pamy4m9DPzi7prtc5j4se72vOAEEWAGOvAY+BTMxTPn7GW63?= =?Windows-1252?Q?fXfu8KNeyy5tbtDkI+tHQu4UxnI4yKU5sB5z9owby+ya51PjOsBhlXLV?= =?Windows-1252?Q?12+dvejo7IejbUGBB8zGm6TX/IjRLEJBuIYQuWVHgd7PQlATqdm55V9h?= =?Windows-1252?Q?sp7N3FkHf2sfJIjcNctCGKbuHou1atJoCQvICeC1Q2rU+pBPlj1tNs2s?= =?Windows-1252?Q?e/5ZrpgooyhISrhLAIrQyoPNSOSRkY09V/gFtDWbhwwxUpKurojlVZeo?= =?Windows-1252?Q?KIycwN/zItThP4URGGbJmq5crK6mAG8BDMo7FV+siku/UTAvHJ5mVqmM?= =?Windows-1252?Q?nVUtyNUWtk5K//Y9vR4zdOx32zPIS8oLYvP3OEU5B8P82RtAAY5UT1H8?= =?Windows-1252?Q?sesDz66ft80wPPP322fpKmZ+0emg4qBUI144P4U2TDKm2SkNEakSrdBs?= =?Windows-1252?Q?iIw5XCzm5ADRPoOiAgRKdOSv7kQh3NVZBxNhucyGvh6PyGUWRc2K/1Xo?= =?Windows-1252?Q?FDHccReRoH1ah61kvAwq9xzgoYyGd1+u1GSLl6LKCHBIElMSWq27cbo3?= =?Windows-1252?Q?uD9hOqAHavmwpVEOPYfqGKF6kAzhRQWgi5KsbL7SEQXHFkud/OgMuRn+?= =?Windows-1252?Q?eqoMMNJyzdQYymCUqgvpSoIwAyNkkZWxPtM1mpjY+jb+5zXwOOCT8UrL?= =?Windows-1252?Q?S9MqdZxolVqBgsTFNSK771RMJ5J59HHFirxgaEW24lkDY0zbZLY1+kj3?= =?Windows-1252?Q?qPHaebiEofn9CMsMj/Z9ww=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: 4df528ca-35e5-4bff-d864-08d93aca1d5e X-MS-Exchange-CrossTenant-originalarrivaltime: 29 Jun 2021 06:49:59.8483 (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: XdmsxnCX2GU6SK8IXWG3cdGCzmFkbwRlSMHQu1S5XNSm+k0WGphEYUSv3KKoKbcrBc3zQZNMbUiFoQFSMojx/Q== X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW4PR21MB2059 Content-Language: en-US Content-Type: multipart/alternative; boundary="_000_MW4PR21MB19078F47FAFEC52CF4281DECEF029MW4PR21MB1907namp_" --_000_MW4PR21MB19078F47FAFEC52CF4281DECEF029MW4PR21MB1907namp_ Content-Type: text/plain; charset="Windows-1252" Content-Transfer-Encoding: quoted-printable 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, Le= if; Bret Barkelew; Michael Kubacki Subject: [EXTERNAL] Re: [edk2-devel] [PATCH v1 0/5] EDK2 Code First: PI Spe= cification: 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 member > 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 is >>> 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, an= d >>>> while the expression for the MSVC compiler is undefined behaviour as p= er >>>> 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 o= n >>>> flexible arrays is not UB in any case to my knowledge. >>>> >>>> However, the same way as your new suggestion, you can replace OFFSET_O= F >>>> 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 he= re >>>> 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 to >>>> 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 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 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 (as >>>>>>>>>> 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 a= re >>>>>>>>>> trying to seek ideas on how to catch developer mistakes caused b= y >>>>>>>>>> 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". Th= e >>>>>>>> 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 implement= s >>>>>>> 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 "clas= s" >>>>>> of undefined behavior didn't need to be consistent with each other. >>>>>> >>>>>> Thanks >>>>>> Laszlo >>>>>> --_000_MW4PR21MB19078F47FAFEC52CF4281DECEF029MW4PR21MB1907namp_ Content-Type: text/html; charset="Windows-1252" Content-Transfer-Encoding: quoted-printable

The fact that it may vary per ABI seems like a prett= y big gotcha if the SMM/MM Core was compiled at a different time or on a di= fferent 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 Bar= kelew; 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, La= szlo Ersek wrote:
> On 06/25/21 20:47, Kun Qin wrote:
>> Hi Mike,
>>
>> Thanks for the information. I can update the patch and proposed sp= ec
>> change to use flexible array in v-next if there is no other concer= ns.
>>
>> 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<= br> > 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, wher= e
there are 4 Bytes of padding after "b" (as "c" may as w= ell 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 &qu= ot;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 po= ssibly
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", paragr= aph 16:
>
> "In most situations, the flexible array member is ignored. In
> particular, the size of the structure is as if the flexible array memb= er
> 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 of= fsetof(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 u= sed at a
>>> time
>>> flexible array members were not supported by all compilers (la= te 1990's).
>>> The workarounds used to handle the array of size [1] are very<= br> >>> confusing when
>>> reading the C  code and the fact that sizeof() does not p= roduce 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 <le= rsek@redhat.com>;
>>>> devel@edk2.groups.io
>>>> Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A<= br> >>>> <hao.a.wu@intel.com>; Dong, Eric <eric.dong@intel= .com>; Ni, Ray
>>>> <ray.ni@intel.com>; Kinney, Michael D <michael.d.= kinney@intel.com>;
>>>> Liming Gao <gaoliming@byosoft.com.cn>; Liu, Zhiguang=
>>>> <zhiguang.liu@intel.com>; Andrew Fish <afish@appl= e.com>; Leif
>>>> Lindholm <leif@nuviainc.com>; Bret Barkelew
>>>> <Bret.Barkelew@microsoft.com>; michael.kubacki@micro= soft.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 i= ntrinsic, and
>>>> while the expression for the MSVC compiler is undefined be= haviour as per
>>>> the C standard, it is well-defined for MSVC due to their o= wn
>>>> implementation being identical. From my standpoint, all su= pported
>>>> 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 repl= ace OFFSET_OF
>>>> with sizeof. While this *can* lead to wasted space with ce= rtain
>>>> structure layouts (e.g. when the flexible array overlays p= adding 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 whe= n used
>>>> (involves address calculation and casting), but the bigges= t 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 st= ill need to
>>>> ensure the alignment requirement of the trailing array dat= a 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 bet= ter 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 si= zeof, and
>>>>> read/write to `Data` will follow the range indicated b= y MessageLength.
>>>>> But yes, that will enforce developers to update their = 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 wrote:<= br> >>>>>>>>> On 16.06.21 22:58, Kun Qin wrote:
>>>>>>>>>> On 06/16/2021 00:02, Marvin H=E4us= er wrote:
>>>>>>>>>>> 2) Is it feasible yet with the= current set of supported
>>>>>>>>>>> compilers to
>>>>>>>>>>> support flexible arrays?
>>>>>>>>>> My impression is that flexible arr= ays are already supported (as
>>>>>>>>>> seen
>>>>>>>>>> in UnitTestFrameworkPkg/PrivateInc= lude/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 cat= ch developer mistakes caused by
>>>>>>>>>> this
>>>>>>>>>> change. So any input is appreciate= d.
>>>>>>>>> Huh, interesting. Last time I tried I = was told about
>>>>>>>>> incompatibilities
>>>>>>>>> with MSVC, but I know some have been d= ropped 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, an= d I didn't even know
>>>>>>>> that we
>>>>>>>> disallowed it for the sake of particular V= S 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 inter= esting -- the OFFSET_OF()
>>>>>>>> macro definition for non-gcc / non-clang:<= br> >>>>>>>>
>>>>>>>> #define OFFSET_OF(TYPE, Field) ((UINTN) &a= mp;(((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 actu= ally 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 arra= y is relevant (which is
>>>>>>> constant for all instances of the structure no= matter the amount of
>>>>>>> elements actually stored). Any specific concer= n? 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_MW4PR21MB19078F47FAFEC52CF4281DECEF029MW4PR21MB1907namp_--