Disabling safe string constraint assertions

Disabling safe string constraint assertions

[Vitaly Cheptsov]

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

Hello,

I am creating a new thread for this issue, as for some reason half of my messages do not display in the web-interface and some of e-mail clients in the previous one. It seems that somebody sent broken HTML code to groups.io <http://groups.io/>, and it did not like to be quoted. It will be quite nice if all the messages sent are plain-text to avoid such issues in the future.

Back to the issue, we want to make constraint assertions (SAFE_STRING_CONSTRAINT_CHECK) in SafeString.c be optionally disabled in DEBUG builds, as they break our ability to use some of BaseLib interfaces to verify untrusted user data in UEFI Applications. The background of this problem is covered in a bugzilla[1], and was also extensively discussed in the last proposed patch discussion thread[2]. We want the solution to the problem to land in the next stable tag: edk2-stable202005, and can submit the patch.

Currently there is no clear decision on what approach to take, as there are three different proposals to implement:

1) Andrew Fish’s approach with C11-like constraint handler[3].
2) Marvin Häuser’s approach with return codes and constraint assertions being moved to the caller side[4].
3) My approach suggesting DebugLib interface simplification, permitting us to resolve the problem and keep backwards compatibility[5].

My personal opinion is that:

— Marvin’s way is very interesting, but it would require implementing code on the caller side basically for every call, and this would requires extra programming effort. It may also be uneasy to teach people how to do this right, and this does not strictly provide a good solution for situations, where nesting is more than 2 (i.e. when user code calls library code, which itself calls library code). Because of these downsides I am afraid it is impractical to adopt it in EDK II.
— Andrew’s way is reasonable from the C standard point of view, but it does not work quite right with reentrancy and I am not positive we need the dynamic handling, especially because of DEBUG/RELEASE build confusion. I explained this in my previous e-mail, which I attached to the end of this message as quote, since groups.io <http://groups.io/> ate the previous send. Other than that, I would say that I can implement this approach if there is no other option.
— My own approach makes sense, as it reduces the amount of code in every DebugLib and actually simplifies the development in the future. I believe currently this is the most reasonable route we can take, and suggest other parties to consider it.

Since the three of us each have their own suggestions, I ask Mike and others to rejoin our discussions and give their opinions quickly, so that we can choose, perhaps vote, and proceed with the implementation. Thanks for your dedication.

Best wishes,
Vitaly

[1] https://bugzilla.tianocore.org/show_bug.cgi?id=2054 <https://bugzilla.tianocore.org/show_bug.cgi?id=2054>
[2] https://edk2.groups.io/g/devel/topic/69401948 <https://edk2.groups.io/g/devel/topic/69401948>
[3] https://edk2.groups.io/g/devel/message/54520 <https://edk2.groups.io/g/devel/message/54520>
[4] https://edk2.groups.io/g/devel/message/54544 <https://edk2.groups.io/g/devel/message/54544>
[5] https://edk2.groups.io/g/devel/message/54499 <https://edk2.groups.io/g/devel/message/54499>


> Andrew,
> 
> I am not sure you received my message with the explanations[1], where I believe I covered the nuances, but it is nice to see you on the tune.
> 
> Making EDK II replicate more of the functionality from Bounds Checking Annex from C11 is an interesting idea. The historical part of our SafeStringLib is that it indeed tried to follow Microsoft's initial design (later contributed as a C standard Annex K) but completely ignored the ability to configure the handling logic. This is the problem we actually try to resolve here.
> 
> The reality behind this annex is, however, that very few implementations agreed to adopt it, including C standard library in macOS. The design of its functions permits some level of flexibility, but I think for most of their uses it is not strictly necessary or is not easily achievable. For example, one of the largest problems of this annex is that it entirely ignores threads or reentrancy. In EDK II we do not (yet) have threads in their true sense, but we do have events, which can interrupt code execution.
> 
> The code you presented is not strictly safe, as without TPL adjustment it may affect the behaviour of some random event handler. Thus the most likely use of the constraint handler configuration will be not changing it before the call to a function, but rather configuring close to the entry point of an application or a driver. For this exact reason it is questionable whether the need to have it configurable in runtime is actually needed. Or rather it is questionable whether anybody will actually use it and if he does, whether he will able to do it properly without shooting into the leg. Another issue of runtime configuration is binary size, but this I believe should be easy to avoid within macros body through pcds that disable assertions before calling the active handler.
> 
> All in all, I believe the suggested approach is also fine to us. However, I would personally suggest my approach, as I believe it is cleaner. For now let’s wait for Mike to comment before I consider starting the implementation.
> 
> Best wishes,
> Vitaly
> 
> [1] https://edk2.groups.io/g/devel/message/54499 <https://edk2.groups.io/g/devel/message/54499>


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

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

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

[Andrew Fish]

[-- Attachment #1: Type: text/plain, Size: 6602 bytes --]

Vitaly,

I was planing on bringing it up at the Stewards meeting that was scheduled for today, but it got rescheduled to next week. So if we don't get traction on the mailing list I'll bring it up in the meeting. 

> On Mar 3, 2020, at 1:12 PM, Vitaly Cheptsov <cheptsov@ispras.ru> wrote:
> 
> Hello,
> 
> I am creating a new thread for this issue, as for some reason half of my messages do not display in the web-interface and some of e-mail clients in the previous one. It seems that somebody sent broken HTML code to groups.io <http://groups.io/>, and it did not like to be quoted. It will be quite nice if all the messages sent are plain-text to avoid such issues in the future.
> 
> Back to the issue, we want to make constraint assertions (SAFE_STRING_CONSTRAINT_CHECK) in SafeString.c be optionally disabled in DEBUG builds, as they break our ability to use some of BaseLib interfaces to verify untrusted user data in UEFI Applications. The background of this problem is covered in a bugzilla[1], and was also extensively discussed in the last proposed patch discussion thread[2]. We want the solution to the problem to land in the next stable tag: edk2-stable202005, and can submit the patch.
> 
> Currently there is no clear decision on what approach to take, as there are three different proposals to implement:
> 
> 1) Andrew Fish’s approach with C11-like constraint handler[3].
> 2) Marvin Häuser’s approach with return codes and constraint assertions being moved to the caller side[4].
> 3) My approach suggesting DebugLib interface simplification, permitting us to resolve the problem and keep backwards compatibility[5].
> 
> My personal opinion is that:
> 
> — Marvin’s way is very interesting, but it would require implementing code on the caller side basically for every call, and this would requires extra programming effort. It may also be uneasy to teach people how to do this right, and this does not strictly provide a good solution for situations, where nesting is more than 2 (i.e. when user code calls library code, which itself calls library code). Because of these downsides I am afraid it is impractical to adopt it in EDK II.
> — Andrew’s way is reasonable from the C standard point of view, but it does not work quite right with reentrancy and I am not positive we need the dynamic handling, especially because of DEBUG/RELEASE build confusion. I explained this in my previous e-mail, which I attached to the end of this message as quote, since groups.io <http://groups.io/> ate the previous send. Other than that, I would say that I can implement this approach if there is no other option.

I agree that a save restore would be required to deal with TPL issues, so my initial API set was incomplete and I added the extra API. 

Given every EFI driver and application is bound to its own static library instance managing TPL is not too problematic in a practical programming sense. Usually it is hardware drivers that have timer events, to manage things like DMA. The most common TPL usage is preventing reentrancy in the protocols, but in this case after a driver is loaded it is generally always called indirectly via its protocol APIs. It is common for applications to not run at elevated TPL at all. 

Thanks,

Andrew Fish

> — My own approach makes sense, as it reduces the amount of code in every DebugLib and actually simplifies the development in the future. I believe currently this is the most reasonable route we can take, and suggest other parties to consider it.
> 
> Since the three of us each have their own suggestions, I ask Mike and others to rejoin our discussions and give their opinions quickly, so that we can choose, perhaps vote, and proceed with the implementation. Thanks for your dedication.
> 
> Best wishes,
> Vitaly
> 
> [1] https://bugzilla.tianocore.org/show_bug.cgi?id=2054 <https://bugzilla.tianocore.org/show_bug.cgi?id=2054>
> [2] https://edk2.groups.io/g/devel/topic/69401948 <https://edk2.groups.io/g/devel/topic/69401948>
> [3] https://edk2.groups.io/g/devel/message/54520 <https://edk2.groups.io/g/devel/message/54520>
> [4] https://edk2.groups.io/g/devel/message/54544 <https://edk2.groups.io/g/devel/message/54544>
> [5] https://edk2.groups.io/g/devel/message/54499 <https://edk2.groups.io/g/devel/message/54499>
> 
> 
>> Andrew,
>> 
>> I am not sure you received my message with the explanations[1], where I believe I covered the nuances, but it is nice to see you on the tune.
>> 
>> Making EDK II replicate more of the functionality from Bounds Checking Annex from C11 is an interesting idea. The historical part of our SafeStringLib is that it indeed tried to follow Microsoft's initial design (later contributed as a C standard Annex K) but completely ignored the ability to configure the handling logic. This is the problem we actually try to resolve here.
>> 
>> The reality behind this annex is, however, that very few implementations agreed to adopt it, including C standard library in macOS. The design of its functions permits some level of flexibility, but I think for most of their uses it is not strictly necessary or is not easily achievable. For example, one of the largest problems of this annex is that it entirely ignores threads or reentrancy. In EDK II we do not (yet) have threads in their true sense, but we do have events, which can interrupt code execution.
>> 
>> The code you presented is not strictly safe, as without TPL adjustment it may affect the behaviour of some random event handler. Thus the most likely use of the constraint handler configuration will be not changing it before the call to a function, but rather configuring close to the entry point of an application or a driver. For this exact reason it is questionable whether the need to have it configurable in runtime is actually needed. Or rather it is questionable whether anybody will actually use it and if he does, whether he will able to do it properly without shooting into the leg. Another issue of runtime configuration is binary size, but this I believe should be easy to avoid within macros body through pcds that disable assertions before calling the active handler.
>> 
>> All in all, I believe the suggested approach is also fine to us. However, I would personally suggest my approach, as I believe it is cleaner. For now let’s wait for Mike to comment before I consider starting the implementation. 
>> 
>> Best wishes,
>> Vitaly
>> 
>> [1] https://edk2.groups.io/g/devel/message/54499 <https://edk2.groups.io/g/devel/message/54499>
> 
> 


