Laszlo, Thanks for the quick response. I am unsure whether EDK II mailing list will be happy with 30 letters but I will batch-send it now. The patch is hardly 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 мая 2020 г., в 18:19, Laszlo Ersek <ler...@redhat.com> написал(а): > > On 05/11/20 14:03, Vitaly Cheptsov wrote: >> Hello, >> >> 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 >> <https://github.com/tianocore/edk2/pull/601> > > 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. > >> Let me know if any further changes are needed from my side. I hope this >> still is in time for the May tag. > > If this work counts as a feature, then its review has to complete by the > soft feature freeze (2020-05-15). > > Thanks, > Laszlo > >> >> Best wishes, >> Vitaly >> >>> 19 марта 2020 г., в 03:04, Vitaly Cheptsov <chept...@ispras.ru> написал(а): >>> >>> 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 >>> 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. 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 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 >>> 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 >>> resolved by an alternative implementation of the build tools. >>> >>> 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 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 >>> architecture 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 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. >>> >>> 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 >>> 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 shared 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 <af...@apple.com> wrote: >>>> >>>> Vitaly, >>>> >>>> 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 >>>> either 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 >>>> 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/tianocore/edk2/blob/master/OvmfPkg/OvmfPkgX64.dsc#L856 >>>> <https://github.com/tianocore/edk2/blob/master/OvmfPkg/OvmfPkgX64.dsc#L856> >>>> ShellPkg/Application/Shell/Shell.inf { >>>> <LibraryClasses> >>>> >>>> ShellCommandLib|ShellPkg/Library/UefiShellCommandLib/UefiShellCommandLib.inf >>>> >>>> NULL|ShellPkg/Library/UefiShellLevel2CommandsLib/UefiShellLevel2CommandsLib.inf >>>> >>>> NULL|ShellPkg/Library/UefiShellLevel1CommandsLib/UefiShellLevel1CommandsLib.inf >>>> >>>> NULL|ShellPkg/Library/UefiShellLevel3CommandsLib/UefiShellLevel3CommandsLib.inf >>>> >>>> NULL|ShellPkg/Library/UefiShellDriver1CommandsLib/UefiShellDriver1CommandsLib.inf >>>> >>>> NULL|ShellPkg/Library/UefiShellDebug1CommandsLib/UefiShellDebug1CommandsLib.inf >>>> >>>> NULL|ShellPkg/Library/UefiShellInstall1CommandsLib/UefiShellInstall1CommandsLib.inf >>>> >>>> NULL|ShellPkg/Library/UefiShellNetwork1CommandsLib/UefiShellNetwork1CommandsLib.inf >>>> !if $(NETWORK_IP6_ENABLE) == TRUE >>>> >>>> NULL|ShellPkg/Library/UefiShellNetwork2CommandsLib/UefiShellNetwork2CommandsLib.inf >>>> !endif >>>> >>>> HandleParsingLib|ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.inf >>>> PrintLib|MdePkg/Library/BasePrintLib/BasePrintLib.inf >>>> >>>> BcfgCommandLib|ShellPkg/Library/UefiShellBcfgCommandLib/UefiShellBcfgCommandLib.inf >>>> >>>> <PcdsFixedAtBuild> >>>> gEfiMdePkgTokenSpaceGuid.PcdDebugPropertyMask|0xFF >>>> gEfiShellPkgTokenSpaceGuid.PcdShellLibAutoInitialize|FALSE >>>> gEfiMdePkgTokenSpaceGuid.PcdUefiLibMaxPrintBufferSize|8000 >>>> } >>>> >>>> >>>> Thanks, >>>> >>>> Andrew Fish >>>> >>>>> On Mar 18, 2020, at 2:31 PM, Vitaly Cheptsov <chept...@ispras.ru >>>>> <mailto:chept...@ispras.ru>> wrote: >>>>> >>>>> Mike, >>>>> >>>>> That explains the current behaviour, but makes me even more confused. >>>>> >>>>> I do not really understand how DEC format is responsible for this. >>>>> Libraries, described with INF files, 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 can 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 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 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 realised that they are not exposed for >>>>> the former case as well. >>>>> >>>>> 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 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 much worse than I see, I can just trust you for the >>>>> time 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 <michael.d.kin...@intel.com >>>>>> <mailto:michael.d.kin...@intel.com>> wrote: >>>>>> >>>>>> >>>>>> 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 >>>>>> 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 library 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 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 library class declaration to provide more >>>>>> information. >>>>>> >>>>>> Mike >>>>>> >>>>>> From: Vitaly Cheptsov <chept...@ispras.ru <mailto:chept...@ispras.ru>> >>>>>> Sent: Wednesday, March 18, 2020 1:43 PM >>>>>> To: Kinney, Michael D <michael.d.kin...@intel.com >>>>>> <mailto:michael.d.kin...@intel.com>> >>>>>> Cc: devel@edk2.groups.io <mailto:devel@edk2.groups.io>; Laszlo Ersek >>>>>> <ler...@redhat.com <mailto:ler...@redhat.com>>; Andrew Fish >>>>>> <af...@apple.com <mailto:af...@apple.com>>; Marvin Häuser >>>>>> <mhaeu...@outlook.de <mailto:mhaeu...@outlook.de>>; Gao, Liming >>>>>> <liming....@intel.com <mailto:liming....@intel.com>>; Gao, Zhichao >>>>>> <zhichao....@intel.com <mailto:zhichao....@intel.com>> >>>>>> 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 >>>>>> 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/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 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 марта 2020 г., в 23:35, Kinney, Michael D <michael.d.kin...@intel.com >>>>>> <mailto:michael.d.kin...@intel.com>> написал(а): >>>>>> >>>>>> 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 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. >>>>>> >>>>>> Best regards, >>>>>> >>>>>> Mike >>>>>> >>>>>> From: devel@edk2.groups.io <mailto:devel@edk2.groups.io> >>>>>> <devel@edk2.groups.io <mailto:devel@edk2.groups.io>> On Behalf Of Vitaly >>>>>> Cheptsov >>>>>> Sent: Wednesday, March 18, 2020 12:36 PM >>>>>> To: Laszlo Ersek <ler...@redhat.com <mailto:ler...@redhat.com>>; Andrew >>>>>> Fish <af...@apple.com <mailto:af...@apple.com>>; Kinney, Michael D >>>>>> <michael.d.kin...@intel.com <mailto:michael.d.kin...@intel.com>>; Marvin >>>>>> Häuser <mhaeu...@outlook.de <mailto:mhaeu...@outlook.de>>; Gao, Liming >>>>>> <liming....@intel.com <mailto:liming....@intel.com>>; Gao, Zhichao >>>>>> <zhichao....@intel.com <mailto:zhichao....@intel.com>> >>>>>> Cc: devel@edk2.groups.io <mailto: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 >>>>>> PcdDebugPropertyMask. >>>>>> - 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). >>>>>> >>>>>> 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/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 be great 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 марта 2020 г., в 16:14, Laszlo Ersek <ler...@redhat.com >>>>>> <mailto:ler...@redhat.com>> напиÑал(а): >>>>>> >>>>>> 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 >>>>>> default. >>>>>> >>>>>> 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 >>>>>> >>>>>> >>>>> >>>> >> >> > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#59097): https://edk2.groups.io/g/devel/message/59097 Mute This Topic: https://groups.io/mt/71711587/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
signature.asc
Description: Message signed with OpenPGP
