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 написал(а): > > 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 > On Behalf Of Vitaly Cheptsov > Sent: Wednesday, March 18, 2020 12:36 PM > To: Laszlo Ersek >; Andrew Fish >; Kinney, Michael D >; Marvin Häuser >; 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 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 > написал(а): > > 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 > >