From: "Vitaly Cheptsov" <cheptsov@ispras.ru>
To: "Gao, Liming" <liming.gao@intel.com>
Cc: "Kinney, Michael D" <michael.d.kinney@intel.com>,
"lersek@redhat.com" <lersek@redhat.com>,
"devel@edk2.groups.io" <devel@edk2.groups.io>,
"Andrew Fish" <afish@apple.com>,
"Marvin Häuser" <mhaeuser@outlook.de>,
"Gao, Zhichao" <zhichao.gao@intel.com>
Subject: Re: [edk2-devel] [PATCH V4 00/27] Disabling safe string constraint assertions
Date: Wed, 13 May 2020 21:37:18 +0300 [thread overview]
Message-ID: <8EA54782-5A1C-4767-A0C1-0837D9A60248@ispras.ru> (raw)
In-Reply-To: <BN6PR11MB3972BC99674340E74FD2B45580BF0@BN6PR11MB3972.namprd11.prod.outlook.com>
[-- Attachment #1: Type: text/plain, Size: 12529 bytes --]
Liming,
Firstly, could you please explain why did not you suggest anything alike a couple of months ago when I proposed exactly the same solution and posted a patch asking how to workaround the undeclared identifier issue?
I am somewhat unhappy here doing rounds. I propose a solution, somebody from your team says it is wrong, I propose another solution, you say it is wrong again, months later you start suggesting me to use one of my older solutions but «slightly» change it, so that it starts to look ugly (and I do not mean just this time, it has been at least trice). If your team wants to continue to sabotage any attempts to fix it we could consider ceasing any relationship with EDK II, but I want to believe it is just some misunderstanding which unfortunately escalated this much.
Secondly, «being compatible to the current platform» and leaving on by default is something that nobody really needs. I explicitly mentioned that the defaults are changed in the latest commit message, and so far nobody objects about it. My opinion is that the ill behaviour we have now should never ever be the default. Be that compatibility or not. If you need it for some packages — override it. In fact, Also, in fact I once made the PCD inverted a few revisions ago… and somebody asked to change it :D
Thirdly, I am not happy about code duplication in the debug lib we have now. This makes changing anything in DebugLib a very hard matter. Just like Laszlo says, if DebugLib is frozen forever, we should not even attempt to change things there. It sounds insane if true.
All in all, several people including us, guys from Microsoft and Red Hat have already told you many times that the reasons behind keeping this assertion are illicit in the first place. Perhaps, it should really be just removed in a single small patch without any PCDs and we can just get over it once and forever. If you believe it is not the case, perhaps you could submit a patch on your own? We will review it as soon as possible.
Best wishes,
Vitaly
[1] https://edk2.groups.io/g/devel/message/55961
> 13 мая 2020 г., в 20:59, Gao, Liming <liming.gao@intel.com> написал(а):
>
> Vitaly:
> I would like to propose one solution to update DebugLib.h only. This solution directly declares PcdDebugPropertyMask global variable in DebugLib.h. Then, DebugLib macro can use it. Because most of DebugLib library instances describe PcdDebugPropertyMask in their INF, this PCD can be generated without the additional change. I also evaluate PcdDebugPropertyMask usage in existing platform. This PCD is always used as FixedAtBuild. So, I change PcdDebugPropertyMask type in DEC to FixedAtBuild. This change impact should be smaller than current patch set. Below patch passes the build on OvmfPkg. It should work.
>
> Besides, new PcdDebugPropertyMask BIT6 for Treat constraint violations as ASSERT. This BIT should be enabled by default to be compatible with current platform. Now, most platforms set this PCD PcdDebugPropertyMask in their platform DSC file. PCD value is also required to be updated. Another compatible way is to define BIT6 for Treat constraint violations as empty. When BIT6 is 1, it means constraint violation as empty. When BIT6 is 0, it means constraint violation as ASSERT. If so, this PCD value is not required to be changed in platform DSC.
>
> Thanks
> Liming
> ---
> MdePkg/Include/Library/DebugLib.h | 7 ++++++-
> .../Library/BaseDebugLibNull/BaseDebugLibNull.inf | 4 ++++
> MdePkg/MdePkg.dec | 22 +++++++++++-----------
> 3 files changed, 21 insertions(+), 12 deletions(-)
>
> diff --git a/MdePkg/Include/Library/DebugLib.h b/MdePkg/Include/Library/DebugLib.h
> index baab34bf05..68604de869 100644
> --- a/MdePkg/Include/Library/DebugLib.h
> +++ b/MdePkg/Include/Library/DebugLib.h
> @@ -321,6 +321,11 @@ DebugPrintLevelEnabled (
> #define _DEBUG(Expression) DebugPrint Expression
> #endif
>
> +//
> +// FixedAtBuild PCD value PcdDebugPropertyMask
> +//
> +extern const UINT8 _gPcd_FixedAtBuild_PcdDebugPropertyMask;
> +
> /**
> Macro that calls DebugAssert() if an expression evaluates to FALSE.
>
> @@ -336,7 +341,7 @@ DebugPrintLevelEnabled (
> #if !defined(MDEPKG_NDEBUG)
> #define ASSERT(Expression) \
> do { \
> - if (DebugAssertEnabled ()) { \
> + if ((DEBUG_PROPERTY_DEBUG_ASSERT_ENABLED & _gPcd_FixedAtBuild_PcdDebugPropertyMask) != 0) { \
> if (!(Expression)) { \
> _ASSERT (Expression); \
> ANALYZER_UNREACHABLE (); \
> diff --git a/MdePkg/Library/BaseDebugLibNull/BaseDebugLibNull.inf b/MdePkg/Library/BaseDebugLibNull/BaseDebugLibNull.inf
> index 81a63a5074..1e95a2f077 100644
> --- a/MdePkg/Library/BaseDebugLibNull/BaseDebugLibNull.inf
> +++ b/MdePkg/Library/BaseDebugLibNull/BaseDebugLibNull.inf
> @@ -29,3 +29,7 @@
> [Packages]
> MdePkg/MdePkg.dec
>
> +[Pcd]
> + # This PCD is consumed in DebugLib.h
> + gEfiMdePkgTokenSpaceGuid.PcdDebugPropertyMask ## CONSUMES
> +
> diff --git a/MdePkg/MdePkg.dec b/MdePkg/MdePkg.dec
> index 0ab09195c1..f8aacdf428 100644
> --- a/MdePkg/MdePkg.dec
> +++ b/MdePkg/MdePkg.dec
> @@ -2105,6 +2105,17 @@
> # @Prompt Speculation Barrier Type.
> gEfiMdePkgTokenSpaceGuid.PcdSpeculationBarrierType|0x01|UINT8|0x30001018
>
> + ## The mask is used to control DebugLib behavior.<BR><BR>
> + # BIT0 - Enable Debug Assert.<BR>
> + # BIT1 - Enable Debug Print.<BR>
> + # BIT2 - Enable Debug Code.<BR>
> + # BIT3 - Enable Clear Memory.<BR>
> + # BIT4 - Enable BreakPoint as ASSERT.<BR>
> + # BIT5 - Enable DeadLoop as ASSERT.<BR>
> + # @Prompt Debug Property.
> + # @Expression 0x80000002 | (gEfiMdePkgTokenSpaceGuid.PcdDebugPropertyMask & 0xC0) == 0
> + gEfiMdePkgTokenSpaceGuid.PcdDebugPropertyMask|0|UINT8|0x00000005
> +
> [PcdsFixedAtBuild,PcdsPatchableInModule]
> ## Indicates the maximum length of unicode string used in the following
> # BaseLib functions: StrLen(), StrSize(), StrCmp(), StrnCmp(), StrCpy(), StrnCpy()<BR><BR>
> @@ -2139,17 +2150,6 @@
> # @Prompt Spin Lock Timeout (us).
> gEfiMdePkgTokenSpaceGuid.PcdSpinLockTimeout|10000000|UINT32|0x00000004
>
> - ## The mask is used to control DebugLib behavior.<BR><BR>
> - # BIT0 - Enable Debug Assert.<BR>
> - # BIT1 - Enable Debug Print.<BR>
> - # BIT2 - Enable Debug Code.<BR>
> - # BIT3 - Enable Clear Memory.<BR>
> - # BIT4 - Enable BreakPoint as ASSERT.<BR>
> - # BIT5 - Enable DeadLoop as ASSERT.<BR>
> - # @Prompt Debug Property.
> - # @Expression 0x80000002 | (gEfiMdePkgTokenSpaceGuid.PcdDebugPropertyMask & 0xC0) == 0
> - gEfiMdePkgTokenSpaceGuid.PcdDebugPropertyMask|0|UINT8|0x00000005
> -
> ## This flag is used to control the print out Debug message.<BR><BR>
> # BIT0 - Initialization message.<BR>
> # BIT1 - Warning message.<BR>
> --
>
> Thanks
> Liming
>> -----Original Message-----
>> From: Vitaly Cheptsov <cheptsov@ispras.ru>
>> Sent: Wednesday, May 13, 2020 2:58 AM
>> To: Kinney, Michael D <michael.d.kinney@intel.com>; lersek@redhat.com
>> Cc: devel@edk2.groups.io; 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] [PATCH V4 00/27] Disabling safe string constraint assertions
>>
>> Mike,
>>
>> I see what you mean here, but I believe it is absolutely fine to implement DebugCommonLib.h interface within DebugLib without
>> depending on DebugCommonLib if one absolutely desires it (thought I do not think it is a good idea).
>> In this case, perhaps we can avoid adding DebugCommonLib.h header at all and leave it all in DebugLib.h?
>> So that DebugCommonLib library is entirely private thing to DebugLib, which can either use it or not.
>>
>> Best wishes,
>> Vitaly
>>
>>> 12 мая 2020 г., в 21:18, Kinney, Michael D <michael.d.kinney@intel.com> написал(а):
>>>
>>> Laszlo,
>>>
>>> A library implementation that uses another library class
>>> lists those library classes in the library INF. This is
>>> a way a module can inherit the use of a second lib without
>>> listing the second lib in the module INF. This is the
>>> type of inheritance that is supported by the EDK II build
>>> system and the EDK II meta data files.
>>>
>>> What is not supported today is indirect inheritance from
>>> the libclass .h file itself. There is no mechanism for
>>> the libclass .h file to declare it is using another library
>>> class. This would require extensions to the meta data
>>> files to declare this type dependency.
>>>
>>> How does the build system know to add DebugCommonLib to
>>> the link command for a module that only lists DebugLib
>>> in its library classes section? What if there is a
>>> DebugLib implementation of the DebugLib class that
>>> does not depend on DebugCommonLib. The module link
>>> will then fail when the module writer followed all the
>>> rules. This is why including a libclass from another
>>> libclass is bad idea.
>>>
>>> Mike
>>>
>>>> -----Original Message-----
>>>> From: devel@edk2.groups.io <devel@edk2.groups.io> On
>>>> Behalf Of Laszlo Ersek
>>>> Sent: Tuesday, May 12, 2020 2:51 AM
>>>> To: Kinney, Michael D <michael.d.kinney@intel.com>;
>>>> Vitaly Cheptsov <cheptsov@ispras.ru>;
>>>> devel@edk2.groups.io
>>>> Cc: 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] [PATCH V4 00/27] Disabling
>>>> safe string constraint assertions
>>>>
>>>> On 05/12/20 00:40, Kinney, Michael D wrote:
>>>>> Vitaly,
>>>>>
>>>>> Thank you for the contribution.
>>>>>
>>>>> There are a couple points about this approach that
>>>> need to be discussed.
>>>>>
>>>>> You have included the <Library/DebugCommonLib.h> from
>>>>> MdePkg/Include/Library/DebugLib.h.
>>>>
>>>> Right, I've noticed it. I agree it's unusual. I didn't
>>>> think it was wrong.
>>>>
>>>>> It is very rare for a
>>>>> lib class to include another lib class. This means
>>>> that a module
>>>>> that has a dependency on the DebugLib class inherits
>>>> a hidden
>>>>> dependency on the DebugCommonLib class.
>>>>
>>>> I agree.
>>>>
>>>> I think it should be fine. Any header H1 should include
>>>> such further
>>>> headers H2, H3, ... Hn that are required for making the
>>>> interfaces
>>>> declared in H1 usable in client modules.
>>>>
>>>>> For module INF files,
>>>>> we require the INF file to list all the lib classes
>>>> that the
>>>>> module sources directly use.
>>>>
>>>> Yes, keyword being "directly".
>>>>
>>>>> Since a module that uses the
>>>>> DebugLib uses the ASSERT() and DEBUG() macros, all
>>>> the APIs
>>>>> that the ASSERT() and DEBUG() macros use are also
>>>> directly
>>>>> used by the module.
>>>>
>>>> I believe this is where I disagree. The replacement
>>>> texts of the
>>>> ASSERT() and DEBUG() function-like macros are internals
>>>> of the
>>>> DebugLib.h lib class header, in my opinion. Those
>>>> internals place
>>>> requirements on specific DebugLib instances, not on
>>>> DebugLib class
>>>> consumers.
>>>>
>>>> In other words, when writing a new DebugLib instance,
>>>> the implementor
>>>> has to ensure that the ASSERT() and DEBUG() macros, as
>>>> defined in the
>>>> DebugLib class header, will continue working in
>>>> DebugLib consumer
>>>> modules. The implementor may then choose to make the
>>>> new DebugLib
>>>> instance dependent on the (singleton) DebugCommonLib
>>>> instance, for
>>>> example. (This is being done in patches #3, #4, #16,
>>>> maybe more.) The
>>>> DebugLib consumer module will inherit that dependency,
>>>> and everything
>>>> will work.
>>>>
>>>> Just because ASSERT() and DEBUG() are function-like
>>>> macros and not
>>>> actual functions, I don't think the INF file
>>>> requirements in
>>>> DebugLib-consumer modules should change.
>>>>
>>>>> With this patch series, these macros
>>>>> now use the DebugCommonLib class APIs, which means
>>>> any module
>>>>> that uses the DebugLib also directly uses the
>>>> DebugCommonLib.
>>>>
>>>> In my opinion: indirectly.
>>>>
>>>> Thanks,
>>>> Laszlo
>>>>
>>>>
>>>>
>>>
>
[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
next prev parent reply other threads:[~2020-05-13 18:37 UTC|newest]
Thread overview: 48+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-05-11 15:40 [PATCH V4 00/27] Disabling safe string constraint assertions Vitaly Cheptsov
2020-05-11 15:40 ` [PATCH V4 01/27] MdePkg: Introduce DebugCommonLib interface and BaseDebugCommonLib Vitaly Cheptsov
2020-05-11 20:37 ` [edk2-devel] " Laszlo Ersek
2020-05-11 20:42 ` Laszlo Ersek
2020-05-11 15:40 ` [PATCH V4 02/27] UnitTestFrameworkPkg: Add support for DebugCommonLib Vitaly Cheptsov
2020-05-11 15:40 ` [PATCH V4 03/27] MdePkg: " Vitaly Cheptsov
2020-05-11 15:40 ` [PATCH V4 04/27] MdeModulePkg: " Vitaly Cheptsov
2020-05-11 15:40 ` [PATCH V4 05/27] ArmPkg: " Vitaly Cheptsov
2020-05-11 15:41 ` [PATCH V4 06/27] ArmPlatformPkg: " Vitaly Cheptsov
2020-05-11 15:41 ` [PATCH V4 07/27] ArmVirtPkg: " Vitaly Cheptsov
2020-05-11 20:42 ` [edk2-devel] " Laszlo Ersek
2020-05-11 15:41 ` [PATCH V4 08/27] CryptoPkg: " Vitaly Cheptsov
2020-05-11 15:41 ` [PATCH V4 09/27] DynamicTablesPkg: " Vitaly Cheptsov
2020-05-11 15:41 ` [PATCH V4 10/27] EmbeddedPkg: " Vitaly Cheptsov
2020-05-11 15:41 ` [PATCH V4 11/27] EmulatorPkg: " Vitaly Cheptsov
2020-05-11 15:41 ` [PATCH V4 12/27] FatPkg: " Vitaly Cheptsov
2020-05-11 15:41 ` [PATCH V4 13/27] FmpDevicePkg: " Vitaly Cheptsov
2020-05-11 15:41 ` [PATCH V4 14/27] IntelFsp2Pkg: " Vitaly Cheptsov
2020-05-12 0:49 ` [edk2-devel] " Chiu, Chasel
2020-05-11 15:41 ` [PATCH V4 15/27] IntelFsp2WrapperPkg: " Vitaly Cheptsov
2020-05-12 0:47 ` [edk2-devel] " Chiu, Chasel
2020-05-11 15:41 ` [PATCH V4 16/27] OvmfPkg: " Vitaly Cheptsov
2020-05-11 20:49 ` [edk2-devel] " Laszlo Ersek
2020-05-11 15:41 ` [PATCH V4 17/27] NetworkPkg: " Vitaly Cheptsov
2020-05-11 15:41 ` [PATCH V4 18/27] ShellPkg: " Vitaly Cheptsov
2020-05-11 15:41 ` [PATCH V4 19/27] SecurityPkg: " Vitaly Cheptsov
2020-05-11 15:41 ` [PATCH V4 20/27] PcAtChipsetPkg: " Vitaly Cheptsov
2020-05-11 15:41 ` [PATCH V4 21/27] SignedCapsulePkg: " Vitaly Cheptsov
2020-05-11 15:41 ` [PATCH V4 22/27] SourceLevelDebugPkg: " Vitaly Cheptsov
2020-05-11 15:41 ` [PATCH V4 23/27] StandaloneMmPkg: " Vitaly Cheptsov
2020-05-11 15:41 ` [PATCH V4 24/27] UefiCpuPkg: " Vitaly Cheptsov
2020-05-11 20:52 ` [edk2-devel] " Laszlo Ersek
2020-05-11 15:41 ` [PATCH V4 25/27] UefiPayloadPkg: " Vitaly Cheptsov
2020-05-11 15:41 ` [PATCH V4 26/27] MdePkg: Introduce assertion on constraint debug mask bit Vitaly Cheptsov
2020-05-11 20:58 ` [edk2-devel] " Laszlo Ersek
2020-05-11 22:12 ` Laszlo Ersek
2020-05-11 15:41 ` [PATCH V4 27/27] MdePkg: Use assertion on constraint violation bit in SafeString Vitaly Cheptsov
2020-05-11 21:04 ` [edk2-devel] " Laszlo Ersek
2020-05-11 22:40 ` [PATCH V4 00/27] Disabling safe string constraint assertions Michael D Kinney
2020-05-12 9:50 ` Laszlo Ersek
2020-05-12 17:03 ` Vitaly Cheptsov
2020-05-12 18:18 ` [edk2-devel] " Michael D Kinney
2020-05-12 18:57 ` Vitaly Cheptsov
2020-05-13 17:59 ` Liming Gao
2020-05-13 18:37 ` Vitaly Cheptsov [this message]
2020-05-13 9:16 ` Laszlo Ersek
2020-05-13 14:41 ` [EXTERNAL] " Bret Barkelew
2020-05-13 20:14 ` Brian J. Johnson
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=8EA54782-5A1C-4767-A0C1-0837D9A60248@ispras.ru \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox