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.10493.1589198601596892018 for ; Mon, 11 May 2020 05:03:22 -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 2CA8ECD460; Mon, 11 May 2020 15:03:18 +0300 (MSK) From: "Vitaly Cheptsov" Message-Id: Mime-Version: 1.0 (Mac OS X Mail 13.4 \(3608.80.23.2.2\)) Subject: Re: [edk2-devel] Disabling safe string constraint assertions Date: Mon, 11 May 2020 15:03:17 +0300 In-Reply-To: <338E2F04-B523-473B-B1A4-AD855B8311DE@ispras.ru> Cc: devel@edk2.groups.io To: Andrew Fish , Mike Kinney , Laszlo Ersek , =?utf-8?Q?Marvin_H=C3=A4user?= , "Gao, Liming" , "Gao, Zhichao" References: <4219F456-E97B-403F-80D1-07EA07CE8549@apple.com> <338E2F04-B523-473B-B1A4-AD855B8311DE@ispras.ru> X-Mailer: Apple Mail (2.3608.80.23.2.2) X-Groupsio-MsgNum: 59068 Content-Type: multipart/signed; boundary="Apple-Mail=_18B8BA0C-A0E4-4CFB-A69B-43663718D1C8"; protocol="application/pgp-signature"; micalg=pgp-sha512 --Apple-Mail=_18B8BA0C-A0E4-4CFB-A69B-43663718D1C8 Content-Type: multipart/alternative; boundary="Apple-Mail=_0D9601B1-61BE-4789-9830-DD768A7005CA" --Apple-Mail=_0D9601B1-61BE-4789-9830-DD768A7005CA Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 Hello, The new version of the patchset was submitted via github (mainly due to th= e amount of patches to avoid spamming the mailing list): https://github.com/tianocore/edk2/pull/601 Let me know if any further changes are needed from my side. I hope this st= ill is in time for the May tag. Best wishes, Vitaly > 19 =D0=BC=D0=B0=D1=80=D1=82=D0=B0 2020 =D0=B3., =D0=B2 03:04, Vitaly Che= ptsov =D0=BD=D0=B0=D0=BF=D0=B8=D1=81=D0=B0=D0=BB(=D0= =B0): >=20 > Andrew, Mike, >=20 > Thank you very much for the comments. Yes, I am aware of PCD overriding = in the DSC file, and in fact we are using it for the exact same purpose to = configure Shell, inject and override some of its libraries with different s= ettings. >=20 > From what I understand the library PCD values should be put to: > 1. AutoGen.c of each application/driver built (as a value; *not* to the = library AutoGen.c). > 2. AutoGen.h of the library itself (as an extern). > 3. AutoGen.h of the dependent library that depends on the library claimi= ng to use the PCD. > 4. AutoGen.h of the application/driver. >=20 > From what I understand, 1 and 2 are already done by the EDK II BaseTools= . So, currently the only things that need to happen are 3 and 4. I do not s= ee any change in the PCD overriding functionality if they land. The only do= wnside I can imagine is a theoretical performance penalty, but this does no= t seem to be a design problem. Such things if they arise are best to be res= olved by an alternative implementation of the build tools. >=20 > The limitation of not building a separate library is indeed somewhat a p= roblem, as it collides with fixed PCDs. I.e. we cannot override fixed PCDs = in the DSC for a particular application, as the library is already built, a= nd fixed PCDs are evaluated during preprocessing/library compilation. Howev= er, nothing changes here, and I assume it can be continued to live with. >=20 > Like I said, for a person like me it seems like a relatively minor chang= e in the BaseTools. Unfortunately, since I have no good grasp of its archit= ecture it will likely take long for me to prepare a solution and ensure tha= t it does not break things for anyone. If there is no-one who can handle it= by the next stable tag I could imagine going with the library route and pe= rhaps filing a feature request in the bugzilla, so that is not forgotten. >=20 > Does the approach of splitting DebugLib into common and implementation p= arts sound good to both of you? I believe you should have a number of custo= m DebugLib implementations. While this approach is not as good as the origi= nal macro route (especially for compilers without LTO), it should still let= everyone add more changes to PCD sets and other shared debugging parts wit= hout the need to change DebugLib implementations after the first and the on= ly transition. >=20 > Best regards, > Vitaly >=20 >> On 19 Mar 2020, at 00:53, Andrew Fish wrote: >>=20 >> =EF=BB=BFVitaly, >>=20 >> The library object files can be shared between modules. If is possible = to override PCD settings per module in the DSC file. So libraries need to e= ither derive their PCD value from the driver/app they are linking with, or = we would need to build different instances of the library with the differen= t PCD defaults and link the correct one. The build system does not support = building extra copies of the libraries so we have the restriction Mike ment= ioned. >>=20 >> https://github.com/tianocore/edk2/blob/master/OvmfPkg/OvmfPkgX64.dsc#L8= 56 >> ShellPkg/Application/Shell/Shell.inf { >> >> ShellCommandLib|ShellPkg/Library/UefiShellCommandLib/UefiShellCom= mandLib.inf >> NULL|ShellPkg/Library/UefiShellLevel2CommandsLib/UefiShellLevel2C= ommandsLib.inf >> NULL|ShellPkg/Library/UefiShellLevel1CommandsLib/UefiShellLevel1C= ommandsLib.inf >> NULL|ShellPkg/Library/UefiShellLevel3CommandsLib/UefiShellLevel3C= ommandsLib.inf >> NULL|ShellPkg/Library/UefiShellDriver1CommandsLib/UefiShellDriver= 1CommandsLib.inf >> NULL|ShellPkg/Library/UefiShellDebug1CommandsLib/UefiShellDebug1C= ommandsLib.inf >> NULL|ShellPkg/Library/UefiShellInstall1CommandsLib/UefiShellInsta= ll1CommandsLib.inf >> NULL|ShellPkg/Library/UefiShellNetwork1CommandsLib/UefiShellNetwo= rk1CommandsLib.inf >> !if $(NETWORK_IP6_ENABLE) =3D=3D TRUE >> NULL|ShellPkg/Library/UefiShellNetwork2CommandsLib/UefiShellNetwo= rk2CommandsLib.inf >> !endif >> HandleParsingLib|ShellPkg/Library/UefiHandleParsingLib/UefiHandle= ParsingLib.inf >> PrintLib|MdePkg/Library/BasePrintLib/BasePrintLib.inf >> BcfgCommandLib|ShellPkg/Library/UefiShellBcfgCommandLib/UefiShell= BcfgCommandLib.inf >>=20 >> >> gEfiMdePkgTokenSpaceGuid.PcdDebugPropertyMask|0xFF >> gEfiShellPkgTokenSpaceGuid.PcdShellLibAutoInitialize|FALSE >> gEfiMdePkgTokenSpaceGuid.PcdUefiLibMaxPrintBufferSize|8000 >> } >>=20 >>=20 >> Thanks, >>=20 >> Andrew Fish >>=20 >>> On Mar 18, 2020, at 2:31 PM, Vitaly Cheptsov > wrote: >>>=20 >>> Mike, >>>=20 >>> That explains the current behaviour, but makes me even more confused. >>>=20 >>> I do not really understand how DEC format is responsible for this. Lib= raries, described with INF files, consume PCDs and potentially override the= ir values. DEC files produce PCDs, which libraries or modules (drivers, app= ications) can consume. Header-only libraries have no INF files, and thus ar= e not really libraries one can depend on, and thus can have no PCDs. I cann= ot make a connection of how a library consuming a PCD could influence on a = DEC file. >>>=20 >>> BaseTools' AutoGen implements DependentLibraryList and LibraryPcdList = properties, which effectively gather all library PCDs for a module. So they= already have all the information about the PCDs used and needed to be adde= d to AutoGen.c and AutoGen.h. >>>=20 >>> I expected them to add library PCD definitions to AutoGen.h for module= s, but for some reason it does not happen. They also explicitly skip PCD de= pendency walk for libraries, which I assumed to be some questionable perfor= mance optimisation before I realised that they are not exposed for the form= er case as well. >>>=20 >>> It is very possible that I miss something, but to me it looks like the= fact that we cannot see library PCDs in modules and higher level libraries= is just an artificial limitation, which should be possible to lift with re= asonably few changes in BaseTools for a person that is well aware of their = codebase. Could you give a better insight on this or perhaps ask somebody w= ho knows BaseTools internals? >>>=20 >>> If you believe it is much worse than I see, I can just trust you for t= he time being and focus on implementing an alternative approach by separati= ng a common DebugCommonLib. >>>=20 >>> Thanks! >>>=20 >>> Best regards, >>> Vitaly >>>=20 >>>> On 18 Mar 2020, at 23:55, Kinney, Michael D > wrote: >>>>=20 >>>> =EF=BB=BF >>>> Vitaly, >>>>=20 >>>> It has to do with where PCDs are declared in INF files. >>>>=20 >>>> If you access a PCD from a macro like you have added to a library cla= ss, 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 PCDs from the li= brary instances. We do not have a feature that allows a library class (whi= ch only has a .h file and a one line declaration in a DEC file) to provide = extra information such as PCDs that the library class uses. We would need = a significant extension to the DEC file format and build tools for a librar= y class declaration to provide more information. >>>>=20 >>>> Mike >>>>=20 >>>> From: Vitaly Cheptsov = > >>>> 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 >>>>=20 >>>> Mike, >>>>=20 >>>> Thanks for the clarification. I failed to find it in the specs, but t= he code of the BaseTools kind of gave me such a suspect. >>>> Is there any particular reason why this limitation was added? At the = moment I do not see a good reason why this is done. >>>>=20 >>>> If there is one, I guess we could consider some other approach, for e= xample, we can factor out these functions to a separate DebugHelperLib/Debu= gBaseLib/DebugCommonLib, which every DebugLib will depend on. This will mak= e sense 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 Debu= gLib implementations and update them for a minor Pcd change. >>>>=20 >>>> If there is no good reason, to be honest, it feels like we should jus= t fix this. After reading the spec I do not see what kind of compiler issue= could arise here with normal PCDs. >>>>=20 >>>> Best regards, >>>> Vitaly >>>>=20 >>>>=20 >>>> 18 =D0=BC=D0=B0=D1=80=D1=82=D0=B0 2020 =D0=B3., =D0=B2 23:35, Kinney,= Michael 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 ev= al the PCD. This is a known restriction in how PCDs work between libs and = modules 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= design 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 >; Andr= ew Fish >; Kinney, Michael D >; Marvin H=C3=A4= user >; 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= the current EDK II build system. >>>> I attached the patch to this e-mail, however, it will not compile for= reasonably obscure causes. >>>>=20 >>>> From what I understand: >>>> - DebugLib header now directly uses PCDs from DebugLib, like PcdDebug= PropertyMask. >>>> - Any library implementing DebugLib should now depend on these PCDs, = which seems fairly natural (and I fixed that in BaseDebugLibNull). >>>> - Any library using DebugLib header should depend on DebugLib, which = also depend on DebugLib to get its PCDs (that already looks fine). >>>>=20 >>>> 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/BaseOrderedCollec= tionRedBlackTreeLib.c:1151:9: error: use of undeclared identifier '_PCD_GET= _MODE_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 som= ebody else handles that. >>>> I do not believe it is a great idea to abandon the idea of dropping D= ebugAssertEnabled-like functions, so I suggest us to focus on resolving the= build system limitation rather than trying a new approach. >>>>=20 >>>> Best regards, >>>> Vitaly >>>>=20 >>>>=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= 2020 =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 >>>>=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 informat= ion >>>> about this behaviour. >>>>=20 >>>> The only thing in question is whether this should be a separate PCD o= r >>>> 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 ide= a. >>>>=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 d= efault. >>>>=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 abo= ve >>>> certainly sounds worth submitting for review. >>>>=20 >>>> (NB I don't plan to review in detail -- I just meant to comment on th= e >>>> design, since I was asked to.) >>>>=20 >>>> Thanks! >>>> Laszlo >>>>=20 >>>>=20 >>>=20 >>=20 --Apple-Mail=_0D9601B1-61BE-4789-9830-DD768A7005CA Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=utf-8 Hello,
The new version of the patchset was submi= tted via github (mainly due to the amount of patches to avoid spamming the = mailing list):
https://github.com/tianocore/edk2/pull/601

