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> > написал(а): > > 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 (#55964): https://edk2.groups.io/g/devel/message/55964 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
