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.web10.642.1584564196610349405 for ; Wed, 18 Mar 2020 13:43:17 -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 7E798C010E; Wed, 18 Mar 2020 23:43:14 +0300 (MSK) From: "Vitaly Cheptsov" Message-Id: Mime-Version: 1.0 (Mac OS X Mail 13.0 \(3608.60.0.2.5\)) Subject: Re: [edk2-devel] Disabling safe string constraint assertions Date: Wed, 18 Mar 2020 23:43:12 +0300 In-Reply-To: Cc: "devel@edk2.groups.io" , Laszlo Ersek , Andrew Fish , =?utf-8?Q?Marvin_H=C3=A4user?= , "Gao, Liming" , "Gao, Zhichao" To: "Kinney, Michael D" References: <5F8859C7-5776-4DF0-99F5-1D0BC56B50C4@apple.com> <318F875F-3C9A-4236-841B-5BB9908CC139@ispras.ru> <38e4a6e4-ca31-3c2f-5df5-fb4b67e05f33@redhat.com> X-Mailer: Apple Mail (2.3608.60.0.2.5) X-Groupsio-MsgNum: 55964 Content-Type: multipart/signed; boundary="Apple-Mail=_346EA1B7-A38C-4E92-9AE2-37F2904B813A"; protocol="application/pgp-signature"; micalg=pgp-sha512 --Apple-Mail=_346EA1B7-A38C-4E92-9AE2-37F2904B813A Content-Type: multipart/alternative; boundary="Apple-Mail=_F237D885-D903-4B05-8751-3FC8E87A697A" --Apple-Mail=_F237D885-D903-4B05-8751-3FC8E87A697A Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 Mike, Thanks for the clarification. I failed to find it in the specs, but the co= de of the BaseTools kind of gave me such a suspect. Is there any particular reason why this limitation was added? At the momen= t I do not see a good reason why this is done. If there is one, I guess we could consider some other approach, for exampl= e, we can factor out these functions to a separate DebugHelperLib/DebugBase= Lib/DebugCommonLib, which every DebugLib will depend on. This will make sen= se to me as a workaround of such limitation, as neither us, nor Andrew, as = he mentioned previously, are happy of having to duplicate code in DebugLib = implementations and update them for a minor Pcd change. If there is no good reason, to be honest, it feels like we should just fix= this. After reading the spec I do not see what kind of compiler issue coul= d arise here with normal PCDs. Best regards, Vitaly > 18 =D0=BC=D0=B0=D1=80=D1=82=D0=B0 2020 =D0=B3., =D0=B2 23:35, Kinney, Mi= chael D =D0=BD=D0=B0=D0=BF=D0=B8=D1=81=D0=B0= =D0=BB(=D0=B0): >=20 > Vitaly, >=20 > The break you are seeing is because you are not using functions to eval = the PCD. This is a known restriction in how PCDs work between libs and mod= ules and is why the current design uses the XxxEnabled() functions. >=20 > I have not reviewed this issue in a very long time, so I do not know if = there are any attributes of newer compilers that would allow a different de= sign now. >=20 > Best regards, >=20 > Mike >=20 > From: devel@edk2.groups.io > On Behalf Of Vitaly Cheptsov > Sent: Wednesday, March 18, 2020 12:36 PM > To: Laszlo Ersek >; Andrew = Fish >; Kinney, Michael D >; Marvin H=C3=A4use= r >; Gao, Liming >; Gao, Zhichao > > Cc: devel@edk2.groups.io > Subject: Re: [edk2-devel] Disabling safe string constraint assertions >=20 > Hello! >=20 > I have a prototype of the patch, but there currently is an issue with th= e current EDK II build system. > I attached the patch to this e-mail, however, it will not compile for re= asonably obscure causes. >=20 > From what I understand: > - DebugLib header now directly uses PCDs from DebugLib, like PcdDebugPro= pertyMask. > - Any library implementing DebugLib should now depend on these PCDs, whi= ch seems fairly natural (and I fixed that in BaseDebugLibNull). > - Any library using DebugLib header should depend on DebugLib, which als= o depend on DebugLib to get its PCDs (that already looks fine). >=20 > However, for some reason DebugLib PCDs are not included in Autogen.h hea= der for other libraries some reason, and we get errors like: > MdePkg/Library/BaseOrderedCollectionRedBlackTreeLib/BaseOrderedCollectio= nRedBlackTreeLib.c:1151:9: error: use of undeclared identifier '_PCD_GET_MO= DE_8_PcdDebugPropertyMask' >=20 > I am not familiar with the build system well enough to resolve this, so = I either need guidance on where to look first or it will be great if somebo= dy else handles that. > I do not believe it is a great idea to abandon the idea of dropping Debu= gAssertEnabled-like functions, so I suggest us to focus on resolving the bu= ild system limitation rather than trying a new approach. >=20 > Best regards, > Vitaly >=20 >=20 >=20 >=20 >=20 > 11 =C3=90=C2=BC=C3=90=C2=B0=C3=91=E2=82=AC=C3=91=E2=80=9A=C3=90=C2=B0 20= 20 =C3=90=C2=B3., =C3=90=C2=B2 16:14, Laszlo Ersek > =C3=90=C2=BD=C3=90=C2=B0=C3=90=C2=BF=C3=90=C2=B8=C3= =91=C2=81=C3=90=C2=B0=C3=90=C2=BB(=C3=90=C2=B0): >=20 > On 03/11/20 14:09, Vitaly Cheptsov wrote: >=20 > Hi everyone, >=20 > So, I believe that by now we mostly agreed to let the original > proposition land as a short-term solution. We end up with: >=20 > 1. A PCD condition within SAFE_STRING_COSTRAINT_CHECK macro. > 2. Make this condition evaluate to TRUE by default (i.e. ASSERT). > 3. Update documentation for BaseLib functions to include the information > about this behaviour. >=20 > The only thing in question is whether this should be a separate PCD or > an extra bit in PcdDebugPropertyMask. I believe that we almost agreed on > two things: >=20 > 1. Adding an extra bit to PcdDebugPropertyMask is cleaner. > 2. Extending DebugLib interface with a new function is not a good idea. >=20 > Therefore I suggest: >=20 > 1.Add #define DEBUG_PROPERTY_ASSERT_CONSTRAINT_ENABLED 0x40. > 2. Create header-only macros to replace functions like > DebugAssertEnabled. We can then use these macros in new code and > deprecate the original functions. > 3. Enable DEBUG_PROPERTY_ASSERT_CONSTRAINT_ENABLED bit in MdePkg by defa= ult. >=20 > I will submit the new version of the patch soon unless there is an > immediate opposing opinion. >=20 > Not sure about any particular deprecation timeline, but to me the above > certainly sounds worth submitting for review. >=20 > (NB I don't plan to review in detail -- I just meant to comment on the > design, since I was asked to.) >=20 > Thanks! > Laszlo >=20 >=20 --Apple-Mail=_F237D885-D903-4B05-8751-3FC8E87A697A Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=utf-8 Mike,