Let me know if any= further changes are needed from my side. I hope this still is in time for = the May tag.

Best= wishes,
Vitaly

19 =D0=BC=D0=B0=D1=80=D1= =82=D0=B0 2020 =D0=B3., =D0=B2 03:04, Vitaly Cheptsov <cheptsov@ispras.ru> =D0=BD=D0=B0=D0= = =BF=D0=B8=D1=81=D0=B0=D0=BB(=D0=B0):

Andrew, Mike,

Thank you very much for the comments. Y= es, I am aware of PCD overriding in the DSC file, and in fact we are using = it for the exact same purpose to configure Shell, inject and override some = of its libraries with different settings.
=
From what I understand the= library PCD values should be put to:
1. A= utoGen.c of each application/driver built (as a value; *not* to the library= AutoGen.c).
2. AutoGen.h of the library i= tself (as an extern).
3. AutoGen.h of the = dependent library that depends on the library claiming to use the PCD.
4. AutoGen.h of the application/driver.
<= div dir=3D"ltr" class=3D"">
>>From what I understand, 1 and 2 are already done by the EDK II BaseTools. So, curre= ntly the only things that need to happen are 3 and 4. I do not see any change in the PCD ov= erriding functionality if they land. The only downside I can im= agine is a theoretical performance penalty, but this does not seem to be a = design problem. Such things if they arise are best to be resolved by an alt= ernative implementation of the build tools.

