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 ). I also resubmitted the V5 of the patchset including all the requested changes. > You have included the 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.) 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 написал(а): > > 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 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 >