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.1803.1584567095666661124 for ; Wed, 18 Mar 2020 14:31:36 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: ispras.ru, ip: 83.149.199.45, mailfrom: cheptsov@ispras.ru) Received: from [10.0.187.81] (unknown [217.114.218.24]) by mail.ispras.ru (Postfix) with ESMTPSA id C8DF5C010E; Thu, 19 Mar 2020 00:31:33 +0300 (MSK) From: "Vitaly Cheptsov" Mime-Version: 1.0 (1.0) Subject: Re: [edk2-devel] Disabling safe string constraint assertions Date: Thu, 19 Mar 2020 00:31:31 +0300 Message-Id: <7867AB09-6EB0-4EC6-93B4-0874DD3B1EB3@ispras.ru> References: Cc: "devel@edk2.groups.io" , Laszlo Ersek , Andrew Fish , =?utf-8?Q?Marvin_H=C3=A4user?= , "Gao, Liming" , "Gao, Zhichao" In-Reply-To: To: "Kinney, Michael D" X-Mailer: iPad Mail (17D50) Content-Type: multipart/alternative; boundary=Apple-Mail-BFECED39-D591-42B2-950E-6A59AD4D5D12 Content-Transfer-Encoding: 7bit --Apple-Mail-BFECED39-D591-42B2-950E-6A59AD4D5D12 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Mike, That explains the current behaviour, but makes me even more confused. I do not really understand how DEC format is responsible for this. Librari= es, described with INF files, consume PCDs and potentially override their v= alues. DEC files produce PCDs, which libraries or modules (drivers, appicat= ions) can consume. Header-only libraries have no INF files, and thus are no= t really libraries one can depend on, and thus can have no PCDs. I cannot m= ake a connection of how a library consuming a PCD could influence on a DEC = file. BaseTools' AutoGen implements DependentLibraryList and LibraryPcdList prop= erties, which effectively gather all library PCDs for a module. So they alr= eady have all the information about the PCDs used and needed to be added to= AutoGen.c and AutoGen.h. I expected them to add library PCD definitions to AutoGen.h for modules, b= ut for some reason it does not happen. They also explicitly skip PCD depend= ency walk for libraries, which I assumed to be some questionable performanc= e optimisation before I realised that they are not exposed for the former c= ase as well. It is very possible that I miss something, but to me it looks like the fac= t that we cannot see library PCDs in modules and higher level libraries is = just an artificial limitation, which should be possible to lift with reason= ably few changes in BaseTools for a person that is well aware of their code= base. Could you give a better insight on this or perhaps ask somebody who k= nows BaseTools internals? If you believe it is much worse than I see, I can just trust you for the t= ime being and focus on implementing an alternative approach by separating a= common DebugCommonLib. Thanks! Best regards, Vitaly > On 18 Mar 2020, at 23:55, Kinney, Michael D = wrote: >=20 > =EF=BB=BF > Vitaly, > > It has to do with where PCDs are declared in INF files. > > If you access a PCD from a macro like you have added to a library class,= the module using that library class does not know there is a macro that us= es a PCD. So the PCD declaration in the Module INF is missing. By only us= ing the PCDs from the library implementation, the library implementation IN= F declares the PCDs it uses and the module inherits the PCDs from the libra= ry instances. We do not have a feature that allows a library class (which = only has a .h file and a one line declaration in a DEC file) to provide ext= ra information such as PCDs that the library class uses. We would need a s= ignificant extension to the DEC file format and build tools for a library c= lass declaration to provide more information. > > Mike > > From: Vitaly Cheptsov =20 > Sent: Wednesday, March 18, 2020 1:43 PM > To: Kinney, Michael D > Cc: devel@edk2.groups.io; Laszlo Ersek ; Andrew Fish = ; Marvin H=C3=A4user ; Gao, Liming ; Gao, Zhichao > Subject: Re: [edk2-devel] Disabling safe string constraint assertions > > 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 limitation was added? At the mom= ent I do not see a good reason why this is done. > > If there is one, I guess we could consider some other approach, for exam= ple, we can factor out these functions to a separate DebugHelperLib/DebugBa= seLib/DebugCommonLib, which every DebugLib will depend on. This will make s= ense to me as a workaround of such limitation, as neither us, nor Andrew, a= s he mentioned previously, are happy of having to duplicate code in DebugLi= b implementations and update them for a minor Pcd change. > > If there is no good reason, to be honest, it feels like we should just f= ix this. After reading the spec I do not see what kind of compiler issue co= uld arise here with normal PCDs. > > Best regards, > Vitaly >=20 >=20 > 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): > > Vitaly, > > 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. > > 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. > > Best regards, > > Mike > > From: devel@edk2.groups.io On Behalf Of Vitaly Ch= eptsov > Sent: Wednesday, March 18, 2020 12:36 PM > To: Laszlo Ersek ; Andrew Fish ; Kin= ney, Michael D ; Marvin H=C3=A4user ; Gao, Liming ; Gao, Zhichao > Cc: devel@edk2.groups.io > Subject: Re: [edk2-devel] Disabling safe string constraint assertions > > Hello! > > 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. > > 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). > > 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' > > 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. > > Best regards, > Vitaly > > > >=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): > > On 03/11/20 14:09, Vitaly Cheptsov wrote: >=20 >=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 > --Apple-Mail-BFECED39-D591-42B2-950E-6A59AD4D5D12 Content-Type: text/html; charset=utf-8 Content-Transfer-Encoding: quoted-printable
Mike,

