Re: [PATCH V4 00/27] Disabling safe string constraint assertions

["Michael D Kinney" <michael.d.kinney@intel.com>]

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.  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.  For module INF files,
we require the INF file to list all the lib classes that the
module sources directly use.  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.  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.
The INF files for all modules that use the DebugLib should
also be updated to list the DebugCommonLib.  If we go down
that path, then it would be cleaner for the modules to include
both DebugLib.h and DebugCommonLib.h so the list of includes
matches the list of lib classes in the INF file.  This would
be an even much larger change than the patch series already
under review.

I think there is general agreement that the ASSERT_CONSTRAINT()
feature is valuable, and the sticking point has been the 
complexity of the change and the impact to the existing modules
and platforms.

In order to address the original problem statement and 
Bugzilla: https://bugzilla.tianocore.org/show_bug.cgi?id=2054
Perhaps we should go back to the original proposal that 
adds one new PCD so the string APIs in the BaseLib can
filter out ASSERT() conditions for UEFI Applications that
want return status behavior without ASSERT() behavior.

The work on this patch series and the other proposals have
all been very valuable and we can continue to look for ways
to implement the general purpose ASSERT_CONSTRAINT() without
so many module and platform side effects.

Thanks,

Mike

> -----Original Message-----
> From: Vitaly Cheptsov <cheptsov@ispras.ru>
> Sent: Monday, May 11, 2020 8:41 AM
> To: devel@edk2.groups.io
> Cc: 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>; Laszlo Ersek
> <lersek@redhat.com>
> Subject: [PATCH V4 00/27] Disabling safe string
> constraint assertions
> 
> Cc: Andrew Fish <afish@apple.com>
> Cc: Mike Kinney <michael.d.kinney@intel.com>
> Cc: Marvin Häuser <mhaeuser@outlook.de>
> Cc: Liming Gao <liming.gao@intel.com>
> Cc: Zhichao Gao <zhichao.gao@intel.com>
> CC: Laszlo Ersek <lersek@redhat.com>
> 
> This changesest hopefully finally resolves the
> longstanding
> https://bugzilla.tianocore.org/show_bug.cgi?id=2054
> 
> Requesting to merge into edk2-stable202005.
> 
> Vitaly Cheptsov (27):
>   MdePkg: Introduce DebugCommonLib interface and
> BaseDebugCommonLib
>   UnitTestFrameworkPkg: Add support for DebugCommonLib
>   MdePkg: Add support for DebugCommonLib
>   MdeModulePkg: Add support for DebugCommonLib
>   ArmPkg: Add support for DebugCommonLib
>   ArmPlatformPkg: Add support for DebugCommonLib
>   ArmVirtPkg: Add support for DebugCommonLib
>   CryptoPkg: Add support for DebugCommonLib
>   DynamicTablesPkg: Add support for DebugCommonLib
>   EmbeddedPkg: Add support for DebugCommonLib
>   EmulatorPkg: Add support for DebugCommonLib
>   FatPkg: Add support for DebugCommonLib
>   FmpDevicePkg: Add support for DebugCommonLib
>   IntelFsp2Pkg: Add support for DebugCommonLib
>   IntelFsp2WrapperPkg: Add support for DebugCommonLib
>   OvmfPkg: Add support for DebugCommonLib
>   NetworkPkg: Add support for DebugCommonLib
>   ShellPkg: Add support for DebugCommonLib
>   SecurityPkg: Add support for DebugCommonLib
>   PcAtChipsetPkg: Add support for DebugCommonLib
>   SignedCapsulePkg: Add support for DebugCommonLib
>   SourceLevelDebugPkg: Add support for DebugCommonLib
>   StandaloneMmPkg: Add support for DebugCommonLib
>   UefiCpuPkg: Add support for DebugCommonLib
>   UefiPayloadPkg: Add support for DebugCommonLib
>   MdePkg: Introduce assertion on constraint debug mask
> bit
>   MdePkg: Use assertion on constraint violation bit in
> SafeString
> 
>  ArmPkg/ArmPkg.dsc
> |   1 +
>  ArmPkg/Drivers/ArmCrashDumpDxe/ArmCrashDumpDxe.dsc
> |   1 +
>  ArmPkg/Library/SemiHostingDebugLib/DebugLib.c
> |  84 ----------
> 
> ArmPkg/Library/SemiHostingDebugLib/SemiHostingDebugLib.
> inf                             |   3 +-
>  ArmPlatformPkg/ArmPlatformPkg.dsc
> |   1 +
>  ArmVirtPkg/ArmVirt.dsc.inc
> |   1 +
>  CryptoPkg/CryptoPkg.dsc
> |   2 +
>  DynamicTablesPkg/DynamicTablesPkg.dsc
> |   1 +
>  EmbeddedPkg/EmbeddedPkg.dsc
> |   1 +
>  EmulatorPkg/EmulatorPkg.dsc
> |   1 +
>  FatPkg/FatPkg.dsc
> |   1 +
>  FmpDevicePkg/FmpDevicePkg.dsc
> |   1 +
>  IntelFsp2Pkg/IntelFsp2Pkg.dsc
> |   1 +
> 
> IntelFsp2Pkg/Library/BaseFspDebugLibSerialPort/BaseFspD
> ebugLibSerialPort.inf           |   1 +
> 
> IntelFsp2Pkg/Library/BaseFspDebugLibSerialPort/DebugLib
> .c                              |  97 -----------
>  IntelFsp2WrapperPkg/IntelFsp2WrapperPkg.dsc
> |   1 +
>  MdeModulePkg/Library/PeiDebugLibDebugPpi/DebugLib.c
> | 100 ------------
> 
> MdeModulePkg/Library/PeiDebugLibDebugPpi/PeiDebugLibDeb
> ugPpi.inf                       |   1 +
> 
> MdeModulePkg/Library/PeiDxeDebugLibReportStatusCode/Deb
> ugLib.c                         |  98 -----------
> 
> MdeModulePkg/Library/PeiDxeDebugLibReportStatusCode/Pei
> DxeDebugLibReportStatusCode.inf |   1 +
>  MdeModulePkg/MdeModulePkg.dsc
> |   1 +
>  MdePkg/Include/Library/BaseLib.h
> | 120 +++++++-------
>  MdePkg/Include/Library/DebugCommonLib.h
> | 172 ++++++++++++++++++++
>  MdePkg/Include/Library/DebugLib.h
> | 155 +++---------------
> 
> MdePkg/Library/BaseDebugCommonLib/BaseDebugCommonLib.in
> f                               |  39 +++++
> 
> MdePkg/Library/BaseDebugCommonLib/BaseDebugCommonLib.un
> i                               |  16 ++
>  MdePkg/Library/BaseDebugCommonLib/DebugCommonLib.c
> | 133 +++++++++++++++
>  MdePkg/Library/BaseDebugLibNull/BaseDebugLibNull.inf
> |  10 ++
>  MdePkg/Library/BaseDebugLibNull/DebugLib.c
> |  98 -----------
> 
> MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSeria
> lPort.inf                       |   1 +
>  MdePkg/Library/BaseDebugLibSerialPort/DebugLib.c
> |  98 -----------
>  MdePkg/Library/BaseLib/BaseLib.inf
> |   1 +
>  MdePkg/Library/BaseLib/SafeString.c
> |   2 +-
>  MdePkg/Library/DxeRuntimeDebugLibSerialPort/DebugLib.c
> |  98 -----------
> 
> MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeD
> ebugLibSerialPort.inf           |   1 +
>  MdePkg/Library/UefiDebugLibConOut/DebugLib.c
> |  98 -----------
> 
> MdePkg/Library/UefiDebugLibConOut/UefiDebugLibConOut.in
> f                               |   1 +
> 
> MdePkg/Library/UefiDebugLibDebugPortProtocol/DebugLib.c
> |  99 -----------
> 
> MdePkg/Library/UefiDebugLibDebugPortProtocol/UefiDebugL
> ibDebugPortProtocol.inf         |   1 +
>  MdePkg/Library/UefiDebugLibStdErr/DebugLib.c
> |  98 -----------
> 
> MdePkg/Library/UefiDebugLibStdErr/UefiDebugLibStdErr.in
> f                               |   1 +
>  MdePkg/MdePkg.dec
> |   6 +-
>  MdePkg/MdePkg.dsc
> |   1 +
>  MdePkg/MdePkg.uni
> |   3 +-
>  NetworkPkg/NetworkPkg.dsc
> |   1 +
>  OvmfPkg/Library/PlatformDebugLibIoPort/DebugLib.c
> |  98 -----------
> 
> OvmfPkg/Library/PlatformDebugLibIoPort/PlatformRomDebug
> LibIoPort.inf                   |   1 +
>  OvmfPkg/OvmfPkgIa32.dsc
> |   1 +
>  OvmfPkg/OvmfPkgIa32X64.dsc
> |   1 +
>  OvmfPkg/OvmfPkgX64.dsc
> |   1 +
>  OvmfPkg/OvmfXen.dsc
> |   1 +
>  PcAtChipsetPkg/PcAtChipsetPkg.dsc
> |   1 +
>  SecurityPkg/SecurityPkg.dsc
> |   1 +
>  ShellPkg/ShellPkg.dsc
> |   1 +
>  SignedCapsulePkg/SignedCapsulePkg.dsc
> |   1 +
>  SourceLevelDebugPkg/SourceLevelDebugPkg.dsc
> |   1 +
>  StandaloneMmPkg/StandaloneMmPkg.dsc
> |   1 +
>  UefiCpuPkg/UefiCpuPkg.dsc
> |   1 +
>  UefiPayloadPkg/UefiPayloadPkgIa32.dsc
> |   1 +
>  UefiPayloadPkg/UefiPayloadPkgIa32X64.dsc
> |   1 +
> 
> UnitTestFrameworkPkg/Library/Posix/DebugLibPosix/DebugL
> ibPosix.c                       |  94 -----------
> 
> UnitTestFrameworkPkg/Library/Posix/DebugLibPosix/DebugL
> ibPosix.inf                     |   1 +
>  UnitTestFrameworkPkg/UnitTestFrameworkPkgHost.dsc.inc
> |   1 +
> 
> UnitTestFrameworkPkg/UnitTestFrameworkPkgTarget.dsc.inc
> |   1 +
>  64 files changed, 501 insertions(+), 1360 deletions(-)
>  create mode 100644
> MdePkg/Include/Library/DebugCommonLib.h
>  create mode 100644
> MdePkg/Library/BaseDebugCommonLib/BaseDebugCommonLib.in
> f
>  create mode 100644
> MdePkg/Library/BaseDebugCommonLib/BaseDebugCommonLib.un
> i
>  create mode 100644
> MdePkg/Library/BaseDebugCommonLib/DebugCommonLib.c
> 
> --
> 2.24.2 (Apple Git-127)