[-- Attachment #2: Type: text/html, Size: 10082 bytes --]

Re: Disabling safe string constraint assertions

[Laszlo Ersek]

On 03/03/20 22:12, Vitaly Cheptsov wrote:
> Hello,
> 
> I am creating a new thread for this issue, as for some reason half of my messages do not display in the web-interface and some of e-mail clients in the previous one. It seems that somebody sent broken HTML code to groups.io <http://groups.io/>, and it did not like to be quoted. It will be quite nice if all the messages sent are plain-text to avoid such issues in the future.
> 
> Back to the issue, we want to make constraint assertions (SAFE_STRING_CONSTRAINT_CHECK) in SafeString.c be optionally disabled in DEBUG builds, as they break our ability to use some of BaseLib interfaces to verify untrusted user data in UEFI Applications. The background of this problem is covered in a bugzilla[1], and was also extensively discussed in the last proposed patch discussion thread[2]. We want the solution to the problem to land in the next stable tag: edk2-stable202005, and can submit the patch.
> 
> Currently there is no clear decision on what approach to take, as there are three different proposals to implement:
> 
> 1) Andrew Fish’s approach with C11-like constraint handler[3].
> 2) Marvin Häuser’s approach with return codes and constraint assertions being moved to the caller side[4].
> 3) My approach suggesting DebugLib interface simplification, permitting us to resolve the problem and keep backwards compatibility[5].
> 
> My personal opinion is that:
> 
> — Marvin’s way is very interesting, but it would require implementing code on the caller side basically for every call, and this would requires extra programming effort. It may also be uneasy to teach people how to do this right, and this does not strictly provide a good solution for situations, where nesting is more than 2 (i.e. when user code calls library code, which itself calls library code). Because of these downsides I am afraid it is impractical to adopt it in EDK II.
> — Andrew’s way is reasonable from the C standard point of view, but it does not work quite right with reentrancy and I am not positive we need the dynamic handling, especially because of DEBUG/RELEASE build confusion. I explained this in my previous e-mail, which I attached to the end of this message as quote, since groups.io <http://groups.io/> ate the previous send. Other than that, I would say that I can implement this approach if there is no other option.
> — My own approach makes sense, as it reduces the amount of code in every DebugLib and actually simplifies the development in the future. I believe currently this is the most reasonable route we can take, and suggest other parties to consider it.
> 
> Since the three of us each have their own suggestions, I ask Mike and others to rejoin our discussions and give their opinions quickly, so that we can choose, perhaps vote, and proceed with the implementation. Thanks for your dedication.

I support the original proposal given in:

  https://bugzilla.tianocore.org/show_bug.cgi?id=2054#c0

also known as the v3 posting:

* [edk2-devel] [PATCH v3 1/1]
  MdePkg: Add PCD to disable safe string constraint assertions

https://edk2.groups.io/g/devel/message/52838
http://mid.mail-archive.com/20200103171242.63839-2-vit9696@protonmail.com

With the following tweak: I think we should rather introduce a new bit
in "PcdDebugPropertyMask" than an entirely new BOOLEAN PCD.

(Note that both PcdDebugPropertyMask and the propsed new PCD support
precisely the following PCD access methods: PcdsFixedAtBuild,
PcdsPatchableInModule.)

I don't feel too strongly about this part (i.e., whether we introduce a
new PCD, or a new bit in PcdDebugPropertyMask), as long as we make sure
that the access method can *never* become dynamic.

I do prefer quite strongly the original proposal, at a higher level.

Here's why I support the original proposal:

- In a brand new code base (or brand new set of APIs), I would fight
tooth and nail to keep such ASSERT()s out of the *base* APIs. It's up to
the caller to check return values. Also, additional wrapper
functions/macros can be offered *on top* of the base APIs that
internalize the ASSERT()s for special use cases.

But this ship has sailed, for edk2.

- I'm unfriendly towards callbacks. They make the behavior of code
implicit rather than explicit, and implicit is more difficult to reason
about. IMO callbacks should be considered a last resort only.

Just my two cents, because I've been asked to comment.

Thanks
Laszlo

Re: Disabling safe string constraint assertions

[Andrew Fish]

> On Mar 4, 2020, at 5:33 AM, Laszlo Ersek <lersek@redhat.com> wrote:
> 
> On 03/03/20 22:12, Vitaly Cheptsov wrote:
>> Hello,
>> 
>> I am creating a new thread for this issue, as for some reason half of my messages do not display in the web-interface and some of e-mail clients in the previous one. It seems that somebody sent broken HTML code to groups.io <http://groups.io/>, and it did not like to be quoted. It will be quite nice if all the messages sent are plain-text to avoid such issues in the future.
>> 
>> Back to the issue, we want to make constraint assertions (SAFE_STRING_CONSTRAINT_CHECK) in SafeString.c be optionally disabled in DEBUG builds, as they break our ability to use some of BaseLib interfaces to verify untrusted user data in UEFI Applications. The background of this problem is covered in a bugzilla[1], and was also extensively discussed in the last proposed patch discussion thread[2]. We want the solution to the problem to land in the next stable tag: edk2-stable202005, and can submit the patch.
>> 
>> Currently there is no clear decision on what approach to take, as there are three different proposals to implement:
>> 
>> 1) Andrew Fish’s approach with C11-like constraint handler[3].
>> 2) Marvin Häuser’s approach with return codes and constraint assertions being moved to the caller side[4].
>> 3) My approach suggesting DebugLib interface simplification, permitting us to resolve the problem and keep backwards compatibility[5].
>> 
>> My personal opinion is that:
>> 
>> — Marvin’s way is very interesting, but it would require implementing code on the caller side basically for every call, and this would requires extra programming effort. It may also be uneasy to teach people how to do this right, and this does not strictly provide a good solution for situations, where nesting is more than 2 (i.e. when user code calls library code, which itself calls library code). Because of these downsides I am afraid it is impractical to adopt it in EDK II.
>> — Andrew’s way is reasonable from the C standard point of view, but it does not work quite right with reentrancy and I am not positive we need the dynamic handling, especially because of DEBUG/RELEASE build confusion. I explained this in my previous e-mail, which I attached to the end of this message as quote, since groups.io <http://groups.io/> ate the previous send. Other than that, I would say that I can implement this approach if there is no other option.
>> — My own approach makes sense, as it reduces the amount of code in every DebugLib and actually simplifies the development in the future. I believe currently this is the most reasonable route we can take, and suggest other parties to consider it.
>> 
>> Since the three of us each have their own suggestions, I ask Mike and others to rejoin our discussions and give their opinions quickly, so that we can choose, perhaps vote, and proceed with the implementation. Thanks for your dedication.
> 
> I support the original proposal given in:
> 
>  https://bugzilla.tianocore.org/show_bug.cgi?id=2054#c0
> 
> also known as the v3 posting:
> 
> * [edk2-devel] [PATCH v3 1/1]
>  MdePkg: Add PCD to disable safe string constraint assertions
> 
> https://edk2.groups.io/g/devel/message/52838
> http://mid.mail-archive.com/20200103171242.63839-2-vit9696@protonmail.com
> 
> With the following tweak: I think we should rather introduce a new bit
> in "PcdDebugPropertyMask" than an entirely new BOOLEAN PCD.
> 

Laszlo,

+1

> (Note that both PcdDebugPropertyMask and the propsed new PCD support
> precisely the following PCD access methods: PcdsFixedAtBuild,
> PcdsPatchableInModule.)
> 
> I don't feel too strongly about this part (i.e., whether we introduce a
> new PCD, or a new bit in PcdDebugPropertyMask), as long as we make sure
> that the access method can *never* become dynamic.
> 
> I do prefer quite strongly the original proposal, at a higher level.
> 
> Here's why I support the original proposal:
> 
> - In a brand new code base (or brand new set of APIs), I would fight
> tooth and nail to keep such ASSERT()s out of the *base* APIs. It's up to
> the caller to check return values. Also, additional wrapper
> functions/macros can be offered *on top* of the base APIs that
> internalize the ASSERT()s for special use cases.
> 
> But this ship has sailed, for edk2.
> 

Crazy idea but we could add versions of APIs that don't do the checking as new APIs? That would make life easier for untrusted code, Runtime Drivers, code running on the AP, and other places that the ASSERTs could have side effects. 

> - I'm unfriendly towards callbacks. They make the behavior of code
> implicit rather than explicit, and implicit is more difficult to reason
> about. IMO callbacks should be considered a last resort only.
> 

Not my 1st choice either as I agree with your first point about given the caller control. I was just wanted to go through the exercise of what it would take to make it like C11, and give the caller control. 

> Just my two cents, because I've been asked to comment.

Thanks for helping out.

Thanks,

Andrew Fish

> 
> Thanks
> Laszlo
> 

Re: Disabling safe string constraint assertions

[Laszlo Ersek]

