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). 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
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.)

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.

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.)

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

(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