Thanks for the clarification. I failed to = find it in the specs, but the code of the BaseTools kind of gave me such a = suspect.
Is there any particular reason why this limit= ation was added? At the moment I do not see a good reason why this is done.=

If there is one,= I guess we could consider some other approach, for example, we can factor = out these functions to a separate DebugHelperLib/DebugBaseLib/DebugCommonLi= b, which every DebugLib will depend on. This will make sense to me as a wor= karound of such limitation, as neither us, nor Andrew, as he mentioned prev= iously, are happy of having to duplicate code in DebugLib implementations a= nd update them for a minor Pcd change.

=
If there is no good reason, to be honest, it feels li= ke we should just fix this. After reading the spec I do not see what kind o= f compiler issue could arise here with normal PCDs.
Best regards,
Vital= y

18 =D0=BC=D0=B0=D1=80=D1=82=D0=B0 2020 =D0=B3., =D0=B2 23:35,= Kinney, Michael D <michael.d.kinney@intel.com> =D0=BD=D0=B0=D0=BF=D0=B8=D1=81=D0= = =B0=D0=BB(=D0=B0):

Vitaly,
 
The= break you are seeing is because you are not using functions to eval the PC= D.  This is a known restriction in how PCDs work between libs and modul= es and is why the current design uses the XxxEnabled() functions.
=  
I have not reviewed this issue in a very long time, so I do not = know if there are any attributes of newer compilers that would allow a diff= erent design now.
 
Best regards,
 = ;
Mike
<= o:p class=3D""> 
<= b class=3D"">From: devel= @edk2.groups.io <<= a href=3D"mailto:devel@edk2.groups.io" style=3D"color: purple; text-decorat= ion: underline;" class=3D"">devel@edk2.groups.io> On Behalf Of Vitaly Cheptsov
Sent: Wednesda= y, March 18, 2020 12:36 PM
To: Laszlo Ersek <lersek@redhat.com>; Andrew Fish <afish@apple.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Marvin H=C3=A4user <mhaeuser@outlook.de>; Gao, Liming <liming.gao@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>
Cc: devel@edk2.groups.io
Subject: Re: [edk2-d= evel] Disabling safe string constraint assertions
 
Hello!=
 
<= div class=3D"">
I have a protot= ype of the patch, but there currently is an issue with the current EDK II b= uild system.
I attached the patch to this e-mail,= however, it will not compile for reasonably obscure causes.
 
=
From what I understand:
- DebugLib header now = directly uses PCDs from DebugLib, like PcdDebugPropertyMask.
- Any library implementing DebugLib should now depe= nd on these PCDs, which seems fairly natural (and I fixed that in BaseDebug= LibNull).
- Any library using DebugLib header sho= uld depend on DebugLib, which also depend on DebugLib to get its PCDs (that= already looks fine).
 