On 03/04/20 18:53, Andrew Fish wrote:
> 
> 
>> On Mar 4, 2020, at 5:33 AM, Laszlo Ersek <lersek@redhat.com> wrote:
>>
>> On 03/03/20 22:12, Vitaly Cheptsov wrote:
>>> Hello,
>>>
>>> I am creating a new thread for this issue, as for some reason half of my messages do not display in the web-interface and some of e-mail clients in the previous one. It seems that somebody sent broken HTML code to groups.io <http://groups.io/>, and it did not like to be quoted. It will be quite nice if all the messages sent are plain-text to avoid such issues in the future.
>>>
>>> Back to the issue, we want to make constraint assertions (SAFE_STRING_CONSTRAINT_CHECK) in SafeString.c be optionally disabled in DEBUG builds, as they break our ability to use some of BaseLib interfaces to verify untrusted user data in UEFI Applications. The background of this problem is covered in a bugzilla[1], and was also extensively discussed in the last proposed patch discussion thread[2]. We want the solution to the problem to land in the next stable tag: edk2-stable202005, and can submit the patch.
>>>
>>> Currently there is no clear decision on what approach to take, as there are three different proposals to implement:
>>>
>>> 1) Andrew Fish’s approach with C11-like constraint handler[3].
>>> 2) Marvin Häuser’s approach with return codes and constraint assertions being moved to the caller side[4].
>>> 3) My approach suggesting DebugLib interface simplification, permitting us to resolve the problem and keep backwards compatibility[5].
>>>
>>> My personal opinion is that:
>>>
>>> — Marvin’s way is very interesting, but it would require implementing code on the caller side basically for every call, and this would requires extra programming effort. It may also be uneasy to teach people how to do this right, and this does not strictly provide a good solution for situations, where nesting is more than 2 (i.e. when user code calls library code, which itself calls library code). Because of these downsides I am afraid it is impractical to adopt it in EDK II.
>>> — Andrew’s way is reasonable from the C standard point of view, but it does not work quite right with reentrancy and I am not positive we need the dynamic handling, especially because of DEBUG/RELEASE build confusion. I explained this in my previous e-mail, which I attached to the end of this message as quote, since groups.io <http://groups.io/> ate the previous send. Other than that, I would say that I can implement this approach if there is no other option.
>>> — My own approach makes sense, as it reduces the amount of code in every DebugLib and actually simplifies the development in the future. I believe currently this is the most reasonable route we can take, and suggest other parties to consider it.
>>>
>>> Since the three of us each have their own suggestions, I ask Mike and others to rejoin our discussions and give their opinions quickly, so that we can choose, perhaps vote, and proceed with the implementation. Thanks for your dedication.
>>
>> I support the original proposal given in:
>>
>>  https://bugzilla.tianocore.org/show_bug.cgi?id=2054#c0
>>
>> also known as the v3 posting:
>>
>> * [edk2-devel] [PATCH v3 1/1]
>>  MdePkg: Add PCD to disable safe string constraint assertions
>>
>> https://edk2.groups.io/g/devel/message/52838
>> http://mid.mail-archive.com/20200103171242.63839-2-vit9696@protonmail.com
>>
>> With the following tweak: I think we should rather introduce a new bit
>> in "PcdDebugPropertyMask" than an entirely new BOOLEAN PCD.
>>
> 
> Laszlo,
> 
> +1
> 
>> (Note that both PcdDebugPropertyMask and the propsed new PCD support
>> precisely the following PCD access methods: PcdsFixedAtBuild,
>> PcdsPatchableInModule.)
>>
>> I don't feel too strongly about this part (i.e., whether we introduce a
>> new PCD, or a new bit in PcdDebugPropertyMask), as long as we make sure
>> that the access method can *never* become dynamic.
>>
>> I do prefer quite strongly the original proposal, at a higher level.
>>
>> Here's why I support the original proposal:
>>
>> - In a brand new code base (or brand new set of APIs), I would fight
>> tooth and nail to keep such ASSERT()s out of the *base* APIs. It's up to
>> the caller to check return values. Also, additional wrapper
>> functions/macros can be offered *on top* of the base APIs that
>> internalize the ASSERT()s for special use cases.
>>
>> But this ship has sailed, for edk2.
>>
> 
> Crazy idea but we could add versions of APIs that don't do the checking as new APIs? That would make life easier for untrusted code, Runtime Drivers, code running on the AP, and other places that the ASSERTs could have side effects. 

I'm not sure I understand "new APIs" correctly.

If it's an entirely new set of APIs, then I think it's not good for
Vitaly -- I understand Vitaly wants to use the existent codebase without
much churn, just with the ASSERTs removed.

If you mean a new library instance, that's different. If the package
owners do not mind the code duplication that's inherent in creating such
an alternative library instance, then sure, it can work for me.

> 
>> - I'm unfriendly towards callbacks. They make the behavior of code
>> implicit rather than explicit, and implicit is more difficult to reason
>> about. IMO callbacks should be considered a last resort only.
>>
> 
> Not my 1st choice either as I agree with your first point about given the caller control. I was just wanted to go through the exercise of what it would take to make it like C11, and give the caller control. 
> 
>> Just my two cents, because I've been asked to comment.
> 
> Thanks for helping out.

Thanks!
Laszlo

Re: Disabling safe string constraint assertions

[Andrew Fish]

[-- Attachment #1: Type: text/plain, Size: 6248 bytes --]



> On Mar 4, 2020, at 10:18 AM, Laszlo Ersek <lersek@redhat.com> wrote:
> 
> On 03/04/20 18:53, Andrew Fish wrote:
>> 
>> 
>>> On Mar 4, 2020, at 5:33 AM, Laszlo Ersek <lersek@redhat.com> wrote:
>>> 
>>> On 03/03/20 22:12, Vitaly Cheptsov wrote:
>>>> Hello,
>>>> 
>>>> I am creating a new thread for this issue, as for some reason half of my messages do not display in the web-interface and some of e-mail clients in the previous one. It seems that somebody sent broken HTML code to groups.io <http://groups.io/>, and it did not like to be quoted. It will be quite nice if all the messages sent are plain-text to avoid such issues in the future.
>>>> 
>>>> Back to the issue, we want to make constraint assertions (SAFE_STRING_CONSTRAINT_CHECK) in SafeString.c be optionally disabled in DEBUG builds, as they break our ability to use some of BaseLib interfaces to verify untrusted user data in UEFI Applications. The background of this problem is covered in a bugzilla[1], and was also extensively discussed in the last proposed patch discussion thread[2]. We want the solution to the problem to land in the next stable tag: edk2-stable202005, and can submit the patch.
>>>> 
>>>> Currently there is no clear decision on what approach to take, as there are three different proposals to implement:
>>>> 
>>>> 1) Andrew Fish’s approach with C11-like constraint handler[3].
>>>> 2) Marvin Häuser’s approach with return codes and constraint assertions being moved to the caller side[4].
>>>> 3) My approach suggesting DebugLib interface simplification, permitting us to resolve the problem and keep backwards compatibility[5].
>>>> 
>>>> My personal opinion is that:
>>>> 
>>>> — Marvin’s way is very interesting, but it would require implementing code on the caller side basically for every call, and this would requires extra programming effort. It may also be uneasy to teach people how to do this right, and this does not strictly provide a good solution for situations, where nesting is more than 2 (i.e. when user code calls library code, which itself calls library code). Because of these downsides I am afraid it is impractical to adopt it in EDK II.
>>>> — Andrew’s way is reasonable from the C standard point of view, but it does not work quite right with reentrancy and I am not positive we need the dynamic handling, especially because of DEBUG/RELEASE build confusion. I explained this in my previous e-mail, which I attached to the end of this message as quote, since groups.io <http://groups.io/> ate the previous send. Other than that, I would say that I can implement this approach if there is no other option.
>>>> — My own approach makes sense, as it reduces the amount of code in every DebugLib and actually simplifies the development in the future. I believe currently this is the most reasonable route we can take, and suggest other parties to consider it.
>>>> 
>>>> Since the three of us each have their own suggestions, I ask Mike and others to rejoin our discussions and give their opinions quickly, so that we can choose, perhaps vote, and proceed with the implementation. Thanks for your dedication.
>>> 
>>> I support the original proposal given in:
>>> 
>>> https://bugzilla.tianocore.org/show_bug.cgi?id=2054#c0
>>> 
>>> also known as the v3 posting:
>>> 
>>> * [edk2-devel] [PATCH v3 1/1]
>>> MdePkg: Add PCD to disable safe string constraint assertions
>>> 
>>> https://edk2.groups.io/g/devel/message/52838
>>> http://mid.mail-archive.com/20200103171242.63839-2-vit9696@protonmail.com
>>> 
>>> With the following tweak: I think we should rather introduce a new bit
>>> in "PcdDebugPropertyMask" than an entirely new BOOLEAN PCD.
>>> 
>> 
>> Laszlo,
>> 
>> +1
>> 
>>> (Note that both PcdDebugPropertyMask and the propsed new PCD support
>>> precisely the following PCD access methods: PcdsFixedAtBuild,
>>> PcdsPatchableInModule.)
>>> 
>>> I don't feel too strongly about this part (i.e., whether we introduce a
>>> new PCD, or a new bit in PcdDebugPropertyMask), as long as we make sure
>>> that the access method can *never* become dynamic.
>>> 
>>> I do prefer quite strongly the original proposal, at a higher level.
>>> 
>>> Here's why I support the original proposal:
>>> 
>>> - In a brand new code base (or brand new set of APIs), I would fight
>>> tooth and nail to keep such ASSERT()s out of the *base* APIs. It's up to
>>> the caller to check return values. Also, additional wrapper
>>> functions/macros can be offered *on top* of the base APIs that
>>> internalize the ASSERT()s for special use cases.
>>> 
>>> But this ship has sailed, for edk2.
>>> 
>> 
>> Crazy idea but we could add versions of APIs that don't do the checking as new APIs? That would make life easier for untrusted code, Runtime Drivers, code running on the AP, and other places that the ASSERTs could have side effects. 
> 
> I'm not sure I understand "new APIs" correctly.
> 
> If it's an entirely new set of APIs, then I think it's not good for
> Vitaly -- I understand Vitaly wants to use the existent codebase without
> much churn, just with the ASSERTs removed.
> 
> If you mean a new library instance, that's different. If the package
> owners do not mind the code duplication that's inherent in creating such
> an alternative library instance, then sure, it can work for me.
> 

Laszlo,

Sorry the new APIs would be in addition, and the simplest way to cleanly solve your concern that I share about the caller not having control. 

I don't want a long term goal to block getting the short term features needed by the community.

Thanks,

Andrew Fish

