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>
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 марта 2020 г., в 03:04, Vitaly Cheptsov <[email protected]> написал(а): > > 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 <[email protected]> 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 <[email protected] >>> <mailto:[email protected]>> 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 <[email protected] >>>> <mailto:[email protected]>> 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 <[email protected] <mailto:[email protected]>> >>>> Sent: Wednesday, March 18, 2020 1:43 PM >>>> To: Kinney, Michael D <[email protected] >>>> <mailto:[email protected]>> >>>> Cc: [email protected] <mailto:[email protected]>; Laszlo Ersek >>>> <[email protected] <mailto:[email protected]>>; Andrew Fish >>>> <[email protected] <mailto:[email protected]>>; Marvin Häuser >>>> <[email protected] <mailto:[email protected]>>; Gao, Liming >>>> <[email protected] <mailto:[email protected]>>; Gao, Zhichao >>>> <[email protected] <mailto:[email protected]>> >>>> 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 <[email protected] >>>> <mailto:[email protected]>> написал(а): >>>> >>>> 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: [email protected] <mailto:[email protected]> >>>> <[email protected] <mailto:[email protected]>> On Behalf Of Vitaly >>>> Cheptsov >>>> Sent: Wednesday, March 18, 2020 12:36 PM >>>> To: Laszlo Ersek <[email protected] <mailto:[email protected]>>; Andrew >>>> Fish <[email protected] <mailto:[email protected]>>; Kinney, Michael D >>>> <[email protected] <mailto:[email protected]>>; Marvin >>>> Häuser <[email protected] <mailto:[email protected]>>; Gao, Liming >>>> <[email protected] <mailto:[email protected]>>; Gao, Zhichao >>>> <[email protected] <mailto:[email protected]>> >>>> Cc: [email protected] <mailto:[email protected]> >>>> 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 <[email protected] >>>> <mailto:[email protected]>> напиÑал(а): >>>> >>>> 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 (#59068): https://edk2.groups.io/g/devel/message/59068 Mute This Topic: https://groups.io/mt/71711587/21656 Group Owner: [email protected] Unsubscribe: https://edk2.groups.io/g/devel/unsub [[email protected]] -=-=-=-=-=-=-=-=-=-=-=-
signature.asc
Description: Message signed with OpenPGP
