From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail.ispras.ru (mail.ispras.ru [83.149.199.45]) by mx.groups.io with SMTP id smtpd.web12.1375.1589395043687077163 for ; Wed, 13 May 2020 11:37:24 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: ispras.ru, ip: 83.149.199.45, mailfrom: cheptsov@ispras.ru) Received: from [127.0.0.1] (unknown [77.232.9.83]) by mail.ispras.ru (Postfix) with ESMTPSA id BDC81CD461; Wed, 13 May 2020 21:37:19 +0300 (MSK) From: "Vitaly Cheptsov" Message-Id: <8EA54782-5A1C-4767-A0C1-0837D9A60248@ispras.ru> Mime-Version: 1.0 (Mac OS X Mail 13.4 \(3608.80.23.2.2\)) Subject: Re: [edk2-devel] [PATCH V4 00/27] Disabling safe string constraint assertions Date: Wed, 13 May 2020 21:37:18 +0300 In-Reply-To: Cc: "Kinney, Michael D" , "lersek@redhat.com" , "devel@edk2.groups.io" , Andrew Fish , =?utf-8?Q?Marvin_H=C3=A4user?= , "Gao, Zhichao" To: "Gao, Liming" References: <20200511154121.3878-1-cheptsov@ispras.ru> <44ac1ca1-953a-21a2-0c9e-c83aca153b0b@redhat.com> <09ED0896-A19B-4710-A92A-46B678BF94D4@ispras.ru> X-Mailer: Apple Mail (2.3608.80.23.2.2) X-Groupsio-MsgNum: 59449 Content-Type: multipart/signed; boundary="Apple-Mail=_FBD8AD63-2DE1-476E-90A5-60557DAE9BEC"; protocol="application/pgp-signature"; micalg=pgp-sha512 --Apple-Mail=_FBD8AD63-2DE1-476E-90A5-60557DAE9BEC Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 Liming, Firstly, could you please explain why did not you suggest anything alike a= couple of months ago when I proposed exactly the same solution and posted = a patch asking how to workaround the undeclared identifier issue? I am somewhat unhappy here doing rounds. I propose a solution, somebody fr= om your team says it is wrong, I propose another solution, you say it is wr= ong again, months later you start suggesting me to use one of my older solu= tions but =C2=ABslightly=C2=BB change it, so that it starts to look ugly (a= nd I do not mean just this time, it has been at least trice). If your team = wants to continue to sabotage any attempts to fix it we could consider ceas= ing any relationship with EDK II, but I want to believe it is just some mis= understanding which unfortunately escalated this much. Secondly, =C2=ABbeing compatible to the current platform=C2=BB and leaving= on by default is something that nobody really needs. I explicitly mentione= d that the defaults are changed in the latest commit message, and so far no= body objects about it. My opinion is that the ill behaviour we have now sho= uld never ever be the default. Be that compatibility or not. If you need it= for some packages =E2=80=94 override it. In fact, Also, in fact I once mad= e the PCD inverted a few revisions ago=E2=80=A6 and somebody asked to chang= e it :D Thirdly, I am not happy about code duplication in the debug lib we have no= w. This makes changing anything in DebugLib a very hard matter. Just like L= aszlo says, if DebugLib is frozen forever, we should not even attempt to ch= ange things there. It sounds insane if true. All in all, several people including us, guys from Microsoft and Red Hat h= ave already told you many times that the reasons behind keeping this assert= ion are illicit in the first place. Perhaps, it should really be just remov= ed in a single small patch without any PCDs and we can just get over it onc= e and forever. If you believe it is not the case, perhaps you could submit = a patch on your own? We will review it as soon as possible. Best wishes, Vitaly [1] https://edk2.groups.io/g/devel/message/55961 > 13 =D0=BC=D0=B0=D1=8F 2020 =D0=B3., =D0=B2 20:59, Gao, Liming =D0=BD=D0=B0=D0=BF=D0=B8=D1=81=D0=B0=D0=BB(=D0=B0): >=20 > Vitaly: > I would like to propose one solution to update DebugLib.h only. This so= lution directly declares PcdDebugPropertyMask global variable in DebugLib.h= . Then, DebugLib macro can use it. Because most of DebugLib library instanc= es describe PcdDebugPropertyMask in their INF, this PCD can be generated wi= thout the additional change. I also evaluate PcdDebugPropertyMask usage in = existing platform. This PCD is always used as FixedAtBuild. So, I change Pc= dDebugPropertyMask type in DEC to FixedAtBuild. This change impact should b= e smaller than current patch set. Below patch passes the build on OvmfPkg. = It should work. >=20 > Besides, new PcdDebugPropertyMask BIT6 for Treat constraint violations = as ASSERT. This BIT should be enabled by default to be compatible with curr= ent platform. Now, most platforms set this PCD PcdDebugPropertyMask in thei= r platform DSC file. PCD value is also required to be updated. Another comp= atible way is to define BIT6 for Treat constraint violations as empty. When= BIT6 is 1, it means constraint violation as empty. When BIT6 is 0, it mean= s constraint violation as ASSERT. If so, this PCD value is not required to = be changed in platform DSC. >=20 > Thanks > Liming > --- > MdePkg/Include/Library/DebugLib.h | 7 ++++++- > .../Library/BaseDebugLibNull/BaseDebugLibNull.inf | 4 ++++ > MdePkg/MdePkg.dec | 22 +++++++++++-----= ------ > 3 files changed, 21 insertions(+), 12 deletions(-) >=20 > diff --git a/MdePkg/Include/Library/DebugLib.h b/MdePkg/Include/Library/= DebugLib.h > index baab34bf05..68604de869 100644 > --- a/MdePkg/Include/Library/DebugLib.h > +++ b/MdePkg/Include/Library/DebugLib.h > @@ -321,6 +321,11 @@ DebugPrintLevelEnabled ( > #define _DEBUG(Expression) DebugPrint Expression > #endif >=20 > +// > +// FixedAtBuild PCD value PcdDebugPropertyMask > +// > +extern const UINT8 _gPcd_FixedAtBuild_PcdDebugPropertyMask; > + > /** > Macro that calls DebugAssert() if an expression evaluates to FALSE. >=20 > @@ -336,7 +341,7 @@ DebugPrintLevelEnabled ( > #if !defined(MDEPKG_NDEBUG) > #define ASSERT(Expression) \ > do { \ > - if (DebugAssertEnabled ()) { \ > + if ((DEBUG_PROPERTY_DEBUG_ASSERT_ENABLED & _gPcd_FixedAtBuild_Pcd= DebugPropertyMask) !=3D 0) { \ > if (!(Expression)) { \ > _ASSERT (Expression); \ > ANALYZER_UNREACHABLE (); \ > diff --git a/MdePkg/Library/BaseDebugLibNull/BaseDebugLibNull.inf b/MdeP= kg/Library/BaseDebugLibNull/BaseDebugLibNull.inf > index 81a63a5074..1e95a2f077 100644 > --- a/MdePkg/Library/BaseDebugLibNull/BaseDebugLibNull.inf > +++ b/MdePkg/Library/BaseDebugLibNull/BaseDebugLibNull.inf > @@ -29,3 +29,7 @@ > [Packages] > MdePkg/MdePkg.dec >=20 > +[Pcd] > + # This PCD is consumed in DebugLib.h > + gEfiMdePkgTokenSpaceGuid.PcdDebugPropertyMask ## CONSUMES > + > diff --git a/MdePkg/MdePkg.dec b/MdePkg/MdePkg.dec > index 0ab09195c1..f8aacdf428 100644 > --- a/MdePkg/MdePkg.dec > +++ b/MdePkg/MdePkg.dec > @@ -2105,6 +2105,17 @@ > # @Prompt Speculation Barrier Type. > gEfiMdePkgTokenSpaceGuid.PcdSpeculationBarrierType|0x01|UINT8|0x300010= 18 >=20 > + ## The mask is used to control DebugLib behavior.