>> 
>>> - I'm unfriendly towards callbacks. They make the behavior of code
>>> implicit rather than explicit, and implicit is more difficult to reason
>>> about. IMO callbacks should be considered a last resort only.
>>> 
>> 
>> Not my 1st choice either as I agree with your first point about given the caller control. I was just wanted to go through the exercise of what it would take to make it like C11, and give the caller control. 
>> 
>>> Just my two cents, because I've been asked to comment.
>> 
>> Thanks for helping out.
> 
> Thanks!
> Laszlo


[-- Attachment #2: Type: text/html, Size: 17360 bytes --]

Re: Disabling safe string constraint assertions

[cheptsov]

[-- Attachment #1: Type: text/plain, Size: 7522 bytes --]

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.

Best wishes,
Vitaly

>> On 4 Mar 2020, at 21:57, Andrew Fish <afish@apple.com> wrote:
> 
> 
>> On Mar 4, 2020, at 10:18 AM, Laszlo Ersek <lersek@redhat.com> wrote:
>> 
>>> On 03/04/20 18:53, Andrew Fish wrote:
>>> 
>>> 
>>>> On Mar 4, 2020, at 5:33 AM, Laszlo Ersek <lersek@redhat.com> wrote:
>>>> 
>>>>> On 03/03/20 22:12, Vitaly Cheptsov wrote:
>>>>> Hello,
>>>>> 
>>>>> I am creating a new thread for this issue, as for some reason half of my messages do not display in the web-interface and some of e-mail clients in the previous one. It seems that somebody sent broken HTML code to groups.io <http://groups.io/>, and it did not like to be quoted. It will be quite nice if all the messages sent are plain-text to avoid such issues in the future.
>>>>> 
>>>>> Back to the issue, we want to make constraint assertions (SAFE_STRING_CONSTRAINT_CHECK) in SafeString.c be optionally disabled in DEBUG builds, as they break our ability to use some of BaseLib interfaces to verify untrusted user data in UEFI Applications. The background of this problem is covered in a bugzilla[1], and was also extensively discussed in the last proposed patch discussion thread[2]. We want the solution to the problem to land in the next stable tag: edk2-stable202005, and can submit the patch.
>>>>> 
>>>>> Currently there is no clear decision on what approach to take, as there are three different proposals to implement:
>>>>> 
>>>>> 1) Andrew Fish’s approach with C11-like constraint handler[3].
>>>>> 2) Marvin Häuser’s approach with return codes and constraint assertions being moved to the caller side[4].
>>>>> 3) My approach suggesting DebugLib interface simplification, permitting us to resolve the problem and keep backwards compatibility[5].
>>>>> 
>>>>> My personal opinion is that:
>>>>> 
>>>>> — Marvin’s way is very interesting, but it would require implementing code on the caller side basically for every call, and this would requires extra programming effort. It may also be uneasy to teach people how to do this right, and this does not strictly provide a good solution for situations, where nesting is more than 2 (i.e. when user code calls library code, which itself calls library code). Because of these downsides I am afraid it is impractical to adopt it in EDK II.
>>>>> — Andrew’s way is reasonable from the C standard point of view, but it does not work quite right with reentrancy and I am not positive we need the dynamic handling, especially because of DEBUG/RELEASE build confusion. I explained this in my previous e-mail, which I attached to the end of this message as quote, since groups.io <http://groups.io/> ate the previous send. Other than that, I would say that I can implement this approach if there is no other option.
>>>>> — My own approach makes sense, as it reduces the amount of code in every DebugLib and actually simplifies the development in the future. I believe currently this is the most reasonable route we can take, and suggest other parties to consider it.
>>>>> 
>>>>> Since the three of us each have their own suggestions, I ask Mike and others to rejoin our discussions and give their opinions quickly, so that we can choose, perhaps vote, and proceed with the implementation. Thanks for your dedication.
>>>> 
>>>> I support the original proposal given in:
>>>> 
>>>> https://bugzilla.tianocore.org/show_bug.cgi?id=2054#c0
>>>> 
>>>> also known as the v3 posting:
>>>> 
>>>> * [edk2-devel] [PATCH v3 1/1]
>>>> MdePkg: Add PCD to disable safe string constraint assertions
>>>> 
>>>> https://edk2.groups.io/g/devel/message/52838
>>>> http://mid.mail-archive.com/20200103171242.63839-2-vit9696@protonmail.com
>>>> 
>>>> With the following tweak: I think we should rather introduce a new bit
>>>> in "PcdDebugPropertyMask" than an entirely new BOOLEAN PCD.
>>> 
>>> Laszlo,
>>> 
>>> +1
>>> 
>>>> (Note that both PcdDebugPropertyMask and the propsed new PCD support
>>>> precisely the following PCD access methods: PcdsFixedAtBuild,
>>>> PcdsPatchableInModule.)
>>>> 
>>>> I don't feel too strongly about this part (i.e., whether we introduce a
>>>> new PCD, or a new bit in PcdDebugPropertyMask), as long as we make sure
>>>> that the access method can *never* become dynamic.
>>>> 
>>>> I do prefer quite strongly the original proposal, at a higher level.
>>>> 
>>>> Here's why I support the original proposal:
>>>> 
>>>> - In a brand new code base (or brand new set of APIs), I would fight
>>>> tooth and nail to keep such ASSERT()s out of the *base* APIs. It's up to
>>>> the caller to check return values. Also, additional wrapper
>>>> functions/macros can be offered *on top* of the base APIs that
>>>> internalize the ASSERT()s for special use cases.
>>>> 
>>>> But this ship has sailed, for edk2.
>>> 
>>> Crazy idea but we could add versions of APIs that don't do the checking as new APIs? That would make life easier for untrusted code, Runtime Drivers, code running on the AP, and other places that the ASSERTs could have side effects. 
>> 
>> I'm not sure I understand "new APIs" correctly.
>> 
>> If it's an entirely new set of APIs, then I think it's not good for
>> Vitaly -- I understand Vitaly wants to use the existent codebase without
>> much churn, just with the ASSERTs removed.
>> 
>> If you mean a new library instance, that's different. If the package
>> owners do not mind the code duplication that's inherent in creating such
>> an alternative library instance, then sure, it can work for me.
>> 
> 
> Laszlo,
> 
> Sorry the new APIs would be in addition, and the simplest way to cleanly solve your concern that I share about the caller not having control. 
> 
> I don't want a long term goal to block getting the short term features needed by the community.
> 
> Thanks,
> 
> Andrew Fish
> 
>>> 
>>>> - I'm unfriendly towards callbacks. They make the behavior of code
>>>> implicit rather than explicit, and implicit is more difficult to reason
>>>> about. IMO callbacks should be considered a last resort only.
>>> 
>>> Not my 1st choice either as I agree with your first point about given the caller control. I was just wanted to go through the exercise of what it would take to make it like C11, and give the caller control. 
>>> 
>>>> Just my two cents, because I've been asked to comment.
>>> 
>>> Thanks for helping out.
>> 
>> Thanks!
>> Laszlo

