Re: [edk2-devel] Disabling safe string constraint assertions

["Vitaly Cheptsov" <cheptsov@ispras.ru>]

[-- Attachment #1.1: Type: text/plain, Size: 5156 bytes --]

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.kinney@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 <lersek@redhat.com <mailto:lersek@redhat.com>>; Andrew Fish <afish@apple.com <mailto:afish@apple.com>>; Kinney, Michael D <michael.d.kinney@intel.com <mailto:michael.d.kinney@intel.com>>; Marvin Häuser <mhaeuser@outlook.de <mailto:mhaeuser@outlook.de>>; Gao, Liming <liming.gao@intel.com <mailto:liming.gao@intel.com>>; Gao, Zhichao <zhichao.gao@intel.com <mailto:zhichao.gao@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 <lersek@redhat.com <mailto:lersek@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
> 
> 


[-- Attachment #1.2: Type: text/html, Size: 15112 bytes --]

[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]