From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-pg1-f180.google.com (mail-pg1-f180.google.com [209.85.215.180]) by mx.groups.io with SMTP id smtpd.web08.2855.1625039801283203242 for ; Wed, 30 Jun 2021 00:56:41 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@gmail.com header.s=20161025 header.b=WEZ7CWwQ; spf=pass (domain: gmail.com, ip: 209.85.215.180, mailfrom: kuqin12@gmail.com) Received: by mail-pg1-f180.google.com with SMTP id d12so1438216pgd.9 for ; Wed, 30 Jun 2021 00:56:41 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=subject:to:cc:references:from:message-id:date:user-agent :mime-version:in-reply-to:content-language:content-transfer-encoding; bh=xZWmFVhBU+NEcCvKgzA0D/5TnZpFL6ShEOBHFacutKg=; b=WEZ7CWwQOFt5KuZJrtNcmwOkty9l8G+uIzM3jA4HN58WbTvwXn8g09xcRWYpQXatkI B3yg1dispmc2z/qHIvFhyTfj9Sc2gfz4AdscxzXK2gYADdfEBKir4fY+CERB70hb6MjF vq7Z/7xjDoWFPJtrzhpVjcEib6uEy1lf0WxQm1+/t7bH3q46jZje8xb2lYFgOQQGI3yM ohwCgpQetnsG7hp9yd29Hesb5ZhMkWzTQzNc9ceOHdzXz+Gu/dueS/Ri5ArvierGoH5k eybuxkD+cKry52PGcN4s/pkF6PgqRsW002jCytJklDEATfubx2UrVs3J8gUoBLxEM8aT E1kw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=xZWmFVhBU+NEcCvKgzA0D/5TnZpFL6ShEOBHFacutKg=; b=ugD3gRLu3WSP9haExCkxYiF4l2+pWtvhp959U/Diq92j55RTLMXr2AYJNtIwdo5JwB QNUSJ33FLQmhBgjsLneJjKRp6lTnF+oQ1GJnQzsvjtR1Gnl/8WLZ6Pa9aG1hEwASbfRx jJ0RcNfzyPV+9JSQp5NImge4AIebXANDS3S7/qkxdte17I1yJrSKIhQgy7zSFStiN53M q0iof1ONvZ60ytuGPN0fjdk2PvON5BW6bP8gPpTIDqv/ax+sGrnFvJodJVBo+1jXnwsp tW61XPfmmh9mBzy0sjJzimogVAMNdhCVKNGK7RqDS3kNX/oG5gVXEPSjEWj9KJogKBiN D23g== X-Gm-Message-State: AOAM530FDe7hLMwLH9ztriJ1YSCHNNbPPfnaY9wPzqKlFnM+ARTPdyWh 1SOA4n5SRVVvhRnYBylGu90= X-Google-Smtp-Source: ABdhPJxczpXjxupmlGXknCaX9ST5Lbm4Rk/s7P/pAXxo5ce9+d4V+ljDh5dY4TxVMHZLfoA0zUEUyA== X-Received: by 2002:a62:f947:0:b029:2e9:c502:7939 with SMTP id g7-20020a62f9470000b02902e9c5027939mr34376805pfm.34.1625039800805; Wed, 30 Jun 2021 00:56:40 -0700 (PDT) Return-Path: Received: from [192.168.50.18] ([50.35.88.161]) by smtp.gmail.com with ESMTPSA id 27sm17301880pgr.31.2021.06.30.00.56.39 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 30 Jun 2021 00:56:40 -0700 (PDT) Subject: Re: [EXTERNAL] Re: [edk2-devel] [PATCH v1 0/5] EDK2 Code First: PI Specification: Update EFI_MM_COMMUNICATE_HEADER To: "Kinney, Michael D" , "devel@edk2.groups.io" , "bret.barkelew@microsoft.com" , =?UTF-8?Q?Marvin_H=c3=a4user?= , Laszlo Ersek Cc: "Wang, Jian J" , "Wu, Hao A" , "Dong, Eric" , "Ni, Ray" , Liming Gao , "Liu, Zhiguang" , Andrew Fish , "Lindholm, Leif" , Michael Kubacki References: <20210610014259.1151-1-kuqin12@gmail.com> <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> <71554541-e28d-3d50-7ea9-03a0386fca30@gmail.com> From: "Kun Qin" Message-ID: Date: Wed, 30 Jun 2021 00:56:38 -0700 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Thunderbird/78.11.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit Thanks for the clarification. I will work on v-next with flexible array as Data field. Regards, Kun On 06/29/2021 18:07, Kinney, Michael D wrote: > If it breaks in the future, then that would be due to a new compiler that > or changes to the configuration of an existing compiler that break compatibility > with UEFI ABI. The compiler issue must be resolved before the new compiler > or change to existing compiler are accepted. > > Mike > >> -----Original Message----- >> From: Kun Qin >> Sent: Tuesday, June 29, 2021 4:11 PM >> To: Kinney, Michael D ; devel@edk2.groups.io; bret.barkelew@microsoft.com; Marvin Häuser >> ; Laszlo Ersek >> 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 >> >> Hi Mike, >> >> Thanks for the note. I will add this check for sanity check in v-next, >> assuming this will not fail for currently supported compilers. >> >> Just curious, what do we normally do if this type of check start to >> break in the future? >> >> Thanks, >> Kun >> >> On 06/29/2021 10:28, Kinney, Michael D wrote: >>> Good idea on use of STATIC_ASSERT(). >>> >>> For structures that use flexible array members, we can add a >>> STATIC_ASSERT() for the sizeof() and OFFSET_OF() returning the same result. >>> >>> For example: >>> >>> STATIC_ASSERT (sizeof (EFI_MM_COMMUNICATE_HEADER) == OFFSET_OF >>> (EFI_MM_COMMUNICATE_HEADER, Data)); >>> >>> Mike >>> >>> *From:*devel@edk2.groups.io *On Behalf Of *Bret >>> Barkelew via groups.io >>> *Sent:* Tuesday, June 29, 2021 9:00 AM >>> *To:* Marvin Häuser ; 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 >>> >>> Good note. Thanks! >>> >>> - Bret >>> >>> *From: *Marvin Häuser >>> *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, Leif ; 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’s invoking the communication. >>> > >>> > - Bret >>> > >>> > *From: *Marvin Häuser >> > >>> > *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) == 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) >= 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äuser > >>> > >>>> 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 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äuser wrote: >>> > >>>>>>> On 22.06.21 17:34, Laszlo Ersek wrote: >>> > >>>>>>>> On 06/18/21 11:37, Marvin Häuser wrote: >>> > >>>>>>>>> On 16.06.21 22:58, Kun Qin wrote: >>> > >>>>>>>>>> On 06/16/2021 00:02, Marvin Häuser 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 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 >>> > >>>>>> >>> > >>> >>> >>>