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.14518.1589211340280796272 for ; Mon, 11 May 2020 08:35:41 -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 63F2CCD464; Mon, 11 May 2020 18:35:35 +0300 (MSK) From: "Vitaly Cheptsov" Message-Id: <50D19DA8-550C-4886-9B42-345D2ACC415D@ispras.ru> 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 18:35:34 +0300 In-Reply-To: Cc: Andrew Fish , Mike Kinney , =?utf-8?Q?Marvin_H=C3=A4user?= , "Gao, Liming" , "Gao, Zhichao" , devel@edk2.groups.io To: Laszlo Ersek 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: 59097 Content-Type: multipart/signed; boundary="Apple-Mail=_DA650754-A863-4389-A039-BA57C4A314D7"; protocol="application/pgp-signature"; micalg=pgp-sha512 --Apple-Mail=_DA650754-A863-4389-A039-BA57C4A314D7 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 Laszlo, Thanks for the quick response. I am unsure whether EDK II mailing list wil= l be happy with 30 letters but I will batch-send it now. The patch is hardl= y a feature but rather a longstanding, which was scheduled for May release = in any case. I believe we discussed most of the details by this time, and I= hope only some small nuances are left. Best wishes, Vitaly > 11 =D0=BC=D0=B0=D1=8F 2020 =D0=B3., =D0=B2 18:19, Laszlo Ersek =D0=BD=D0=B0=D0=BF=D0=B8=D1=81=D0=B0=D0=BB(=D0=B0): >=20 > On 05/11/20 14:03, Vitaly Cheptsov wrote: >> Hello, >>=20 >> The new version of the patchset was submitted via github (mainly due to= the amount of patches to avoid spamming the mailing list): >> https://github.com/tianocore/edk2/pull/601 >=20 > github pull requests are only used -- at this time -- by contributors > for personal CI runs, and by edk2 maintainers for merging series that > have been reviewed on the list. Patch review remains mailing list-based, > for now. Please post the patches to the list for review. >=20 >> Let me know if any further changes are needed from my side. I hope this= still is in time for the May tag. >=20 > If this work counts as a feature, then its review has to complete by the > soft feature freeze (2020-05-15). >=20 > Thanks, > Laszlo >=20 >>=20 >> Best wishes, >> Vitaly >>=20 >>> 19 =D0=BC=D0=B0=D1=80=D1=82=D0=B0 2020 =D0=B3., =D0=B2 03:04, Vitaly C= heptsov =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 overridin= g in the DSC file, and in fact we are using it for the exact same purpose t= o configure Shell, inject and override some of its libraries with different= settings. >>>=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 th= e 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 clai= ming 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 BaseToo= ls. 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 = downside 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 r= esolved by an alternative implementation of the build tools. >>>=20 >>> The limitation of not building a separate library is indeed somewhat a= problem, as it collides with fixed PCDs. I.e. we cannot override fixed PCD= s in the DSC for a particular application, as the library is already built,= and fixed PCDs are evaluated during preprocessing/library compilation. How= ever, 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 cha= nge in the BaseTools. Unfortunately, since I have no good grasp of its arch= itecture it will likely take long for me to prepare a solution and ensure t= hat 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 = perhaps filing a feature request in the bugzilla, so that is not forgotten. >>>=20 >>> Does the approach of splitting DebugLib into common and implementation= parts sound good to both of you? I believe you should have a number of cus= tom DebugLib implementations. While this approach is not as good as the ori= ginal macro route (especially for compilers without LTO), it should still l= et everyone add more changes to PCD sets and other shared debugging parts w= ithout the need to change DebugLib implementations after the first and the = only 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 possibl= e to override PCD settings per module in the DSC file. So libraries need to= either derive their PCD value from the driver/app they are linking with, o= r we would need to build different instances of the library with the differ= ent PCD defaults and link the correct one. The build system does not suppor= t building extra copies of the libraries so we have the restriction Mike me= ntioned. >>>>=20 >>>> https://github.com/tianocore/edk2/blob/master/OvmfPkg/OvmfPkgX64.dsc#= L856 >>>> ShellPkg/Application/Shell/Shell.inf { >>>> >>>> ShellCommandLib|ShellPkg/Library/UefiShellCommandLib/UefiShellCo= mmandLib.inf >>>> NULL|ShellPkg/Library/UefiShellLevel2CommandsLib/UefiShellLevel2= CommandsLib.inf >>>> NULL|ShellPkg/Library/UefiShellLevel1CommandsLib/UefiShellLevel1= CommandsLib.inf >>>> NULL|ShellPkg/Library/UefiShellLevel3CommandsLib/UefiShellLevel3= CommandsLib.inf >>>> NULL|ShellPkg/Library/UefiShellDriver1CommandsLib/UefiShellDrive= r1CommandsLib.inf >>>> NULL|ShellPkg/Library/UefiShellDebug1CommandsLib/UefiShellDebug1= CommandsLib.inf >>>> NULL|ShellPkg/Library/UefiShellInstall1CommandsLib/UefiShellInst= all1CommandsLib.inf >>>> NULL|ShellPkg/Library/UefiShellNetwork1CommandsLib/UefiShellNetw= ork1CommandsLib.inf >>>> !if $(NETWORK_IP6_ENABLE) =3D=3D TRUE >>>> NULL|ShellPkg/Library/UefiShellNetwork2CommandsLib/UefiShellNetw= ork2CommandsLib.inf >>>> !endif >>>> HandleParsingLib|ShellPkg/Library/UefiHandleParsingLib/UefiHandl= eParsingLib.inf >>>> PrintLib|MdePkg/Library/BasePrintLib/BasePrintLib.inf >>>> BcfgCommandLib|ShellPkg/Library/UefiShellBcfgCommandLib/UefiShel= lBcfgCommandLib.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. L= ibraries, described with INF files, consume PCDs and potentially override t= heir values. DEC files produce PCDs, which libraries or modules (drivers, a= ppications) 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 ca= nnot make a connection of how a library consuming a PCD could influence on = a DEC file. >>>>>=20 >>>>> BaseTools' AutoGen implements DependentLibraryList and LibraryPcdLis= t properties, which effectively gather all library PCDs for a module. So th= ey already have all the information about the PCDs used and needed to be ad= ded to AutoGen.c and AutoGen.h. >>>>>=20 >>>>> I expected them to add library PCD definitions to AutoGen.h for modu= les, but for some reason it does not happen. They also explicitly skip PCD = dependency walk for libraries, which I assumed to be some questionable perf= ormance optimisation before I realised that they are not exposed for the fo= rmer case as well. >>>>>=20 >>>>> It is very possible that I miss something, but to me it looks like t= he fact that we cannot see library PCDs in modules and higher level librari= es 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 thei= r codebase. Could you give a better insight on this or perhaps ask somebody= who knows BaseTools internals? >>>>>=20 >>>>> If you believe it is much worse than I see, I can just trust you for= the time being and focus on implementing an alternative approach by separa= ting 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 c= lass, the module using that library class does not know there is a macro th= at uses a PCD. So the PCD declaration in the Module INF is missing. By on= ly using the PCDs from the library implementation, the library implementati= on INF declares the PCDs it uses and the module inherits the PCDs from the = library instances. We do not have a feature that allows a library class (w= hich only has a .h file and a one line declaration in a DEC file) to provid= e extra information such as PCDs that the library class uses. We would nee= d a significant extension to the DEC file format and build tools for a libr= ary 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 Erse= k >; Andrew Fish >; Marvin H=C3=A4user >; Gao, Liming >; Gao, Zhichao > >>>>>> Subject: Re: [edk2-devel] Disabling safe string constraint assertio= ns >>>>>>=20 >>>>>> Mike, >>>>>>=20 >>>>>> 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 th= e 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= example, we can factor out these functions to a separate DebugHelperLib/De= bugBaseLib/DebugCommonLib, which every DebugLib will depend on. This will m= ake sense to me as a workaround of such limitation, as neither us, nor Andr= ew, as he mentioned previously, are happy of having to duplicate code in De= bugLib implementations and update them for a minor Pcd change. >>>>>>=20 >>>>>> If there is no good reason, to be honest, it feels like we should j= ust fix this. After reading the spec I do not see what kind of compiler iss= ue 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, Kinne= y, 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 = 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() functions. >>>>>>=20 >>>>>> I have not reviewed this issue in a very long time, so I do not kno= w if there are any attributes of newer compilers that would allow a differe= nt 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 >; An= drew Fish >; Kinney, Michael D >; Marvin H=C3= =A4user >; Gao, Liming >; Gao, Zhichao > >>>>>> Cc: devel@edk2.groups.io >>>>>> Subject: Re: [edk2-devel] Disabling safe string constraint assertio= ns >>>>>>=20 >>>>>> Hello! >>>>>>=20 >>>>>> I have a prototype of the patch, but there currently is an issue wi= th the current EDK II build system. >>>>>> I attached the patch to this e-mail, however, it will not compile f= or reasonably obscure causes. >>>>>>=20 >>>>>> From what I understand: >>>>>> - DebugLib header now directly uses PCDs from DebugLib, like PcdDeb= ugPropertyMask. >>>>>> - 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, whic= h 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/BaseOrderedColl= ectionRedBlackTreeLib.c:1151:9: error: use of undeclared identifier '_PCD_G= ET_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 s= omebody 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 t= he 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 inform= ation >>>>>> 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 agre= ed 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 i= dea. >>>>>>=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= default. >>>>>>=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 a= bove >>>>>> 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 >>>>>=20 >>>>=20 >>=20 >>=20 >=20 --Apple-Mail=_DA650754-A863-4389-A039-BA57C4A314D7 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----- iQIzBAEBCgAdFiEEsLABAI5Y5VbvBdmpL8K2O86Eyz4FAl65cMYACgkQL8K2O86E yz4VzA//aQqdFcqZNtS8nT4JmQUqKTRX4rJRbCsV08XQBdIkTjHBhEuFatYaPr51 VG10t0U4jXywoIvwCys3Gq2IW7YYLFcpvAmFJ3CIkNU8SYFbjZ9D5ieM9mI9XWXP F99bU2qMgAtoKL9XmA9pOy26hNFIaX8ZGasHPt52A9aonITHh1riVW6RXMTcSxA+ 3J/TdGuhERtQJfz7Khf1DNDWUNHdzkS3Oq5decSCDZ8hvb7vbquUOs+RvtTIrJz2 Hzn6/pN8qi+uPOdDdVycGJut1Vh/jvOss5m8BRJ+bXfiG/erJNtmtlj72+Lnw8F5 Us8uAjMlR6tjUGiIZraTCddY8SCrUx8X4DfxshSLVgBpSLvMWyzkhMnwWMw9QThB aPrv4v+ctgWF4T6bhS6igVWgcdm3lI63NYd1AK0H/C946+08t9BepAk3MgEVRLDy KGcjKe4aGPOx5rUc7EGixnMheEQHdUiHMsKXV/3Rxfi0n8JqBBXXBb6GsHUZXzAV DpU97S13Oa3fdOr9a1mHJx/fiac9MOKq4Y70NLcSiecviEiEAkOBp9G7MHx8cY+G TIlG88tK1xTha2tJwSM2J+ydkzbgtvtyUtsnV/PniVEmvlVWYhkX8wjdhzBMG7Rd 7Qa2T98T1csBszmPDH9dDVjA1I8alvX25dJX/8h4VYUdqgqmbWQ= =hHQR -----END PGP SIGNATURE----- --Apple-Mail=_DA650754-A863-4389-A039-BA57C4A314D7--