[edk2-devel] Disabling safe string constraint assertions

Wed Mar 18 20:55:26 UTC 2020

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 <cheptsov at ispras.ru>
Sent: Wednesday, March 18, 2020 1:43 PM
To: Kinney, Michael D <michael.d.kinney at intel.com>
Cc: devel at edk2.groups.io; Laszlo Ersek <lersek at redhat.com>; Andrew Fish <afish at apple.com>; Marvin Häuser <mhaeuser at outlook.de>; Gao, Liming <liming.gao at intel.com>; Gao, Zhichao <zhichao.gao at 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.kinney at intel.com<mailto:michael.d.kinney at 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 at edk2.groups.io<mailto:devel at edk2.groups.io> <devel at edk2.groups.io<mailto:devel at edk2.groups.io>> On Behalf Of Vitaly Cheptsov
Sent: Wednesday, March 18, 2020 12:36 PM
To: Laszlo Ersek <lersek at redhat.com<mailto:lersek at redhat.com>>; Andrew Fish <afish at apple.com<mailto:afish at apple.com>>; Kinney, Michael D <michael.d.kinney at intel.com<mailto:michael.d.kinney at intel.com>>; Marvin Häuser <mhaeuser at outlook.de<mailto:mhaeuser at outlook.de>>; Gao, Liming <liming.gao at intel.com<mailto:liming.gao at intel.com>>; Gao, Zhichao <zhichao.gao at intel.com<mailto:zhichao.gao at intel.com>>
Cc: devel at edk2.groups.io<mailto:devel at 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 at redhat.com<mailto:lersek at 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 (#55965): https://edk2.groups.io/g/devel/message/55965
Mute This Topic: https://groups.io/mt/71711587/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/edk2-devel-archive/attachments/20200318/8e78e002/attachment.htm>