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.1079.1584576289489527237 for ; Wed, 18 Mar 2020 17:04:50 -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 1CFD864F; Thu, 19 Mar 2020 03:04:47 +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 03:04:45 +0300 Message-Id: <338E2F04-B523-473B-B1A4-AD855B8311DE@ispras.ru> References: <4219F456-E97B-403F-80D1-07EA07CE8549@apple.com> Cc: devel@edk2.groups.io, Mike Kinney , Laszlo Ersek , =?utf-8?Q?Marvin_H=C3=A4user?= , "Gao, Liming" , "Gao, Zhichao" In-Reply-To: <4219F456-E97B-403F-80D1-07EA07CE8549@apple.com> To: Andrew Fish X-Mailer: iPad Mail (17D50) Content-Type: multipart/alternative; boundary=Apple-Mail-E5DBA04C-44B5-49F8-8F33-E1C88BC2A88B Content-Transfer-Encoding: 7bit --Apple-Mail-E5DBA04C-44B5-49F8-8F33-E1C88BC2A88B Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Andrew, Mike, 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 co= nfigure Shell, inject and override some of its libraries with different set= tings. >>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 li= brary AutoGen.c). 2. AutoGen.h of the library itself (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. >>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 see= any change in the PCD overriding functionality if they land. The only down= side I can imagine is a theoretical performance penalty, but this does not = seem to be a design problem. Such things if they arise are best to be resol= ved by an alternative implementation of the build tools. The limitation of not building a separate library is indeed somewhat a pro= blem, 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, and 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. Unfortunately, since I have no good grasp of its architec= ture it will likely take long for me to prepare a solution and ensure that = it does not break things for anyone. If there is no-one who can handle it b= y the next stable tag I could imagine going with the library route and perh= aps filing a feature request in the bugzilla, so that is not forgotten. Does the approach of splitting DebugLib into common and implementation par= ts sound good to both of you? I believe you should have a number of custom = DebugLib implementations. While this approach is not as good as the origina= l macro route (especially for compilers without LTO), it should still let e= veryone add more changes to PCD sets and other shared debugging parts witho= ut the need to change DebugLib implementations after the first and the only= transition. Best regards, Vitaly > 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 t= o override PCD settings per module in the DSC file. So libraries need to ei= ther derive their PCD value from the driver/app they are linking with, or w= e would need to build different instances of the library with the different= PCD defaults and link the correct one. The build system does not support b= uilding extra copies of the libraries so we have the restriction Mike menti= oned.=20 >=20 > https://github.com/tianocore/edk2/blob/master/OvmfPkg/OvmfPkgX64.dsc#L85= 6 > ShellPkg/Application/Shell/Shell.inf { > > ShellCommandLib|ShellPkg/Library/UefiShellCommandLib/UefiShellComm= andLib.inf > NULL|ShellPkg/Library/UefiShellLevel2CommandsLib/UefiShellLevel2Co= mmandsLib.inf > NULL|ShellPkg/Library/UefiShellLevel1CommandsLib/UefiShellLevel1Co= mmandsLib.inf > NULL|ShellPkg/Library/UefiShellLevel3CommandsLib/UefiShellLevel3Co= mmandsLib.inf > NULL|ShellPkg/Library/UefiShellDriver1CommandsLib/UefiShellDriver1= CommandsLib.inf > NULL|ShellPkg/Library/UefiShellDebug1CommandsLib/UefiShellDebug1Co= mmandsLib.inf > NULL|ShellPkg/Library/UefiShellInstall1CommandsLib/UefiShellInstal= l1CommandsLib.inf > NULL|ShellPkg/Library/UefiShellNetwork1CommandsLib/UefiShellNetwor= k1CommandsLib.inf > !if $(NETWORK_IP6_ENABLE) =3D=3D TRUE > NULL|ShellPkg/Library/UefiShellNetwork2CommandsLib/UefiShellNetwor= k2CommandsLib.inf > !endif > HandleParsingLib|ShellPkg/Library/UefiHandleParsingLib/UefiHandleP= arsingLib.inf > PrintLib|MdePkg/Library/BasePrintLib/BasePrintLib.inf > BcfgCommandLib|ShellPkg/Library/UefiShellBcfgCommandLib/UefiShellB= cfgCommandLib.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. Libr= aries, described with INF files, consume PCDs and potentially override thei= r values. DEC files produce PCDs, which libraries or modules (drivers, appi= cations) can consume. Header-only libraries have no INF files, and thus are= not really libraries one can depend on, and thus can have no PCDs. I canno= t make a connection of how a library consuming a PCD could influence on a D= EC file. >>=20 >> BaseTools' AutoGen implements DependentLibraryList and LibraryPcdList p= roperties, which effectively gather all library PCDs for a module. So they = already have all the information about the PCDs used and needed to be added= to AutoGen.c and AutoGen.h. >>=20 >> I expected them to add library PCD definitions to AutoGen.h for modules= , but for some reason it does not happen. They also explicitly skip PCD dep= endency walk for libraries, which I assumed to be some questionable perform= ance optimisation before I realised that they are not exposed for the forme= r 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 rea= sonably few changes in BaseTools for a person that is well aware of their c= odebase. Could you give a better insight on this or perhaps ask somebody wh= o knows BaseTools internals? >>=20 >> If you believe it is much worse than I see, I can just trust you for th= e time being and focus on implementing an alternative approach by separatin= g 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, >>> >>> 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 clas= s, 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 lib= rary instances. We do not have a feature that allows a library class (whic= h only has a .h file and a one line declaration in a DEC file) to provide e= xtra 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 library= class 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 Fis= h ; 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 th= e code of the BaseTools kind of gave me such a suspect. >>> Is there any particular reason why this limitation was added? At the m= oment I do not see a good reason why this is done. >>> >>> If there is one, I guess we could consider some other approach, for ex= ample, we can factor out these functions to a separate DebugHelperLib/Debug= BaseLib/DebugCommonLib, which every DebugLib will depend on. This will make= 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 Debug= Lib 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 >>>=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): >>> >>> Vitaly, >>> >>> The break you are seeing is because you are not using functions to eva= l the PCD. This is a known restriction in how PCDs work between libs and m= odules 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 i= f there are any attributes of newer compilers that would allow a different = design now. >>> >>> Best regards, >>> >>> Mike >>> >>> From: devel@edk2.groups.io On Behalf Of Vitaly = Cheptsov >>> Sent: Wednesday, March 18, 2020 12:36 PM >>> To: Laszlo Ersek ; Andrew Fish ; K= inney, 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 = 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 DebugLib, like PcdDebugP= ropertyMask. >>> - Any library implementing DebugLib should now depend on these PCDs, w= hich seems fairly natural (and I fixed that in BaseDebugLibNull). >>> - Any library using DebugLib header should depend on DebugLib, which a= lso depend on DebugLib to get its PCDs (that already looks fine). >>> >>> However, for some reason DebugLib PCDs are not included in Autogen.h h= eader for other libraries some reason, and we get errors like: >>> MdePkg/Library/BaseOrderedCollectionRedBlackTreeLib/BaseOrderedCollect= ionRedBlackTreeLib.c:1151:9: error: use of undeclared identifier '_PCD_GET_= MODE_8_PcdDebugPropertyMask' >>> >>> I am not familiar with the build system well enough to resolve this, s= o I either need guidance on where to look first or it will be great if some= body else handles that. >>> I do not believe it is a great idea to abandon the idea of dropping De= bugAssertEnabled-like functions, so I suggest us to focus on resolving the = build 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 = 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): >>> >>> 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 informati= on >>> 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 de= fault. >>>=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 abov= e >>> 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-E5DBA04C-44B5-49F8-8F33-E1C88BC2A88B Content-Type: text/html; charset=utf-8 Content-Transfer-Encoding: quoted-printable
Andrew, Mike,

Thank you very much for the comme= nts. 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 settings.

<= /div>
From what I understand the library PCD values should = be put to:
1. AutoGen.c of each application/driver bu= ilt (as a value; *not* to the library AutoGen.c).
2. = AutoGen.h of the library itself (as an extern).
3. Au= toGen.h of the dependent library that depends on the library claiming to us= e the PCD.
4. AutoGen.h of the application/driver.

From what I understand, 1 an= d 2 ar= e already done by the EDK II BaseTools. So, currently the only thing= s that need to happen are 3 and 4. I do not see any change in the PCD overriding = functionality if they land. The only downside I can imagine is = a theoretical performance penalty, but this does not seem to be a design pr= oblem. Such things if they arise are best to be resolved by an alternative = implementation of the build tools.

The limitation of not building a separate library is indeed somew= hat a problem, as it collides with fixed PCDs. I.e. we cannot override fixe= d PCDs in the DSC for a particular application, as the library is already b= uilt, and fixed PCDs are evaluated during preprocessing/library compilation= . However, nothing changes here, and I assume it can be continued to live w= ith.

Like I said, for a pe= rson like me it seems like a relatively minor change in the BaseTools. Unfo= rtunately, since I have no good grasp of its architecture it will likely ta= ke long for me to prepare a solution and ensure that it does not break thin= gs 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 perhaps filing a feature r= equest in the bugzilla, so that is not forgotten.
Does the approach of splitting DebugLib into common= and implementation parts sound good to both of you? I believe you should h= ave a number of custom DebugLib implementations. While this approach is not= as good as the original macro route (especially for compilers without LTO)= , it should still let everyone add more changes to PCD sets and other share= d debugging parts without the need to change 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 shared bet= ween modules. If is possible to override PCD settings per module in the DSC= file. So libraries need to either derive their PCD value from the driver/a= pp they are linking with, or we would need to build different instances of = the library with the different PCD defaults and link the correct one. The b= uild system does not support building extra copies of the libraries so we h= ave the restriction Mike mentioned. 

https://github.com/tianocore= /edk2/blob/master/OvmfPkg/OvmfPkgX64.dsc#L856
ShellPkg/Application/Shell/Shell.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 Ma= r 18, 2020, at 2:31 PM, Vitaly Cheptsov <cheptsov@ispras.ru> wrote:

Mike,

That explains the current behaviour, but makes me even more confused.

<= div dir=3D"ltr" style=3D"caret-color: rgb(0, 0, 0); font-family: Helvetica;= font-size: 12px; font-style: normal; font-variant-caps: normal; font-weigh= t: normal; letter-spacing: normal; text-align: start; text-indent: 0px; tex= t-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-str= oke-width: 0px; text-decoration: none;" class=3D"">I do not really understa= nd how DEC format is responsible for this. Libraries, described with INF fi= les, consume PCDs and potentially override their values. DEC files produce = PCDs, which libraries or modules (drivers, appications) can consume. Header= -only libraries have no INF files, and thus are not really libraries one ca= n depend on, and thus can have no PCDs. I cannot make a connection of how a= library consuming a PCD could influence on a DEC file.

BaseTools' AutoGen implements Dependent= LibraryList and LibraryPcdList properties, which effectively gather all lib= rary PCDs for a module. So they already 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, but for some reason it does not = happen. They also explicitly skip PCD dependency walk for libraries, which = I assumed to be some questionable performance optimisation before I realise= d that they are not exposed for the former case as well.

It is very possible that I miss someth= ing, but to me it looks like the fact that we cannot see library PCDs in mo= dules and higher level libraries is just an artificial limitation, which sh= ould be possible to lift with reasonably few changes in BaseTools for a per= son that is well aware of their codebase. Could you give a better insight o= n this or perhaps ask somebody who knows BaseTools internals?

If you believe it is much wors= e than I see, I can just trust you for the time being and focus on implemen= ting 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:

=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 using the PCDs from the library implementation, the library implement= ation INF declares the PCDs it uses and the module inherits the PCDs from t= he library instances.  We do not have a feature that allows a library c= lass (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.  W= e would need a significant extension to the DEC file format and build tools= for a library class declaration to provide more information.
 
Mike
 
= From: Vitaly Cheptsov <cheptsov@ispras.ru> 
Sent:=  Wednesday, March 18, 202= 0 1:43 PM
To: Kinney, Michael D <michael.d.kinney@intel.com>
<= b class=3D"">Cc: devel@edk2.groups.io; La= szlo Ersek <lersek@redha= t.com>; Andrew Fish <afish@apple.com>; Marvin H=C3=A4user <mhaeuser@outlook.de>; Gao, Liming <liming.gao@intel.com>;= Gao, Zhichao <zhich= ao.gao@intel.com>
Subject: Re: [edk2-devel] Disabling safe st= ring constraint assertions
 
Mike,
&nb= sp;
Thanks for the clarification. I failed to find it in the spec= s, but the code of the BaseTools kind of gave me such a suspect.
Is there any particular reason why this limitation was ad= ded? At the moment I do not see a good reason why this is done.
 
If there is one, I gues= s we could consider some other approach, for example, we can factor out the= se functions to a separate DebugHelperLib/DebugBaseLib/DebugCommonLib, whic= h every DebugLib will depend on. This will make 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 DebugLib implementations and upda= te them for a minor Pcd change.
&= nbsp;
<= span class=3D"">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 comp= iler issue could arise here with normal PCDs.<= /div>
 
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 PCD.  This is a known restriction in how PCDs work between libs an= d modules and is why the current design uses the XxxEnabled() function= s.
 
I have not re= viewed this issue in a very long time, so I do not know if there are any at= tributes 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 Cheptsov
Sent: Wednesd= ay, March 18, 2020 12:36 PM
To: Laszlo Ersek <lersek@redhat.com= >; Andrew Fish <afish@apple.com>; Kinney, Michael D <mi= chael.d.kinney@intel.com>; Marvin H=C3=A4user <mhaeuser@outlook= .de>; Gao, Liming <liming.gao@intel.com>; Gao, Z= hichao <zhichao.gao@intel.com>
Cc= : devel@edk2.groups.io
Subject: Re: [edk2-devel] Disabling safe string constraint = assertions
 <= /o:p>
Hello!
 
I have a prot= otype of the patch, but there currently is an issue with the current EDK II= build system.
I attached t= he patch to this e-mail, however, it will not compile for reasonably obscur= e causes.
 
From what I understand:
- DebugLib header now directly=  uses PCDs from DebugLib, like PcdDebugPropertyMask.
- Any library implementing DebugLib should n= ow depend on these PCDs, which seems fairly natural (and I fixed that in Ba= seDebugLibNull).
- Any l= ibrary using DebugLib header should depend on DebugLib, which also depend o= n DebugLib to get its PCDs (that already looks fine).=
 
<= /div>
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/BaseOrderedCollectio= nRedBlackTreeLib/BaseOrderedCollectionRedBlackTreeLib.c:1151:9: error: use = of undeclared identifier '_PCD_GET_MODE_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 b= e great if somebody else handles that.
I do not believe it is a great idea to abandon the idea of drop= ping DebugAssertEnabled-like functions, so I suggest us to focus on re= solving the build system limitation rather than trying a new approach.
<= div style=3D"margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibr= i, sans-serif;" class=3D""> 
Best regards,
Vitaly
 <= o:p 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 ev= eryone,

So, I believe that by now we mostly ag= reed to let the original
proposition land as a short-term sol= ution. We end up with:

1. A PCD condition with= in SAFE_STRING_COSTRAINT_CHECK macro.
2. Make this condition = evaluate to TRUE by default (i.e. ASSERT).
3. Update document= ation 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 PcdD= ebugPropertyMask. I believe that we almost agreed on
two thin= gs:

1. Adding an extra bit to PcdDebugProperty= Mask is cleaner.
2. Extending DebugLib interface with a new f= unction is not a good idea.

Therefore I sugges= t:

1.Add #define DEBUG_PROPERTY_ASSERT_CONSTRA= INT_ENABLED 0x40.
2. Create header-only macros to replace fun= ctions 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= default.

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

Not sure abo= ut any particular deprecation timeline, but to me the above
c= ertainly 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-E5DBA04C-44B5-49F8-8F33-E1C88BC2A88B--