> + # BIT0 - Enable Debug Assert.
> + # BIT1 - Enable Debug Print.
> + # BIT2 - Enable Debug Code.
> + # BIT3 - Enable Clear Memory.
> + # BIT4 - Enable BreakPoint as ASSERT.
> + # BIT5 - Enable DeadLoop as ASSERT.
> + # @Prompt Debug Property. > + # @Expression 0x80000002 | (gEfiMdePkgTokenSpaceGuid.PcdDebugPropert= yMask & 0xC0) =3D=3D 0 > + gEfiMdePkgTokenSpaceGuid.PcdDebugPropertyMask|0|UINT8|0x00000005 > + > [PcdsFixedAtBuild,PcdsPatchableInModule] > ## Indicates the maximum length of unicode string used in the followin= g > # BaseLib functions: StrLen(), StrSize(), StrCmp(), StrnCmp(), StrCpy= (), StrnCpy()

> @@ -2139,17 +2150,6 @@ > # @Prompt Spin Lock Timeout (us). > gEfiMdePkgTokenSpaceGuid.PcdSpinLockTimeout|10000000|UINT32|0x00000004 >=20 > - ## The mask is used to control DebugLib behavior.

> - # BIT0 - Enable Debug Assert.
> - # BIT1 - Enable Debug Print.
> - # BIT2 - Enable Debug Code.
> - # BIT3 - Enable Clear Memory.
> - # BIT4 - Enable BreakPoint as ASSERT.
> - # BIT5 - Enable DeadLoop as ASSERT.
> - # @Prompt Debug Property. > - # @Expression 0x80000002 | (gEfiMdePkgTokenSpaceGuid.PcdDebugPropert= yMask & 0xC0) =3D=3D 0 > - gEfiMdePkgTokenSpaceGuid.PcdDebugPropertyMask|0|UINT8|0x00000005 > - > ## This flag is used to control the print out Debug message.