The limitation of not bu= ilding a separate library is indeed somewhat a problem, as it collides with= fixed PCDs. I.e. we cannot override fixed PCDs in the DSC for a particular= application, as the library is already built, and fixed PCDs are evaluated= during preprocessing/library compilation. However, nothing changes here, a= nd I assume it can be continued to live with.

Like I said, for a = person like me it seems like a relatively minor change in the BaseTools. Un= fortunately, since I have no good grasp of its architecture it will likely = take long for me to prepare a solution and ensure that it does not break th= ings for anyone. If there is no-one who can handle it by the next stable ta= g I could imagine going with the library route and perhaps filing a feature= request in the bugzilla, so that is not forgotten.

Does the approac= h of splitting DebugLib into common and implementation parts sound good to = both of you? I believe you should have a number of custom DebugLib implemen= tations. While this approach is not as good as the original macro route (es= pecially for compilers without LTO), it should still let everyone add more = changes to PCD sets and other shared debugging parts without the need to ch= ange DebugLib implementations after the first and the only transition.

Best regards,
Vitaly

On 19 Mar 2020, at 00:53, Andrew Fish <afish@apple.com> wrote:

=EF=BB=BFVitaly,

The library object files can be share= d between modules. If is possible to override PCD settings per module in th= e DSC file. So libraries need to either derive their PCD value from the dri= ver/app they are linking with, or we would need to build different instance= s of the library with the different PCD defaults and link the correct one. = The build system does not support building extra copies of the libraries so= we have the restriction Mike mentioned. 

