From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-pg1-f177.google.com (mail-pg1-f177.google.com [209.85.215.177]) by mx.groups.io with SMTP id smtpd.web11.552.1624646867776245675 for ; Fri, 25 Jun 2021 11:47:47 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@gmail.com header.s=20161025 header.b=Ufb311Oj; spf=pass (domain: gmail.com, ip: 209.85.215.177, mailfrom: kuqin12@gmail.com) Received: by mail-pg1-f177.google.com with SMTP id m2so8606431pgk.7 for ; Fri, 25 Jun 2021 11:47:47 -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=xZfzfn+jay0KyIMoNdVBEELikLtnjffkGy8S08UNaIg=; b=Ufb311OjYq9C50E+Tzl6MTwJ2wd4MX0HjUnmSY/rTWXiXiAN/YCwSMX5bYB7Yi2Xsa hIA+Hj35etXrhMabp+lFvTWZwtwUrucs88Nl1kai1i2HacZDF80haduwpDa1oCXjRuYl EhkDztwUjTBiLO7p5Kjf9xdueSUEe6vs9B2Qjb103CMYd1WRJtuU8RedAwdTqIfbE5gA YriMeFFuX0wL2BItyJS2eyibz5Ln4AY8Wa5D9RWQ453x23EKm/4NwmDZb5f2O+2GcmBT nlm1MmRDibnecA+FvCnOgO1I9IcLEx8zejOCKFoH4YtMzreDx8G88icGp3hqhqOK6xwZ wYVg== 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=xZfzfn+jay0KyIMoNdVBEELikLtnjffkGy8S08UNaIg=; b=aL8ii6Zt6psSJLCLoyunt8r6CIRbdSA5/gAaCFv5fS+GBHuAp+h0eJKv+ydxBL6F1P MiVoftwtj/xi6GDYsrsAZBZqXrZSvFv3CZkP1VH6q+wlO0ZUWBM+1QvqHIJhByy4+cft t5D8yIvLVPL/uDDGMAfsTbriyYQxcZxBP12NIyunJ4TLCkyGFDH8/m3B+6x4B0kDJXkY gdOGBFzMU0Gx1W90MkPTJIxOMFQ5M6sV2mtpH3drwLqdia91n2OPocyNBfy+YzdpktUo wbqAWx4OaZ7D8xXa+CU/pueZ/mgEfocG+mSFvjNh6ljgc04ragoug0ULfXUynC8cKBaq j5ig== X-Gm-Message-State: AOAM530Ak+QVS18H/FwHVWO+5yHrwgHq8gR/uNfWZ+qq+rRJAXwecbtG 8q/mEvAlf2fGdCzvHCHxnfA= X-Google-Smtp-Source: ABdhPJxB0s0Cr20pC9ha55ylRVlHXR/0Ekw4YN/X/8jIEUMp5zjGSCbxEPOqIllLWUpfzxzkkiiQ0w== X-Received: by 2002:aa7:9384:0:b029:2cc:5e38:933a with SMTP id t4-20020aa793840000b02902cc5e38933amr11764420pfe.81.1624646867291; Fri, 25 Jun 2021 11:47:47 -0700 (PDT) Return-Path: Received: from [192.168.50.18] ([50.35.88.161]) by smtp.gmail.com with ESMTPSA id j13sm6438588pfh.145.2021.06.25.11.47.46 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 25 Jun 2021 11:47:46 -0700 (PDT) Subject: Re: [edk2-devel] [PATCH v1 0/5] EDK2 Code First: PI Specification: Update EFI_MM_COMMUNICATE_HEADER To: "Kinney, Michael D" , =?UTF-8?Q?Marvin_H=c3=a4user?= , Laszlo Ersek , "devel@edk2.groups.io" Cc: "Wang, Jian J" , "Wu, Hao A" , "Dong, Eric" , "Ni, Ray" , Liming Gao , "Liu, Zhiguang" , Andrew Fish , Leif Lindholm , Bret Barkelew , "michael.kubacki@microsoft.com" 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> From: "Kun Qin" Message-ID: <0c931dbd-8a8d-fd61-b4ad-bd5ff16812d9@gmail.com> Date: Fri, 25 Jun 2021 11:47:46 -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 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. 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 >>>> >