> # BIT0 - Initialization message.
> # BIT1 - Warning message.
> -- >=20 > Thanks > Liming >> -----Original Message----- >> From: Vitaly Cheptsov >> Sent: Wednesday, May 13, 2020 2:58 AM >> To: Kinney, Michael D ; lersek@redhat.com >> Cc: devel@edk2.groups.io; Andrew Fish ; Marvin H=C3=A4= user ; Gao, Liming >> ; Gao, Zhichao >> Subject: Re: [edk2-devel] [PATCH V4 00/27] Disabling safe string constr= aint assertions >>=20 >> Mike, >>=20 >> I see what you mean here, but I believe it is absolutely fine to implem= ent DebugCommonLib.h interface within DebugLib without >> depending on DebugCommonLib if one absolutely desires it (thought I do = not think it is a good idea). >> In this case, perhaps we can avoid adding DebugCommonLib.h header at al= l and leave it all in DebugLib.h? >> So that DebugCommonLib library is entirely private thing to DebugLib, w= hich can either use it or not. >>=20 >> Best wishes, >> Vitaly >>=20 >>> 12 =D0=BC=D0=B0=D1=8F 2020 =D0=B3., =D0=B2 21:18, Kinney, Michael D =D0=BD=D0=B0=D0=BF=D0=B8=D1=81=D0=B0=D0=BB(=D0= =B0): >>>=20 >>> Laszlo, >>>=20 >>> A library implementation that uses another library class >>> lists those library classes in the library INF. This is >>> a way a module can inherit the use of a second lib without >>> listing the second lib in the module INF. This is the >>> type of inheritance that is supported by the EDK II build >>> system and the EDK II meta data files. >>>=20 >>> What is not supported today is indirect inheritance from >>> the libclass .h file itself. There is no mechanism for >>> the libclass .h file to declare it is using another library >>> class. This would require extensions to the meta data >>> files to declare this type dependency. >>>=20 >>> How does the build system know to add DebugCommonLib to >>> the link command for a module that only lists DebugLib >>> in its library classes section? What if there is a >>> DebugLib implementation of the DebugLib class that >>> does not depend on DebugCommonLib. The module link >>> will then fail when the module writer followed all the >>> rules. This is why including a libclass from another >>> libclass is bad idea. >>>=20 >>> Mike >>>=20 >>>> -----Original Message----- >>>> From: devel@edk2.groups.io On >>>> Behalf Of Laszlo Ersek >>>> Sent: Tuesday, May 12, 2020 2:51 AM >>>> To: Kinney, Michael D ; >>>> Vitaly Cheptsov ; >>>> devel@edk2.groups.io >>>> Cc: Andrew Fish ; Marvin H=C3=A4user >>>> ; Gao, Liming >>>> ; Gao, Zhichao >>>> >>>> Subject: Re: [edk2-devel] [PATCH V4 00/27] Disabling >>>> safe string constraint assertions >>>>=20 >>>> On 05/12/20 00:40, Kinney, Michael D wrote: >>>>> Vitaly, >>>>>=20 >>>>> Thank you for the contribution. >>>>>=20 >>>>> There are a couple points about this approach that >>>> need to be discussed. >>>>>=20 >>>>> You have included the from >>>>> MdePkg/Include/Library/DebugLib.h. >>>>=20 >>>> Right, I've noticed it. I agree it's unusual. I didn't >>>> think it was wrong. >>>>=20 >>>>> It is very rare for a >>>>> lib class to include another lib class. This means >>>> that a module >>>>> that has a dependency on the DebugLib class inherits >>>> a hidden >>>>> dependency on the DebugCommonLib class. >>>>=20 >>>> I agree. >>>>=20 >>>> I think it should be fine. Any header H1 should include >>>> such further >>>> headers H2, H3, ... Hn that are required for making the >>>> interfaces >>>> declared in H1 usable in client modules. >>>>=20 >>>>> For module INF files, >>>>> we require the INF file to list all the lib classes >>>> that the >>>>> module sources directly use. >>>>=20 >>>> Yes, keyword being "directly". >>>>=20 >>>>> Since a module that uses the >>>>> DebugLib uses the ASSERT() and DEBUG() macros, all >>>> the APIs >>>>> that the ASSERT() and DEBUG() macros use are also >>>> directly >>>>> used by the module. >>>>=20 >>>> I believe this is where I disagree. The replacement >>>> texts of the >>>> ASSERT() and DEBUG() function-like macros are internals >>>> of the >>>> DebugLib.h lib class header, in my opinion. Those >>>> internals place >>>> requirements on specific DebugLib instances, not on >>>> DebugLib class >>>> consumers. >>>>=20 >>>> In other words, when writing a new DebugLib instance, >>>> the implementor >>>> has to ensure that the ASSERT() and DEBUG() macros, as >>>> defined in the >>>> DebugLib class header, will continue working in >>>> DebugLib consumer >>>> modules. The implementor may then choose to make the >>>> new DebugLib >>>> instance dependent on the (singleton) DebugCommonLib >>>> instance, for >>>> example. (This is being done in patches #3, #4, #16, >>>> maybe more.) The >>>> DebugLib consumer module will inherit that dependency, >>>> and everything >>>> will work. >>>>=20 >>>> Just because ASSERT() and DEBUG() are function-like >>>> macros and not >>>> actual functions, I don't think the INF file >>>> requirements in >>>> DebugLib-consumer modules should change. >>>>=20 >>>>> With this patch series, these macros >>>>> now use the DebugCommonLib class APIs, which means >>>> any module >>>>> that uses the DebugLib also directly uses the >>>> DebugCommonLib. >>>>=20 >>>> In my opinion: indirectly. >>>>=20 >>>> Thanks, >>>> Laszlo >>>>=20 >>>>=20 >>>>=20 >>>=20 >=20 --Apple-Mail=_FBD8AD63-2DE1-476E-90A5-60557DAE9BEC Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename=signature.asc Content-Type: application/pgp-signature; name=signature.asc Content-Description: Message signed with OpenPGP -----BEGIN PGP SIGNATURE----- iQIzBAEBCgAdFiEEsLABAI5Y5VbvBdmpL8K2O86Eyz4FAl68Pl4ACgkQL8K2O86E yz74mA/9ESkvuDjd2ZXIm4gljbv63tTbbVYRM1cJ6vMO8ax8JiHcEH5K/bEtex4I UrpzG0Yr5hL2zH0WhKbqqaNJHx3behepQ9j21D8l2z4kMKtE5KUfbnoxpPL8seBe wTQ25XpNypu8GOXzOe7RWJ+/QPwpp61WbDJFM2u+rI79CUTOj7JkOURdw+txv0Ou 9uVnk6jAzEl3b6TcVXj7lcagwBWUyxvf6uiHIPDF30XYqfZLfpADx8aboPaSvlRc fPpJcrc2g9leVvGuNdTDSfe4wRxSvdjPWOYI7uR9ar7jD22TBvLKyG+8F4dZFb7z y9C2AJDqk5ilRTSlsiky9ROJQpOU01G+mvjC1UyVQKF/YrejPx+qFBzs50mHzN/o dD95j2j38NgxQsCMjBzf49dHu0CfIjXiZiDB3Bs24WpznW8g5OvgtjNJWs1/693Q LZoqb14wtTENVL0aZewspcgwc55Dlz64qoT5SQzybpjHys5bQW0KMSsuDqev5OSm LgYq3v8yRpKV0+N/hEZF2578udYFv9ZN4Ku2QunBrEYCr8wvpXHux1LwZWdh2cNX 3n+RKopIWzMIUZG90N1LJkJssY5MIbVOQmCePPcOz/xlL90kKOfHAqh+Q51Df3eP Fi4tdIFz/jCAmPEYRwq3Mb+m1Fz7JE1NasYCspJeBQkrpKap+lQ= =PWT/ -----END PGP SIGNATURE----- --Apple-Mail=_FBD8AD63-2DE1-476E-90A5-60557DAE9BEC--