https://github.com/tian= ocore/edk2/blob/master/OvmfPkg/OvmfPkgX64.dsc#L856
= ShellPkg/Application/Shell/She= ll.inf {
= <LibraryClasses>
= ShellCommandLib|ShellPkg/Library/UefiShellCommandLib/UefiShellCommandLi= b.inf
= NULL|ShellPkg/Library/UefiShellLevel2CommandsLib/UefiShellLevel2Command= sLib.inf
= NULL|ShellPkg/Library/UefiShellLevel1CommandsLib/UefiShellLevel1Command= sLib.inf
= NULL|ShellPkg/Library/UefiShellLevel3CommandsLib/UefiShellLevel3Command= sLib.inf
= NULL|ShellPkg/Library/UefiShellDriver1CommandsLib/UefiShellDriver1Comma= ndsLib.inf
= NULL|ShellPkg/Library/UefiShellDebug1CommandsLib/UefiShellDebug1Command= sLib.inf
= NULL|ShellPkg/Library/UefiShellInstall1CommandsLib/UefiShellInstall1Com= mandsLib.inf
= NULL|ShellPkg/Library/UefiShellNetwork1CommandsLib/UefiShellNetwork1Com= mandsLib.inf
!i= f $(NETWORK_IP6_ENABLE) =3D=3D TRUE
= NULL|ShellPkg/Library/UefiShellNetwork2CommandsLib/UefiShellNetwork2Com= mandsLib.inf
!e= ndif
= HandleParsingLib|ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsin= gLib.inf
= PrintLib|MdePkg/Library/BasePrintLib/BasePrintLib.inf
= BcfgCommandLib|ShellPkg/Library/UefiShellBcfgCommandLib/UefiShellBcfgCo= mmandLib.inf
= <PcdsFixedAtBuild>
= gEfiMdePkgTokenSpaceGuid.PcdDebugPropertyMask|0xFF
= gEfiShellPkgTokenSpaceGuid.PcdShellLibAutoInitialize|FALSE
= gEfiMdePkgTokenSpaceGuid.PcdUefiLibMaxPrintBufferSize|8000
= }