However, for some reason DebugLib PCDs are not included in Autogen.h= header for other libraries some reason, and we get errors like:
MdePkg/Library/BaseOrderedCollectionRedBlackTreeLib/BaseO= rderedCollectionRedBlackTreeLib.c:1151:9: error: use of undeclared identifi= er '_PCD_GET_MODE_8_PcdDebugPropertyMask'
 
I am not familiar with the build system well enou= gh to resolve this, so I either need guidance on where to look first or it = will be great if somebody else handles that.
I do= not believe it is a great idea to abandon the idea of dropping DebugA= ssertEnabled-like functions, so I suggest us to focus on resolving the buil= d system limitation rather than trying a new approach.
 
Best regards,=
Vitaly
 =
 
 


11 =C3=90=C2=BC=C3=90=C2=B0=C3=91=E2=82=AC=C3=91=E2=80=9A=C3= =90=C2=B0 2020 =C3=90=C2=B3., =C3=90=C2=B2 16:14, Laszlo Ersek <lersek@redhat.com> =C3=90=C2=BD=C3=90=C2=B0=C3= =90=C2=BF=C3=90=C2=B8=C3=91=C2=81=C3=90=C2=B0=C3=90=C2=BB(=C3=90=C2=B0):
 
On 03/11/20 14:09, Vitaly Cheptsov wrote:

Hi everyone,

So, I believe t= hat by now we mostly agreed to let the original
proposition l= and as a short-term solution. We end up with:

= 1. A PCD condition within SAFE_STRING_COSTRAINT_CHECK macro.
= 2. Make this condition evaluate to TRUE by default (i.e. ASSERT).
3. Update documentation for BaseLib functions to include the informa= tion
about this behaviour.

The o= nly thing in question is whether this should be a separate PCD or
an extra bit in PcdDebugPropertyMask. I believe that we almost agree= d on
two things:

1. Adding an ex= tra bit to PcdDebugPropertyMask is cleaner.
2. Extending Debu= gLib interface with a new function is not a good idea.

Therefore I suggest:

1.Add #define DE= BUG_PROPERTY_ASSERT_CONSTRAINT_ENABLED 0x40.
2. Create header= -only macros to replace functions like
DebugAssertEnabled. We= can then use these macros in new code and
deprecate the orig= inal functions.
3. Enable DEBUG_PROPERTY_ASSERT_CONSTRAI= NT_ENABLED bit in MdePkg by default.

I will su= bmit the new version of the patch soon unless there is an
imm= ediate opposing opinion.

Not sure about any particular d= eprecation timeline, but to me the above
certainly sounds wor= th submitting for review.

(NB I don't plan to = review in detail -- I just meant to comment on the
design, si= nce I was asked to.)

Thanks!
Las= zlo
&n= bsp;

--Apple-Mail=_F237D885-D903-4B05-8751-3FC8E87A697A-- --Apple-Mail=_346EA1B7-A38C-4E92-9AE2-37F2904B813A 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----- iQIzBAEBCgAdFiEEsLABAI5Y5VbvBdmpL8K2O86Eyz4FAl5yh+AACgkQL8K2O86E yz7pKg/9GaXQaqy2rxLmc3C0FjeQKEeHUYUc8KOpI0wFb6CK19eJIbrBxluKcIhA 6MdFYqQz30U9XZoP0JYfIaXjK82XI5ZV+cWef9zoqFKNlYyRTs/51BldVSZhF2xy m4Kx1m9L5yTL7A6I2v0366g4WwXj4KmFaVP0EKLA9mc74vjdqNa7GnPQUndw1WjS v2zXF2yzIjtEUmaiPWeB3BesbNB5An34alQmU9p4UGAS5pAIRTVGXAdaFqbYc+ih LwCko+VhQR0V3Tz4EeR/3FO2hpPrulfvPSW0jNEY0A0IDig63Ki9V6vIqpMde90T eQtr3K2jGNpvKXTKpTLPhjS4H5eSHc6jHyIISSyrikW9NbBEzzFhBeQFkismdxNG E7m4SiAQMQO8swbfG1ha9qdz1nijrmml4dEYHY895uMt2wPS8+WWCJzzM59i+3Xv IyE6LJ3ev7I9LdE6GiBtyFnOKR2YbkFIgMoIUPZgoAA7LjPHqXdjZ3thh3VT0TSR HPJH25EWhMJi5as9r4WLmG8SR6hXCTuLX2cyWGfnMx5tIH6cHY3icsQmxxA4P5ot z0hwHuyWUwXv1FBTHGZNFBENFsTagRbz7VmsbMZDU8Uu5Rt7XB5ymcHES4EvrTEI HhKRwO9Fp+giyOZo+MOC+y4pMLfLRTm6V+zRXQW//l0Lu8XaXEE= =v96M -----END PGP SIGNATURE----- --Apple-Mail=_346EA1B7-A38C-4E92-9AE2-37F2904B813A--