That explains the current behaviour, b= ut makes me even more confused.

I do not really understand how DEC format is responsible for this. Li= braries, described with INF files, consume PCDs and potentially override th= eir values. DEC files produce PCDs, which libraries or modules (drivers, ap= pications) can consume. Header-only libraries have no INF files, and thus a= re not really libraries one can depend on, and thus can have no PCDs. I can= not make a connection of how a library consuming a PCD could influence on a= DEC file.

BaseTools' Auto= Gen implements DependentLibraryList and LibraryPcdList properties, which ef= fectively gather all library PCDs for a module. So they already have all th= e information about the PCDs used and needed to be added to AutoGen.c and A= utoGen.h.

I expected them = to add library PCD definitions to AutoGen.h for modules, but for some reaso= n it does not happen. They also explicitly skip PCD dependency walk for lib= raries, which I assumed to be some questionable performance optimisation be= fore I realised that they are not exposed for the former case as well.

It is very possible that I mis= s something, but to me it looks like the fact that we cannot see library PC= Ds in modules and higher level libraries is just an artificial limitation, = which should be possible to lift with reasonably few changes in BaseTools f= or a person that is well aware of their codebase. Could you give a better i= nsight on this or perhaps ask somebody who knows BaseTools internals?
=

If you believe it is much worse= than I see, I can just trust you for the time being and focus on implement= ing an alternative approach by separating a common DebugCommonLib.

Thanks!

=
Best regards,
Vitaly

On 18 Mar 2020, at 23:55, Kinney= , Michael D <michael.d.kinney@intel.com> wrote:

<= /div>
=EF=BB=BF

V= italy,

<= o:p> 

I= t has to do with where PCDs are declared in INF files.

<= o:p> 

I= f you access a PCD from a macro like you have added to a library class, the= module using that library class does not know there is a macro that uses a PCD.  = So the PCD declaration in the Module INF is missing.  By only using the PCDs from the library implementation, the library= implementation INF declares the PCDs it uses and the module inherits the P= CDs from the library instances.  We do not have a feature that allows a library class (which only ha= s a .h file and a one line declaration in a DEC file) to provide extra info= rmation such as PCDs that the library class uses.  We would need a significant extension to the DEC file format and bu= ild tools for a library class declaration to provide more information.=

<= o:p> 

M= ike

<= o:p> 

From: Vitaly Cheptsov <cheptsov@ispras.ru>
Sent: Wednesday, March 18, 2020 1:43 PM
To: Kinney, Michael D <michael.d.kinney@intel.com>
Cc: devel@edk2.groups.io; Laszlo Ersek <lersek@redhat.com>; A= ndrew Fish <afish@apple.com>; Marvin H=C3=A4user <mhaeuser@outlook= .de>; Gao, Liming <liming.gao@intel.com>; Gao, Zhichao <zhichao= .gao@intel.com>
Subject: Re: [edk2-devel] Disabling safe string constraint assertio= ns

 

Mike,

 

Thanks for the clarification. I failed to find it in the s= pecs, but the code of the BaseTools kind of gave me such a suspect.

Is there any particular reason why this limitation was add= ed? 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 appr= oach, for example, we can factor out these functions to a separate DebugHel= perLib/DebugBaseLib/DebugCommonLib, which every DebugLib will depend on. This will make sense to me as a workaround of su= ch limitation, as neither us, nor Andrew, as he mentioned previously, are h= appy of having to duplicate code in DebugLib implementations and update the= m 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 com= piler issue could 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, Michael D <mic= hael.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 func= tions to eval the PCD.  This is a known restriction in how PCDs work between libs and modules and is why the current design uses the XxxEnabled() func= tions.

 

I have not reviewed this issue in a very long time, so I d= o not know if there are any attributes of newer compilers that would allow = a different design now.

 

Best regards,

 

Mike

 

From: = devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Vitaly Ch= eptsov
Sent: Wednesday, = March 18, 2020 12:36 PM
To: Laszlo Ersek = <lerse= k@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.i= o
Subject: Re: [edk= 2-devel] Disabling safe string constraint assertions

 

Hello!

 

I have a prototype of the patch, but there currently is an= issue with the current EDK II build 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 De= bugLib, like PcdDebugPropertyMask.

- Any library implementing DebugLib should now depend on t= hese PCDs, which seems fairly natural (and I fixed that in BaseDebugLibNull= ).

- Any library using DebugLib header should depend on Debug= Lib, 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:<= o:p>

MdePkg/Library/BaseOrderedCollectionRedBlackTreeLib/BaseOr= deredCollectionRedBlackTreeLib.c:1151:9: error: use of undeclared identifie= r '_PCD_GET_MODE_8_PcdDebugPropertyMask'

 

I am not familiar with the build system well enough to res= olve this, so I either need guidance on where to look first or it will be g= reat if somebody else handles that.

I do not believe it is a great idea to abandon the idea of= dropping DebugAssertEnabled-like functions, so I suggest us to focus = on resolving the build 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 that by now we mostly agreed to let the original
proposition land 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 information about this behaviour.

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:

1. Adding an extra bit to PcdDebugPropertyMask is cleaner.
2. Extending DebugLib interface with a new function is not a good idea.
Therefore I suggest:

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 d= efault.

I will submit the new version of the patch soon unless there is an
immediate opposing opinion.


Not sure about any particular deprecation timeline, but to me the above certainly sounds worth submitting for review.

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

Thanks!
Laszlo

 

 

--Apple-Mail-BFECED39-D591-42B2-950E-6A59AD4D5D12--