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

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

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

Hello everyone,

Let me try in order.

After an internal review I added the following to SafeString commit message:
Note, for packages with constraint assertions disabled this change
turn off the assertions on constraint violations.
This is obvious, and I believe everyone agreed it is not healthy except for some private packages but I guess it is worth mentioning, so that it is cared in these private packages as necessary.

The latest version of the changes  is on GitHub branch (https://github.com/vit9696/edk2/tree/constraint <https://github.com/vit9696/edk2/tree/constraint>). I also resubmitted the V5 of the patchset including all the requested changes.


> 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 do not think it is unusual to include one library header from the other. That’s what we regularly do in our code. I also do not agree that we need to add a dependency on a library consumed by the other library. This is an indirect dependency. Just like Laszlo says.

> In order to address the original problem statement and
> Bugzilla: https://bugzilla.tianocore.org/show_bug.cgi?id=2054 <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.


If you believe it is not possible to merge the current patch into May release, I am fine to submit a small change that will add a PCD to protect SafeString. However, I am not very happy about it as it may take a considerable amount of time to get rid of this PCD in the future, and we (and likely other people) will have to change the code multiple times. I really want to resolve this sick situation with both BaseLib and DebugLib once and forever, as the need to change the hell of DebugLibs all over the place for a small thing is very very wrong.

> Note: unfortunately, the edk2-platforms tree contains a huge number of
> DebugLib class resolutions (124 resolutions in 48 files, at commit
> bfa32ac70a75), and I think the final patch in the present series will
> break those platforms unless you post patches for them too.
> 
> IMO this is another good example why edk2-platforms should *not* exist
> as a separate repository.

I agree that separating edk2-platforms is very sick as well. I can send the patches on them too, but due to their amount (just like you said it will be almost half a hundred) I would rather make a branch and then just merge it. Same for EDK II (e.g. somewhere in edk2-staging). In addition to that is very hard for me to read the review of the patches by e-mails, and spamming the mailing list feels also not so great.

> (... Please consider adding Cc: tags to the commit messages, so that
> each patch email reaches the subject package owners directly too. Anyway
> that's for the future.)


Fixed.

> I think the very last hunk in this patch belongs in patch#26 ("MdePkg:
> Introduce assertion on constraint debug mask bit"), which is where BIT6
> is actually defined for PcdDebugPropertyMask.

> (2) The "MdePkg/MdePkg.uni" file update from patch#1 belongs here, IMO.


Fixed.

> I think the VALID_ARCHITECTURES comment should be dropped altogether.
> 
> At least the current arch list "IA32 X64 EBC" is incorrect; first
> because ARM and AARCH64 are missing (despite the series -- correctly --
> patching Arm*Pkg and such), and second because -- I *think* -- EBC is no
> longer an arch natively supported by edk2. (Mike and Liming will correct
> me on that, if needed.)

Fixed.

> (1) The other two INF files in the same directory should get the same
> update. (I.e., a dependency on DebugCommonLib.)

Nice catch, fixed.

> (2) I believe it should be possible to remove
> "PcdFixedDebugPrintErrorLevel" from all three INF files in the same
> directory. (The only reference, which is being removed, is in
> DebugPrintLevelEnabled().)


This issue is common across the patches, fixed everywhere.

> (1) Ugh, sorry, my point (1) was going to be about out two instances of
> the same typo:
> 
> s/constrain/constraint/

There also were a couple of cases with constrait. Fixed everything.

Best regards,
Vitaly

> 12 мая 2020 г., в 12:50, Laszlo Ersek <lersek@redhat.com> написал(а):
> 
> 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 #1.2: Type: text/html, Size: 18210 bytes --]

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