[-- Attachment #2: Type: text/html, Size: 19618 bytes --]

Re: Disabling safe string constraint assertions

[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

Re: Disabling safe string constraint assertions

[Vitaly Cheptsov]

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

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> написал(а):
> 
> 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.1: Type: text/html, Size: 10353 bytes --]

[-- Attachment #1.2.2: constraint-r3.diff --]
[-- Type: application/octet-stream, Size: 30115 bytes --]

diff --git a/MdePkg/MdePkg.dec b/MdePkg/MdePkg.dec
index ac1f5339af..b060ac2ac9 100644
--- a/MdePkg/MdePkg.dec
+++ b/MdePkg/MdePkg.dec
@@ -2133,9 +2133,10 @@ [PcdsFixedAtBuild,PcdsPatchableInModule]
   #  BIT3 - Enable Clear Memory.<BR>
   #  BIT4 - Enable BreakPoint as ASSERT.<BR>
   #  BIT5 - Enable DeadLoop as ASSERT.<BR>
+  #  BIT6 - Treat constrait violations as ASSERT.<BR>
   # @Prompt Debug Property.
   # @Expression  0x80000002 | (gEfiMdePkgTokenSpaceGuid.PcdDebugPropertyMask & 0xC0) == 0
-  gEfiMdePkgTokenSpaceGuid.PcdDebugPropertyMask|0|UINT8|0x00000005
+  gEfiMdePkgTokenSpaceGuid.PcdDebugPropertyMask|0|UINT8|0x00000045
 
   ## This flag is used to control the print out Debug message.<BR><BR>
   #  BIT0  - Initialization message.<BR>
diff --git a/MdePkg/Library/BaseDebugLibNull/BaseDebugLibNull.inf b/MdePkg/Library/BaseDebugLibNull/BaseDebugLibNull.inf
index 81a63a5074..1173ac30b5 100644
--- a/MdePkg/Library/BaseDebugLibNull/BaseDebugLibNull.inf
+++ b/MdePkg/Library/BaseDebugLibNull/BaseDebugLibNull.inf
@@ -29,3 +29,12 @@ [Sources]
 [Packages]
   MdePkg/MdePkg.dec
 
+
+[LibraryClasses]
+  PcdLib
+
+
+[Pcd]
+  gEfiMdePkgTokenSpaceGuid.PcdDebugPropertyMask         ## CONSUMES
+  gEfiMdePkgTokenSpaceGuid.PcdFixedDebugPrintErrorLevel ## CONSUMES
+
diff --git a/MdePkg/Include/Library/BaseLib.h b/MdePkg/Include/Library/BaseLib.h
index ecadff8b23..08beaa8c23 100644
--- a/MdePkg/Include/Library/BaseLib.h
+++ b/MdePkg/Include/Library/BaseLib.h
@@ -189,7 +189,7 @@ StrnSizeS (
 
   If Destination is not aligned on a 16-bit boundary, then ASSERT().
   If Source is not aligned on a 16-bit boundary, then ASSERT().
-  If an error would be returned, then the function will also ASSERT().
+  If an error would be returned, then the function will also ASSERT_CONSTRAINT().
 
   If an error is returned, then the Destination is unmodified.
 
@@ -225,7 +225,7 @@ StrCpyS (
 
   If Length > 0 and Destination is not aligned on a 16-bit boundary, then ASSERT().
   If Length > 0 and Source is not aligned on a 16-bit boundary, then ASSERT().
-  If an error would be returned, then the function will also ASSERT().
+  If an error would be returned, then the function will also ASSERT_CONSTRAINT().
 
   If an error is returned, then the Destination is unmodified.
 
@@ -263,7 +263,7 @@ StrnCpyS (
 
   If Destination is not aligned on a 16-bit boundary, then ASSERT().
   If Source is not aligned on a 16-bit boundary, then ASSERT().
-  If an error would be returned, then the function will also ASSERT().
+  If an error would be returned, then the function will also ASSERT_CONSTRAINT().
 
   If an error is returned, then the Destination is unmodified.
 
@@ -303,7 +303,7 @@ StrCatS (
 
   If Destination is not aligned on a 16-bit boundary, then ASSERT().
   If Source is not aligned on a 16-bit boundary, then ASSERT().
-  If an error would be returned, then the function will also ASSERT().
+  If an error would be returned, then the function will also ASSERT_CONSTRAINT().
 
   If an error is returned, then the Destination is unmodified.
 
@@ -350,12 +350,12 @@ StrnCatS (
   be ignored. Then, the function stops at the first character that is a not a
   valid decimal character or a Null-terminator, whichever one comes first.
 
-  If String is NULL, then ASSERT().
-  If Data is NULL, then ASSERT().
+  If String is NULL, then ASSERT_CONSTRAINT().
+  If Data is NULL, then ASSERT_CONSTRAINT().
   If String is not aligned in a 16-bit boundary, then ASSERT().
   If PcdMaximumUnicodeStringLength is not zero, and String contains more than
   PcdMaximumUnicodeStringLength Unicode characters, not including the
-  Null-terminator, then ASSERT().
+  Null-terminator, then ASSERT_CONSTRAINT().
 
   If String has no valid decimal digits in the above format, then 0 is stored
   at the location pointed to by Data.
@@ -406,12 +406,12 @@ StrDecimalToUintnS (
   be ignored. Then, the function stops at the first character that is a not a
   valid decimal character or a Null-terminator, whichever one comes first.
 
-  If String is NULL, then ASSERT().
-  If Data is NULL, then ASSERT().
+  If String is NULL, then ASSERT_CONSTRAINT().
+  If Data is NULL, then ASSERT_CONSTRAINT().
   If String is not aligned in a 16-bit boundary, then ASSERT().
   If PcdMaximumUnicodeStringLength is not zero, and String contains more than
   PcdMaximumUnicodeStringLength Unicode characters, not including the
-  Null-terminator, then ASSERT().
+  Null-terminator, then ASSERT_CONSTRAINT().
 
   If String has no valid decimal digits in the above format, then 0 is stored
   at the location pointed to by Data.
@@ -467,12 +467,12 @@ StrDecimalToUint64S (
   the first character that is a not a valid hexadecimal character or NULL,
   whichever one comes first.
 
-  If String is NULL, then ASSERT().
-  If Data is NULL, then ASSERT().
+  If String is NULL, then ASSERT_CONSTRAINT().
+  If Data is NULL, then ASSERT_CONSTRAINT().
   If String is not aligned in a 16-bit boundary, then ASSERT().
   If PcdMaximumUnicodeStringLength is not zero, and String contains more than
   PcdMaximumUnicodeStringLength Unicode characters, not including the
-  Null-terminator, then ASSERT().
+  Null-terminator, then ASSERT_CONSTRAINT().
 
   If String has no valid hexadecimal digits in the above format, then 0 is
   stored at the location pointed to by Data.
@@ -528,12 +528,12 @@ StrHexToUintnS (
   the first character that is a not a valid hexadecimal character or NULL,
   whichever one comes first.
 
-  If String is NULL, then ASSERT().
-  If Data is NULL, then ASSERT().
+  If String is NULL, then ASSERT_CONSTRAINT().
+  If Data is NULL, then ASSERT_CONSTRAINT().
   If String is not aligned in a 16-bit boundary, then ASSERT().
   If PcdMaximumUnicodeStringLength is not zero, and String contains more than
   PcdMaximumUnicodeStringLength Unicode characters, not including the
-  Null-terminator, then ASSERT().
+  Null-terminator, then ASSERT_CONSTRAINT().
 
   If String has no valid hexadecimal digits in the above format, then 0 is
   stored at the location pointed to by Data.
@@ -622,7 +622,7 @@ AsciiStrnSizeS (
 
   This function is similar as strcpy_s defined in C11.
 
-  If an error would be returned, then the function will also ASSERT().
+  If an error would be returned, then the function will also ASSERT_CONSTRAINT().
 
   If an error is returned, then the Destination is unmodified.
 
@@ -656,7 +656,7 @@ AsciiStrCpyS (
 
   This function is similar as strncpy_s defined in C11.
 
-  If an error would be returned, then the function will also ASSERT().
+  If an error would be returned, then the function will also ASSERT_CONSTRAINT().
 
   If an error is returned, then the Destination is unmodified.
 
@@ -692,7 +692,7 @@ AsciiStrnCpyS (
 
   This function is similar as strcat_s defined in C11.
 
-  If an error would be returned, then the function will also ASSERT().
+  If an error would be returned, then the function will also ASSERT_CONSTRAINT().
 
   If an error is returned, then the Destination is unmodified.
 
@@ -730,7 +730,7 @@ AsciiStrCatS (
 
   This function is similar as strncat_s defined in C11.
 
-  If an error would be returned, then the function will also ASSERT().
+  If an error would be returned, then the function will also ASSERT_CONSTRAINT().
 
   If an error is returned, then the Destination is unmodified.
 
@@ -777,11 +777,11 @@ AsciiStrnCatS (
   be ignored. Then, the function stops at the first character that is a not a
   valid decimal character or a Null-terminator, whichever one comes first.
 
-  If String is NULL, then ASSERT().
-  If Data is NULL, then ASSERT().
+  If String is NULL, then ASSERT_CONSTRAINT().
+  If Data is NULL, then ASSERT_CONSTRAINT().
   If PcdMaximumAsciiStringLength is not zero, and String contains more than
   PcdMaximumAsciiStringLength Ascii characters, not including the
-  Null-terminator, then ASSERT().
+  Null-terminator, then ASSERT_CONSTRAINT().
 
   If String has no valid decimal digits in the above format, then 0 is stored
   at the location pointed to by Data.
@@ -832,11 +832,11 @@ AsciiStrDecimalToUintnS (
   be ignored. Then, the function stops at the first character that is a not a
   valid decimal character or a Null-terminator, whichever one comes first.
 
-  If String is NULL, then ASSERT().
-  If Data is NULL, then ASSERT().
+  If String is NULL, then ASSERT_CONSTRAINT().
+  If Data is NULL, then ASSERT_CONSTRAINT().
   If PcdMaximumAsciiStringLength is not zero, and String contains more than
   PcdMaximumAsciiStringLength Ascii characters, not including the
-  Null-terminator, then ASSERT().
+  Null-terminator, then ASSERT_CONSTRAINT().
 
   If String has no valid decimal digits in the above format, then 0 is stored
   at the location pointed to by Data.
@@ -891,11 +891,11 @@ AsciiStrDecimalToUint64S (
   character that is a not a valid hexadecimal character or Null-terminator,
   whichever on comes first.
 
-  If String is NULL, then ASSERT().
-  If Data is NULL, then ASSERT().
+  If String is NULL, then ASSERT_CONSTRAINT().
+  If Data is NULL, then ASSERT_CONSTRAINT().
   If PcdMaximumAsciiStringLength is not zero, and String contains more than
   PcdMaximumAsciiStringLength Ascii characters, not including the
-  Null-terminator, then ASSERT().
+  Null-terminator, then ASSERT_CONSTRAINT().
 
   If String has no valid hexadecimal digits in the above format, then 0 is
   stored at the location pointed to by Data.
@@ -950,11 +950,11 @@ AsciiStrHexToUintnS (
   character that is a not a valid hexadecimal character or Null-terminator,
   whichever on comes first.
 
-  If String is NULL, then ASSERT().
-  If Data is NULL, then ASSERT().
+  If String is NULL, then ASSERT_CONSTRAINT().
+  If Data is NULL, then ASSERT_CONSTRAINT().
   If PcdMaximumAsciiStringLength is not zero, and String contains more than
   PcdMaximumAsciiStringLength Ascii characters, not including the
-  Null-terminator, then ASSERT().
+  Null-terminator, then ASSERT_CONSTRAINT().
 
   If String has no valid hexadecimal digits in the above format, then 0 is
   stored at the location pointed to by Data.
@@ -1506,15 +1506,15 @@ StrHexToUint64 (
   "::" can be used to compress one or more groups of X when X contains only 0.
   The "::" can only appear once in the String.
 
-  If String is NULL, then ASSERT().
+  If String is NULL, then ASSERT_CONSTRAINT().
 
-  If Address is NULL, then ASSERT().
+  If Address is NULL, then ASSERT_CONSTRAINT().
 
   If String is not aligned in a 16-bit boundary, then ASSERT().
 
   If PcdMaximumUnicodeStringLength is not zero, and String contains more than
   PcdMaximumUnicodeStringLength Unicode characters, not including the
-  Null-terminator, then ASSERT().
+  Null-terminator, then ASSERT_CONSTRAINT().
 
   If EndPointer is not NULL and Address is translated from String, a pointer
   to the character that stopped the scan is stored at the location pointed to
@@ -1567,15 +1567,15 @@ StrToIpv6Address (
   When /P is in the String, the function stops at the first character that is not
   a valid decimal digit character after P is converted.
 
-  If String is NULL, then ASSERT().
+  If String is NULL, then ASSERT_CONSTRAINT().
 
-  If Address is NULL, then ASSERT().
+  If Address is NULL, then ASSERT_CONSTRAINT().
 
   If String is not aligned in a 16-bit boundary, then ASSERT().
 
   If PcdMaximumUnicodeStringLength is not zero, and String contains more than
   PcdMaximumUnicodeStringLength Unicode characters, not including the
-  Null-terminator, then ASSERT().
+  Null-terminator, then ASSERT_CONSTRAINT().
 
   If EndPointer is not NULL and Address is translated from String, a pointer
   to the character that stopped the scan is stored at the location pointed to
@@ -1640,8 +1640,8 @@ StrToIpv4Address (
                   oo          Data4[48:55]
                   pp          Data4[56:63]
 
-  If String is NULL, then ASSERT().
-  If Guid is NULL, then ASSERT().
+  If String is NULL, then ASSERT_CONSTRAINT().
+  If Guid is NULL, then ASSERT_CONSTRAINT().
   If String is not aligned in a 16-bit boundary, then ASSERT().
 
   @param  String                   Pointer to a Null-terminated Unicode string.
@@ -1676,16 +1676,16 @@ StrToGuid (
 
   If String is not aligned in a 16-bit boundary, then ASSERT().
 
-  If String is NULL, then ASSERT().
+  If String is NULL, then ASSERT_CONSTRAINT().
 
-  If Buffer is NULL, then ASSERT().
+  If Buffer is NULL, then ASSERT_CONSTRAINT().
 
-  If Length is not multiple of 2, then ASSERT().
+  If Length is not multiple of 2, then ASSERT_CONSTRAINT().
 
   If PcdMaximumUnicodeStringLength is not zero and Length is greater than
-  PcdMaximumUnicodeStringLength, then ASSERT().
+  PcdMaximumUnicodeStringLength, then ASSERT_CONSTRAINT().
 
-  If MaxBufferSize is less than (Length / 2), then ASSERT().
+  If MaxBufferSize is less than (Length / 2), then ASSERT_CONSTRAINT().
 
   @param  String                   Pointer to a Null-terminated Unicode string.
   @param  Length                   The number of Unicode characters to decode.
@@ -1777,7 +1777,7 @@ UnicodeStrToAsciiStr (
   the upper 8 bits, then ASSERT().
 
   If Source is not aligned on a 16-bit boundary, then ASSERT().
-  If an error would be returned, then the function will also ASSERT().
+  If an error would be returned, then the function will also ASSERT_CONSTRAINT().
 
   If an error is returned, then the Destination is unmodified.
 
@@ -1824,7 +1824,7 @@ UnicodeStrToAsciiStrS (
   If any Unicode characters in Source contain non-zero value in the upper 8
   bits, then ASSERT().
   If Source is not aligned on a 16-bit boundary, then ASSERT().
-  If an error would be returned, then the function will also ASSERT().
+  If an error would be returned, then the function will also ASSERT_CONSTRAINT().
 
   If an error is returned, then the Destination is unmodified.
 
@@ -2388,9 +2388,9 @@ AsciiStrHexToUint64 (
   "::" can be used to compress one or more groups of X when X contains only 0.
   The "::" can only appear once in the String.
 
-  If String is NULL, then ASSERT().
+  If String is NULL, then ASSERT_CONSTRAINT().
 
-  If Address is NULL, then ASSERT().
+  If Address is NULL, then ASSERT_CONSTRAINT().
 
   If EndPointer is not NULL and Address is translated from String, a pointer
   to the character that stopped the scan is stored at the location pointed to
@@ -2443,9 +2443,9 @@ AsciiStrToIpv6Address (
   When /P is in the String, the function stops at the first character that is not
   a valid decimal digit character after P is converted.
 
-  If String is NULL, then ASSERT().
+  If String is NULL, then ASSERT_CONSTRAINT().
 
-  If Address is NULL, then ASSERT().
+  If Address is NULL, then ASSERT_CONSTRAINT().
 
   If EndPointer is not NULL and Address is translated from String, a pointer
   to the character that stopped the scan is stored at the location pointed to
@@ -2508,8 +2508,8 @@ AsciiStrToIpv4Address (
                   oo          Data4[48:55]
                   pp          Data4[56:63]
 
-  If String is NULL, then ASSERT().
-  If Guid is NULL, then ASSERT().
+  If String is NULL, then ASSERT_CONSTRAINT().
+  If Guid is NULL, then ASSERT_CONSTRAINT().
 
   @param  String                   Pointer to a Null-terminated ASCII string.
   @param  Guid                     Pointer to the converted GUID.
@@ -2541,16 +2541,16 @@ AsciiStrToGuid (
   decoding stops after Length of characters and outputs Buffer containing
   (Length / 2) bytes.
 
-  If String is NULL, then ASSERT().
+  If String is NULL, then ASSERT_CONSTRAINT().
 
-  If Buffer is NULL, then ASSERT().
+  If Buffer is NULL, then ASSERT_CONSTRAINT().
 
-  If Length is not multiple of 2, then ASSERT().
+  If Length is not multiple of 2, then ASSERT_CONSTRAINT().
 
   If PcdMaximumAsciiStringLength is not zero and Length is greater than
-  PcdMaximumAsciiStringLength, then ASSERT().
+  PcdMaximumAsciiStringLength, then ASSERT_CONSTRAINT().
 
-  If MaxBufferSize is less than (Length / 2), then ASSERT().
+  If MaxBufferSize is less than (Length / 2), then ASSERT_CONSTRAINT().
 
   @param  String                   Pointer to a Null-terminated ASCII string.
   @param  Length                   The number of ASCII characters to decode.
@@ -2632,7 +2632,7 @@ AsciiStrToUnicodeStr (
   equal or greater than ((AsciiStrLen (Source) + 1) * sizeof (CHAR16)) in bytes.
 
   If Destination is not aligned on a 16-bit boundary, then ASSERT().
-  If an error would be returned, then the function will also ASSERT().
+  If an error would be returned, then the function will also ASSERT_CONSTRAINT().
 
   If an error is returned, then the Destination is unmodified.
 
@@ -2678,7 +2678,7 @@ AsciiStrToUnicodeStrS (
   ((MIN(AsciiStrLen(Source), Length) + 1) * sizeof (CHAR8)) in bytes.
 
   If Destination is not aligned on a 16-bit boundary, then ASSERT().
-  If an error would be returned, then the function will also ASSERT().
+  If an error would be returned, then the function will also ASSERT_CONSTRAINT().
 
   If an error is returned, then Destination and DestinationLength are
   unmodified.
diff --git a/MdePkg/Include/Library/DebugLib.h b/MdePkg/Include/Library/DebugLib.h
index f1d55cf62b..38aebd1701 100644
--- a/MdePkg/Include/Library/DebugLib.h
+++ b/MdePkg/Include/Library/DebugLib.h
@@ -16,6 +16,8 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
 #ifndef __DEBUG_LIB_H__
 #define __DEBUG_LIB_H__
 
+#include <Library/PcdLib.h>
+
 //
 // Declare bits for PcdDebugPropertyMask
 //
@@ -25,6 +27,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
 #define DEBUG_PROPERTY_CLEAR_MEMORY_ENABLED       0x08
 #define DEBUG_PROPERTY_ASSERT_BREAKPOINT_ENABLED  0x10
 #define DEBUG_PROPERTY_ASSERT_DEADLOOP_ENABLED    0x20
+#define DEBUG_PROPERTY_ASSERT_CONSTRAINT_ENABLED  0x40
 
 //
 // Declare bits for PcdDebugPrintErrorLevel and the ErrorLevel parameter of DebugPrint()
@@ -71,6 +74,92 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
 #define EFI_D_VERBOSE   DEBUG_VERBOSE
 #define EFI_D_ERROR     DEBUG_ERROR
 
+
+/**
+  Returns TRUE if ASSERT() macros are enabled.
+
+  This macro evaluates to TRUE if the DEBUG_PROPERTY_DEBUG_ASSERT_ENABLED bit of
+  PcdDebugProperyMask is set.  Otherwise, FALSE is returned.
+
+  @retval  TRUE    The DEBUG_PROPERTY_DEBUG_ASSERT_ENABLED bit of PcdDebugProperyMask is set.
+  @retval  FALSE   The DEBUG_PROPERTY_DEBUG_ASSERT_ENABLED bit of PcdDebugProperyMask is clear.
+
+**/
+#define DEBUG_ASSERT_ENABLED() \
+  ((BOOLEAN) ((PcdGet8 (PcdDebugPropertyMask) & DEBUG_PROPERTY_DEBUG_ASSERT_ENABLED) != 0))
+
+
+/**
+  Returns TRUE if DEBUG() macros are enabled.
+
+  This macro evaluates to TRUE if the DEBUG_PROPERTY_DEBUG_PRINT_ENABLED bit of
+  PcdDebugProperyMask is set.  Otherwise, FALSE is returned.
+
+  @retval  TRUE    The DEBUG_PROPERTY_DEBUG_PRINT_ENABLED bit of PcdDebugProperyMask is set.
+  @retval  FALSE   The DEBUG_PROPERTY_DEBUG_PRINT_ENABLED bit of PcdDebugProperyMask is clear.
+
+**/
+#define DEBUG_PRINT_ENABLED() \
+  ((BOOLEAN) ((PcdGet8 (PcdDebugPropertyMask) & DEBUG_PROPERTY_DEBUG_PRINT_ENABLED) != 0))
+
+
+/**
+  Returns TRUE if DEBUG_CODE() macros are enabled.
+
+  This macro evaluates to TRUE if the DEBUG_PROPERTY_DEBUG_CODE_ENABLED bit of
+  PcdDebugProperyMask is set.  Otherwise, FALSE is returned.
+
+  @retval  TRUE    The DEBUG_PROPERTY_DEBUG_CODE_ENABLED bit of PcdDebugProperyMask is set.
+  @retval  FALSE   The DEBUG_PROPERTY_DEBUG_CODE_ENABLED bit of PcdDebugProperyMask is clear.
+
+**/
+#define DEBUG_CODE_ENABLED() \
+  ((BOOLEAN) ((PcdGet8 (PcdDebugPropertyMask) & DEBUG_PROPERTY_DEBUG_CODE_ENABLED) != 0))
+
+
+/**
+  Returns TRUE if DEBUG_CLEAR_MEMORY() macro is enabled.
+
+  This macro evaluates to TRUE if the DEBUG_PROPERTY_CLEAR_MEMORY_ENABLED bit of
+  PcdDebugProperyMask is set.  Otherwise, FALSE is returned.
+
+  @retval  TRUE    The DEBUG_PROPERTY_CLEAR_MEMORY_ENABLED bit of PcdDebugProperyMask is set.
+  @retval  FALSE   The DEBUG_PROPERTY_CLEAR_MEMORY_ENABLED bit of PcdDebugProperyMask is clear.
+
+**/
+#define DEBUG_CLEAR_MEMORY_ENABLED() \
+  ((BOOLEAN) ((PcdGet8 (PcdDebugPropertyMask) & DEBUG_PROPERTY_CLEAR_MEMORY_ENABLED) != 0))
+
+
+/**
+  Returns TRUE if ASSERT_CONSTRAINT() macros are enabled.
+
+  This macro evaluates to TRUE if the DEBUG_PROPERTY_ASSERT_CONSTRAINT_ENABLED bit of
+  PcdDebugProperyMask is set.  Otherwise, FALSE is returned.
+
+  @retval  TRUE    The DEBUG_PROPERTY_ASSERT_CONSTRAINT_ENABLED bit of PcdDebugProperyMask is set.
+  @retval  FALSE   The DEBUG_PROPERTY_ASSERT_CONSTRAINT_ENABLED bit of PcdDebugProperyMask is clear.
+
+**/
+#define DEBUG_ASSERT_CONSTRAINT_ENABLED() \
+  ((BOOLEAN) ((PcdGet8 (PcdDebugPropertyMask) & DEBUG_PROPERTY_ASSERT_CONSTRAINT_ENABLED) != 0))
+
+
+/**
+  Returns TRUE if any one of the bit is set both in ErrorLevel and PcdFixedDebugPrintErrorLevel.
+
+  This macro compares the bit mask of ErrorLevel and PcdFixedDebugPrintErrorLevel.
+
+  @param  ErrorLevel  The error level to compare.
+
+  @retval  TRUE    Current ErrorLevel is supported.
+  @retval  FALSE   Current ErrorLevel is not supported.
+
+**/
+#define DEBUG_PRINT_LEVEL_ENABLED(ErrorLevel) \
+  ((BOOLEAN) (((ErrorLevel) & PcdGet32 (PcdFixedDebugPrintErrorLevel)) != 0))
+
+
 /**
   Prints a debug message to the debug output device if the specified error level is enabled.
 
@@ -308,7 +397,7 @@ DebugPrintLevelEnabled (
 #if !defined(MDE_CPU_EBC) && (!defined (_MSC_VER) || _MSC_VER > 1400)
   #define _DEBUG_PRINT(PrintLevel, ...)              \
     do {                                             \
-      if (DebugPrintLevelEnabled (PrintLevel)) {     \
+      if (DEBUG_PRINT_LEVEL_ENABLED (PrintLevel)) {  \
         DebugPrint (PrintLevel, ##__VA_ARGS__);      \
       }                                              \
     } while (FALSE)
@@ -330,19 +419,37 @@ DebugPrintLevelEnabled (
 
 **/
 #if !defined(MDEPKG_NDEBUG)
-  #define ASSERT(Expression)        \
-    do {                            \
-      if (DebugAssertEnabled ()) {  \
-        if (!(Expression)) {        \
-          _ASSERT (Expression);     \
-          ANALYZER_UNREACHABLE ();  \
-        }                           \
-      }                             \
+  #define ASSERT(Expression)          \
+    do {                              \
+      if (DEBUG_ASSERT_ENABLED ()) {  \
+        if (!(Expression)) {          \
+          _ASSERT (Expression);       \
+          ANALYZER_UNREACHABLE ();    \
+        }                             \
+      }                               \
     } while (FALSE)
 #else
   #define ASSERT(Expression)
 #endif
 
+
+/**
+  Macro that calls ASSERT when constrain assertions are enabled.
+
+  If DEBUG_PROPERTY_ASSERT_CONSTRAINT_ENABLED bit of PcdDebugProperyMask is set,
+  then this macro evaluates to an ASSERT macro passing in the original Expression.
+
+  @param  Expression  Boolean expression.
+
+**/
+#define ASSERT_CONSTRAINT(Expression)          \
+  do {                                         \
+    if (DEBUG_ASSERT_CONSTRAINT_ENABLED ()) {  \
+      ASSERT (Expression);                     \
+    }                                          \
+  } while (FALSE)
+
+
 /**
   Macro that calls DebugPrint().
 
@@ -356,11 +463,11 @@ DebugPrintLevelEnabled (
 
 **/
 #if !defined(MDEPKG_NDEBUG)
-  #define DEBUG(Expression)        \
-    do {                           \
-      if (DebugPrintEnabled ()) {  \
-        _DEBUG (Expression);       \
-      }                            \
+  #define DEBUG(Expression)          \
+    do {                             \
+      if (DEBUG_PRINT_ENABLED ()) {  \
+        _DEBUG (Expression);         \
+      }                              \
     } while (FALSE)
 #else
   #define DEBUG(Expression)
@@ -381,7 +488,7 @@ DebugPrintLevelEnabled (
 #if !defined(MDEPKG_NDEBUG)
   #define ASSERT_EFI_ERROR(StatusParameter)                                              \
     do {                                                                                 \
-      if (DebugAssertEnabled ()) {                                                       \
+      if (DEBUG_ASSERT_ENABLED ()) {                                                     \
         if (EFI_ERROR (StatusParameter)) {                                               \
           DEBUG ((EFI_D_ERROR, "\nASSERT_EFI_ERROR (Status = %r)\n", StatusParameter));  \
           _ASSERT (!EFI_ERROR (StatusParameter));                                        \
@@ -407,7 +514,7 @@ DebugPrintLevelEnabled (
 #if !defined(MDEPKG_NDEBUG)
   #define ASSERT_RETURN_ERROR(StatusParameter)                          \
     do {                                                                \
-      if (DebugAssertEnabled ()) {                                      \
+      if (DEBUG_ASSERT_ENABLED ()) {                                    \
         if (RETURN_ERROR (StatusParameter)) {                           \
           DEBUG ((DEBUG_ERROR, "\nASSERT_RETURN_ERROR (Status = %r)\n", \
             StatusParameter));                                          \
@@ -444,7 +551,7 @@ DebugPrintLevelEnabled (
 #if !defined(MDEPKG_NDEBUG)
   #define ASSERT_PROTOCOL_ALREADY_INSTALLED(Handle, Guid)                               \
     do {                                                                                \
-      if (DebugAssertEnabled ()) {                                                      \
+      if (DEBUG_ASSERT_ENABLED ()) {                                                    \
         VOID  *Instance;                                                                \
         ASSERT (Guid != NULL);                                                          \
         if (Handle == NULL) {                                                           \
@@ -471,7 +578,7 @@ DebugPrintLevelEnabled (
   are not included in a module.
 
 **/
-#define DEBUG_CODE_BEGIN()  do { if (DebugCodeEnabled ()) { UINT8  __DebugCodeLocal
+#define DEBUG_CODE_BEGIN()  do { if (DEBUG_CODE_ENABLED ()) { UINT8  __DebugCodeLocal
 
 
 /**
@@ -512,7 +619,7 @@ DebugPrintLevelEnabled (
 **/
 #define DEBUG_CLEAR_MEMORY(Address, Length)  \
   do {                                       \
-    if (DebugClearMemoryEnabled ()) {        \
+    if (DEBUG_CLEAR_MEMORY_ENABLED ()) {     \
       DebugClearMemory (Address, Length);    \
     }                                        \
   } while (FALSE)
@@ -561,12 +668,12 @@ DebugPrintLevelEnabled (
 
 **/
 #if !defined(MDEPKG_NDEBUG)
-  #define CR(Record, TYPE, Field, TestSignature)                                              \
-    (DebugAssertEnabled () && (BASE_CR (Record, TYPE, Field)->Signature != TestSignature)) ?  \
-    (TYPE *) (_ASSERT (CR has Bad Signature), Record) :                                       \
+  #define CR(Record, TYPE, Field, TestSignature)                                                \
+    (DEBUG_ASSERT_ENABLED () && (BASE_CR (Record, TYPE, Field)->Signature != TestSignature)) ?  \
+    (TYPE *) (_ASSERT (CR has Bad Signature), Record) :                                         \
     BASE_CR (Record, TYPE, Field)
 #else
-  #define CR(Record, TYPE, Field, TestSignature)                                              \
+  #define CR(Record, TYPE, Field, TestSignature)                                                \
     BASE_CR (Record, TYPE, Field)
 #endif
 
diff --git a/MdePkg/Include/Library/UefiLib.h b/MdePkg/Include/Library/UefiLib.h
index 0abb40d6ec..b18e76bb87 100644
--- a/MdePkg/Include/Library/UefiLib.h
+++ b/MdePkg/Include/Library/UefiLib.h
@@ -331,7 +331,7 @@ EfiInitializeLock (
 #if !defined(MDEPKG_NDEBUG)
   #define ASSERT_LOCKED(LockParameter)                  \
     do {                                                \
-      if (DebugAssertEnabled ()) {                      \
+      if (DEBUG_ASSERT_ENABLED ()) {                    \
         ASSERT (LockParameter != NULL);                 \
         if ((LockParameter)->Lock != EfiLockAcquired) { \
           _ASSERT (LockParameter not locked);           \
diff --git a/MdePkg/Library/BaseLib/SafeString.c b/MdePkg/Library/BaseLib/SafeString.c
index 7dc03d2caa..f6cdd76c82 100644
--- a/MdePkg/Library/BaseLib/SafeString.c
+++ b/MdePkg/Library/BaseLib/SafeString.c
@@ -14,7 +14,7 @@
 
 #define SAFE_STRING_CONSTRAINT_CHECK(Expression, Status)  \
   do { \
-    ASSERT (Expression); \
+    ASSERT_CONSTRAINT (Expression); \
     if (!(Expression)) { \
       return Status; \
     } \
diff --git a/MdePkg/MdePkg.uni b/MdePkg/MdePkg.uni
index 5c1fa24065..5addb0eaba 100644
--- a/MdePkg/MdePkg.uni
+++ b/MdePkg/MdePkg.uni
@@ -189,7 +189,8 @@
                                                                                 "BIT2 - Enable Debug Code.<BR>\n"
                                                                                 "BIT3 - Enable Clear Memory.<BR>\n"
                                                                                 "BIT4 - Enable BreakPoint as ASSERT.<BR>\n"
-                                                                                "BIT5 - Enable DeadLoop as ASSERT.<BR>"
+                                                                                "BIT5 - Enable DeadLoop as ASSERT.<BR>\n"
+                                                                                "BIT6 - Treat constrait violations as ASSERT.<BR>"
 
 #string STR_gEfiMdePkgTokenSpaceGuid_ERR_80000002 #language en-US "Reserved bits must be set to zero."
 

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

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

[Michael D Kinney]

[-- Attachment #1: Type: text/plain, Size: 3714 bytes --]

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 <devel@edk2.groups.io> On Behalf Of Vitaly Cheptsov
Sent: Wednesday, March 18, 2020 12:36 PM
To: Laszlo Ersek <lersek@redhat.com>; Andrew Fish <afish@apple.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Marvin Häuser <mhaeuser@outlook.de>; Gao, Liming <liming.gao@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>
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 <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 #2: Type: text/html, Size: 48117 bytes --]

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

[Vitaly Cheptsov]

[-- 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 --]

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

[Michael D Kinney]

[-- Attachment #1: Type: text/plain, Size: 6233 bytes --]

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@ispras.ru>
Sent: Wednesday, March 18, 2020 1:43 PM
To: Kinney, Michael D <michael.d.kinney@intel.com>
Cc: devel@edk2.groups.io; Laszlo Ersek <lersek@redhat.com>; Andrew Fish <afish@apple.com>; Marvin Häuser <mhaeuser@outlook.de>; Gao, Liming <liming.gao@intel.com>; Gao, Zhichao <zhichao.gao@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@intel.com<mailto: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 #2: Type: text/html, Size: 56055 bytes --]

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

[Vitaly Cheptsov]

[-- Attachment #1: Type: text/plain, Size: 7930 bytes --]

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 <michael.d.kinney@intel.com> 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 <cheptsov@ispras.ru> 
> Sent: Wednesday, March 18, 2020 1:43 PM
> To: Kinney, Michael D <michael.d.kinney@intel.com>
> Cc: devel@edk2.groups.io; Laszlo Ersek <lersek@redhat.com>; Andrew Fish <afish@apple.com>; Marvin Häuser <mhaeuser@outlook.de>; Gao, Liming <liming.gao@intel.com>; Gao, Zhichao <zhichao.gao@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@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 <devel@edk2.groups.io> On Behalf Of Vitaly Cheptsov
> Sent: Wednesday, March 18, 2020 12:36 PM
> To: Laszlo Ersek <lersek@redhat.com>; Andrew Fish <afish@apple.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Marvin Häuser <mhaeuser@outlook.de>; Gao, Liming <liming.gao@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>
> 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 <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 #2: Type: text/html, Size: 58163 bytes --]

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

[Andrew Fish]

[-- Attachment #1: Type: text/plain, Size: 10430 bytes --]

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
  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 <cheptsov@ispras.ru> 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 <michael.d.kinney@intel.com> 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 <cheptsov@ispras.ru> 
>> Sent: Wednesday, March 18, 2020 1:43 PM
>> To: Kinney, Michael D <michael.d.kinney@intel.com>
>> Cc: devel@edk2.groups.io; Laszlo Ersek <lersek@redhat.com>; Andrew Fish <afish@apple.com>; Marvin Häuser <mhaeuser@outlook.de>; Gao, Liming <liming.gao@intel.com>; Gao, Zhichao <zhichao.gao@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@intel.com <mailto: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 #2: Type: text/html, Size: 52761 bytes --]

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

[Vitaly Cheptsov]

[-- Attachment #1: Type: text/plain, Size: 12723 bytes --]

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 <afish@apple.com> 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
>   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 <cheptsov@ispras.ru> 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 <michael.d.kinney@intel.com> 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 <cheptsov@ispras.ru> 
>>> Sent: Wednesday, March 18, 2020 1:43 PM
>>> To: Kinney, Michael D <michael.d.kinney@intel.com>
>>> Cc: devel@edk2.groups.io; Laszlo Ersek <lersek@redhat.com>; Andrew Fish <afish@apple.com>; Marvin Häuser <mhaeuser@outlook.de>; Gao, Liming <liming.gao@intel.com>; Gao, Zhichao <zhichao.gao@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@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 <devel@edk2.groups.io> On Behalf Of Vitaly Cheptsov
>>> Sent: Wednesday, March 18, 2020 12:36 PM
>>> To: Laszlo Ersek <lersek@redhat.com>; Andrew Fish <afish@apple.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Marvin Häuser <mhaeuser@outlook.de>; Gao, Liming <liming.gao@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>
>>> 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 <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 #2: Type: text/html, Size: 55863 bytes --]

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

[Vitaly Cheptsov]

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

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 <cheptsov@ispras.ru> написал(а):
> 
> 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 <afish@apple.com> 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 <cheptsov@ispras.ru <mailto:cheptsov@ispras.ru>> 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 <michael.d.kinney@intel.com <mailto:michael.d.kinney@intel.com>> 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 <cheptsov@ispras.ru <mailto:cheptsov@ispras.ru>>
>>>> Sent: Wednesday, March 18, 2020 1:43 PM
>>>> To: Kinney, Michael D <michael.d.kinney@intel.com <mailto:michael.d.kinney@intel.com>>
>>>> Cc: devel@edk2.groups.io <mailto:devel@edk2.groups.io>; Laszlo Ersek <lersek@redhat.com <mailto:lersek@redhat.com>>; Andrew Fish <afish@apple.com <mailto:afish@apple.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>>
>>>> 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@intel.com <mailto: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: 57297 bytes --]

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

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

[Laszlo Ersek]

On 05/11/20 14:03, Vitaly Cheptsov wrote:
> 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>

github pull requests are only used -- at this time -- by contributors
for personal CI runs, and by edk2 maintainers for merging series that
have been reviewed on the list. Patch review remains mailing list-based,
for now. Please post the patches to the list for review.

> Let me know if any further changes are needed from my side. I hope this still is in time for the May tag.

If this work counts as a feature, then its review has to complete by the
soft feature freeze (2020-05-15).

Thanks,
Laszlo

> 
> Best wishes,
> Vitaly
> 
>> 19 марта 2020 г., в 03:04, Vitaly Cheptsov <cheptsov@ispras.ru> написал(а):
>>
>> 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 <afish@apple.com> 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 <cheptsov@ispras.ru <mailto:cheptsov@ispras.ru>> 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 <michael.d.kinney@intel.com <mailto:michael.d.kinney@intel.com>> 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 <cheptsov@ispras.ru <mailto:cheptsov@ispras.ru>>
>>>>> Sent: Wednesday, March 18, 2020 1:43 PM
>>>>> To: Kinney, Michael D <michael.d.kinney@intel.com <mailto:michael.d.kinney@intel.com>>
>>>>> Cc: devel@edk2.groups.io <mailto:devel@edk2.groups.io>; Laszlo Ersek <lersek@redhat.com <mailto:lersek@redhat.com>>; Andrew Fish <afish@apple.com <mailto:afish@apple.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>>
>>>>> 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@intel.com <mailto: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
>>>>>
>>>>>
>>>> 
>>>
> 
> 

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

[Vitaly Cheptsov]

[-- Attachment #1: Type: text/plain, Size: 15492 bytes --]

Laszlo,

Thanks for the quick response. I am unsure whether EDK II mailing list will be happy with 30 letters but I will batch-send it now. The patch is hardly a feature but rather a longstanding, which was scheduled for May release in any case. I believe we discussed most of the details by this time, and I hope only some small nuances are left.

Best wishes,
Vitaly

> 11 мая 2020 г., в 18:19, Laszlo Ersek <lersek@redhat.com> написал(а):
> 
> On 05/11/20 14:03, Vitaly Cheptsov wrote:
>> 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>
> 
> github pull requests are only used -- at this time -- by contributors
> for personal CI runs, and by edk2 maintainers for merging series that
> have been reviewed on the list. Patch review remains mailing list-based,
> for now. Please post the patches to the list for review.
> 
>> Let me know if any further changes are needed from my side. I hope this still is in time for the May tag.
> 
> If this work counts as a feature, then its review has to complete by the
> soft feature freeze (2020-05-15).
> 
> Thanks,
> Laszlo
> 
>> 
>> Best wishes,
>> Vitaly
>> 
>>> 19 марта 2020 г., в 03:04, Vitaly Cheptsov <cheptsov@ispras.ru> написал(а):
>>> 
>>> 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 <afish@apple.com> 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 <cheptsov@ispras.ru <mailto:cheptsov@ispras.ru>> 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 <michael.d.kinney@intel.com <mailto:michael.d.kinney@intel.com>> 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 <cheptsov@ispras.ru <mailto:cheptsov@ispras.ru>>
>>>>>> Sent: Wednesday, March 18, 2020 1:43 PM
>>>>>> To: Kinney, Michael D <michael.d.kinney@intel.com <mailto:michael.d.kinney@intel.com>>
>>>>>> Cc: devel@edk2.groups.io <mailto:devel@edk2.groups.io>; Laszlo Ersek <lersek@redhat.com <mailto:lersek@redhat.com>>; Andrew Fish <afish@apple.com <mailto:afish@apple.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>>
>>>>>> 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@intel.com <mailto: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 #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]