Thanks,

Andrew Fish

On Mar 18, 2020, at 2:31 PM, Vitaly Cheptsov <cheptsov@ispras.ru> wrote:

Mike,

That explains the current behaviour, but makes me even more co= nfused.

I do not r= eally understand how DEC format is responsible for this. Libraries, describ= ed with INF files, consume PCDs and potentially override their values. DEC = files produce PCDs, which libraries or modules (drivers, appications) can c= onsume. Header-only libraries have no INF files, and thus are not really li= braries one can depend on, and thus can have no PCDs. I cannot make a conne= ction of how a library consuming a PCD could influence on a DEC file.
=

BaseTools' AutoGen implem= ents DependentLibraryList and LibraryPcdList properties, which effectively = gather all library PCDs for a module. So they already have all the informat= ion about the PCDs used and needed to be added to AutoGen.c and AutoGen.h.<= /div>

I expected them to a= dd library PCD definitions to AutoGen.h for modules, but for some reason it= does not happen. They also explicitly skip PCD dependency walk for librari= es, which I assumed to be some questionable performance optimisation before= I realised that they are not exposed for the former case as well.

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

=
If you believe it is mu= ch worse than I see, I can just trust you for the time being and focus on i= mplementing an alternative approach by separating a common DebugCommonLib.<= /div>

Thanks!

Best regards,
Vitaly

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

=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 cl= ass, the module using that library class does not know there is a macro tha= t uses a PCD. &= nbsp;So the PCD declaration in the Module INF is missing.  By only using the PCDs from the library implementation, the library imple= mentation INF declares the PCDs it uses and the module inherits the PCDs fr= om the library instances.  We do not have a feature that allows a libra= ry class (which only has a .h file and a one line declaration in a DEC file= ) to provide extra information such as PCDs that the library class uses.  We would need a significant extension to the DEC file format and build t= ools for a library class declaration to provide more information.
 
Mike
<= span class=3D""> 
From: Vitaly Cheptsov <cheptsov@ispras.ru> 
S= ent: Wednesday, March= 18, 2020 1:43 PM
To: Kinney, Michael D <michael.d.kinney@intel.com>
Cc: devel@edk2.groups.io= ; Laszlo Ersek <lers= ek@redhat.com>; Andrew Fish <afish@apple.com>; Marvin H=C3=A4user <mhaeuser@outlook.de>; Gao, Liming &= lt;liming.gao@intel.com<= /a>>; Gao, Zhichao <zhichao.gao@intel.com>
Subject: Re: [edk2-devel] Disabling= safe string constraint assertions
=
 
Mike,
 
Thanks for the clarification. I failed to find it i= n the specs, but the code of the BaseTools kind of gave me such a suspect.<= o:p class=3D"">
Is there any particular reason why this limitatio= n 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 ou= t these functions to a separate DebugHelperLib/DebugBaseLib/DebugCommonLib,= which every DebugLib will depend on. This will make sense to me as a worka= round of such limitation, as neither us, nor Andrew, as he mentioned previo= usly, 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 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 <michael.d.kinney@intel.c= om> =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 modules and is why the current design uses the XxxEnabled() = functions.
 
I hav= e not reviewed this issue in a very long time, so I do not know if there ar= e any attributes of newer compilers that would allow a different design now= .
 
=
Best regards,<= o:p class=3D"">
 
Mike
 
Fro= m: <= /span>devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Vitaly Cheptsov
Sent: <= /span>Wednesday, 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 <= mhaeu= ser@outlook.de>; Gao, Liming <liming.gao@intel.com&= gt;; Gao, Zhichao <zhichao.gao@intel.com>
Cc: devel@edk2= .groups.io
Subject: Re: [edk2-devel] Disabling safe st= ring constraint assertions
 
Hello!<= /div>
 
I= have a prototype of the patch, but there currently is an issue with the cu= rrent EDK II build system.
= I attached the patch to this e-mail, however, it will not compile for reaso= nably obscure causes.
 = ;
