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 (#59096): https://edk2.groups.io/g/devel/message/59096 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] -=-=-=-=-=-=-=-=-=-=-=-