From what I understand:=
- DebugLib header now&n= bsp;directly uses PCDs from DebugLib, like PcdDebugPropertyMask.
- Any library implementing Debug= Lib should now depend on these PCDs, which seems fairly natural (and I fixe= d that in BaseDebugLibNull).
=
- Any library using DebugLib header should depend on DebugLib, which also= depend on DebugLib to get its PCDs (that already looks fine).
 
<= span class=3D"">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/BaseOrderedCo= llectionRedBlackTreeLib/BaseOrderedCollectionRedBlackTreeLib.c:1151:9: erro= r: use of undeclared identifier '_PCD_GET_MODE_8_PcdDebugPropertyMask'
<= div style=3D"margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibr= i, sans-serif;" class=3D""> 
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 wi= ll be great if somebody else handles that.
<= span class=3D"">I do not believe it is a great idea to abandon the idea of = dropping DebugAssertEnabled-like functions, so I suggest us to focus o= n resolving the build system limitation rather than trying a new approach.<= o:p class=3D"">
 
Best regards,
Vitaly
 = ;
 <= /o:p>
<= span class=3D""> 



<= div class=3D"">
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 most= ly agreed to let the original
proposition land as a short-ter= m solution. We end up with:

1. A PCD condition= within SAFE_STRING_COSTRAINT_CHECK macro.
2. Make this condi= tion evaluate to TRUE by default (i.e. ASSERT).
3. Update doc= umentation for BaseLib functions to include the information
a= bout this behaviour.

The only thing in questio= n 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 PcdDebugPro= pertyMask is cleaner.
2. Extending DebugLib interface with a = new function is not a good idea.

Therefore I s= uggest:

1.Add #define DEBUG_PROPERTY_ASSERT_CO= NSTRAINT_ENABLED 0x40.
2. Create header-only macros to replac= e functions like
DebugAssertEnabled. We can then use these ma= cros in new code and
deprecate the original functions.
3. Enable DEBUG_PROPERTY_ASSERT_CONSTRAINT_ENABLED bit in MdeP= kg by default.

I will submit the new version o= f the patch soon unless there is an
immediate opposing opinio= n.

Not sur= e 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 t= he
design, since I was asked to.)

Thanks!
Laszlo
 
 

--Apple-Mail=_0D9601B1-61BE-4789-9830-DD768A7005CA-- --Apple-Mail=_18B8BA0C-A0E4-4CFB-A69B-43663718D1C8 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----- iQIzBAEBCgAdFiEEsLABAI5Y5VbvBdmpL8K2O86Eyz4FAl65PwUACgkQL8K2O86E yz7xPxAArEQPKDNrrIfT5VlQtkdofCPoACy52bF04cP+bechj5r9oy6mjzlJZubz K18V9xM1ZLLTRpv6STWcLQSTso40R+sX6HHxFk71lg6lNi7zY2iuOu5BpifRKnJ7 OFwGoa0GR/QTCp7vIHCtfsviLpWfEv0j4OXkIXUVcM+/7yRp8H2GtYv0yZARRPeR uG2Dsixe1slyfNUwyDqiu7r9CYtb1Gpi1OzqfhmotTepvzNWhIQO8UsM15UuNOT8 K46mFjzdh4byXNC16P19T1S0iYOyt9g9Bdc9VPFSNVzabP9L3DbMjfnbyBRiji0m f7LkePa7C1pVsDLDlJPH81hVDd/bgA2tS9zkGKbHyNbpA+zUg+bxEdvJ4pJiZzsR kzj/580Lt4oFfQsV/yjRoBVJ66MxhNsu2naHirdChhQ/X4i74cJEsCZCfCuBchyC 3LwjD0qgEfg++uPIOjelvFxZflDa3MP0jbcbRhGuqXly+KXUoCtp0pUj0gz6OWO5 uq1Y4Rgajkj94ZwhwkDWWpEoZEQTLrcRK0C4iXo9KWuMjzuJcyYLjbXgpS8jDbNL ZQ/rCdYhbwVtqGR2tA2Ycg632RQty1TvuJibI/Jgra/jfZBQboWcieiHWVk1T5bu CRyV86LRF+eaQ0H8DyBEOI5bGkScGWQxQFPsprNZZs5r7XPOzQk= =kgHC -----END PGP SIGNATURE----- --Apple-Mail=_18B8BA0C-A0E4-4CFB-A69B-43663718D1C8--