<html><head><meta http-equiv="Content-Type" content="text/html; charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class="">Hello everyone,<div class=""><br class=""></div><div class="">Let me try in order.</div><div class=""><br class=""></div><div class="">After an internal review I added the following to SafeString commit message:</div><div class="">Note, for packages with constraint assertions disabled this change</div><div class=""><div class="">turn off the assertions on constraint violations.</div></div><div class="">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.</div><div class=""><br class=""></div><div class="">The latest version of the changes is<span style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);" class=""> on GitHub branch (</span><font color="#000000" class=""><a href="https://github.com/vit9696/edk2/tree/constraint" class="">https://github.com/vit9696/edk2/tree/constraint</a>)</font><span style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);" class="">. I also resubmitted the V5 of the patchset including all the requested changes.</span></div><div class=""><br class=""></div><div class=""><br class=""></div><div class=""><blockquote type="cite" class=""><span style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);" class="">You have included the <Library/DebugCommonLib.h> from </span><br style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);" class=""><span style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);" class="">MdePkg/Include/Library/DebugLib.h. It is very rare for a </span><br style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);" class=""><span style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);" class="">lib class to include another lib class. This means that a module</span><br style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);" class=""><span style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);" class="">that has a dependency on the DebugLib class inherits a hidden</span><br style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);" class=""><span style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);" class="">dependency on the DebugCommonLib class. For module INF files,</span><br style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);" class=""><span style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);" class="">we require the INF file to list all the lib classes that the</span><br style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);" class=""><span style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);" class="">module sources directly use. Since a module that uses the</span><br style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);" class=""><span style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);" class="">DebugLib uses the ASSERT() and DEBUG() macros, all the APIs</span><br style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);" class=""><span style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);" class="">that the ASSERT() and DEBUG() macros use are also directly</span><br style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);" class=""><span style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);" class="">used by the module. With this patch series, these macros</span><br style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);" class=""><span style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);" class="">now use the DebugCommonLib class APIs, which means any module</span><br style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);" class=""><span style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);" class="">that uses the DebugLib also directly uses the DebugCommonLib.</span><br style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);" class=""><span style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);" class="">The INF files for all modules that use the DebugLib should</span><br style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);" class=""><span style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);" class="">also be updated to list the DebugCommonLib. If we go down</span><br style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);" class=""><span style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);" class="">that path, then it would be cleaner for the modules to include</span><br style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);" class=""><span style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);" class="">both DebugLib.h and DebugCommonLib.h so the list of includes</span><br style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);" class=""><span style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);" class="">matches the list of lib classes in the INF file. This would</span><br style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);" class=""><span style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);" class="">be an even much larger change than the patch series already</span><br style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);" class=""><span style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);" class="">under review.</span></blockquote><br class=""></div><div class="">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.</div><div class=""><br class=""></div><div class=""><blockquote type="cite" class=""><span style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);" class="">In order to address the original problem statement and </span><br style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);" class=""><span style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);" class="">Bugzilla: </span><a href="https://bugzilla.tianocore.org/show_bug.cgi?id=2054" class="">https://bugzilla.tianocore.org/show_bug.cgi?id=2054</a><br style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);" class=""><span style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);" class="">Perhaps we should go back to the original proposal that </span><br style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);" class=""><span style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);" class="">adds one new PCD so the string APIs in the BaseLib can</span><br style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);" class=""><span style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);" class="">filter out ASSERT() conditions for UEFI Applications that</span><br style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);" class=""><span style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);" class="">want return status behavior without ASSERT() behavior.</span></blockquote></div><div class=""><br class=""></div><div class="">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.</div><div class=""><br class=""></div><div class=""><blockquote type="cite" class=""><span style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);" class="">Note: unfortunately, the edk2-platforms tree contains a huge number of</span><br style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);" class=""><span style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);" class="">DebugLib class resolutions (124 resolutions in 48 files, at commit</span><br style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);" class=""><span style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);" class="">bfa32ac70a75), and I think the final patch in the present series will</span><br style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);" class=""><span style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);" class="">break those platforms unless you post patches for them too.</span><br style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);" class=""><br style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);" class=""><span style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);" class="">IMO this is another good example why edk2-platforms should *not* exist</span><br style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);" class=""><span style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);" class="">as a separate repository.</span></blockquote><br class=""></div><div class="">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<span style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);" class=""> 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.</span></div><div class=""><br class=""></div><div class=""><blockquote type="cite" class=""><span style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);" class="">(... Please consider adding Cc: tags to the commit messages, so that</span><br style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);" class=""><span style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);" class="">each patch email reaches the subject package owners directly too. Anyway</span><br style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);" class=""><span style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);" class="">that's for the future.)</span></blockquote></div><div class=""><br class=""></div><div class="">Fixed.</div><div class=""><br class=""></div><div class=""><blockquote type="cite" class=""><span style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);" class="">I think the very last hunk in this patch belongs in patch#26 ("MdePkg:</span><br style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);" class=""><span style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);" class="">Introduce assertion on constraint debug mask bit"), which is where BIT6</span><br style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);" class=""><span style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);" class="">is actually defined for PcdDebugPropertyMask.</span></blockquote><br class=""></div><div class=""><blockquote type="cite" class=""><span style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);" class="">(2) The "MdePkg/MdePkg.uni" file update from patch#1 belongs here, IMO.</span><br style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);" class=""></blockquote></div><div class=""><br class=""></div><div class="">Fixed.</div><div class=""><br class=""></div><div class=""><blockquote type="cite" class=""><span style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);" class="">I think the VALID_ARCHITECTURES comment should be dropped altogether.</span><br style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);" class=""><br style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);" class=""><span style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);" class="">At least the current arch list "IA32 X64 EBC" is incorrect; first</span><br style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);" class=""><span style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);" class="">because ARM and AARCH64 are missing (despite the series -- correctly --</span><br style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);" class=""><span style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);" class="">patching Arm*Pkg and such), and second because -- I *think* -- EBC is no</span><br style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);" class=""><span style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);" class="">longer an arch natively supported by edk2. (Mike and Liming will correct</span><br style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);" class=""><span style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);" class="">me on that, if needed.)</span></blockquote><br class=""></div><div class="">Fixed.</div><div class=""><br class=""></div><div class=""><blockquote type="cite" class=""><span style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);" class="">(1) The other two INF files in the same directory should get the same</span><br style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);" class=""><span style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);" class="">update. (I.e., a dependency on DebugCommonLib.)</span></blockquote><br class=""></div><div class="">Nice catch, fixed.</div><div class=""><br class=""></div><div class=""><blockquote type="cite" class=""><span style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);" class="">(2) I believe it should be possible to remove</span><br style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);" class=""><span style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);" class="">"PcdFixedDebugPrintErrorLevel" from all three INF files in the same</span><br style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);" class=""><span style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);" class="">directory. (The only reference, which is being removed, is in</span><br style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);" class=""><span style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);" class="">DebugPrintLevelEnabled().)</span><br class=""></blockquote></div><div class=""><br class=""><div>This issue is common across the patches, fixed everywhere.</div><div><br class=""></div><div><blockquote type="cite" class=""><span style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);" class="">(1) Ugh, sorry, my point (1) was going to be about out two instances of</span><br style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);" class=""><span style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);" class="">the same typo:</span><br style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);" class=""><br style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);" class=""><span style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);" class="">s/constrain/constraint/</span></blockquote><br class=""></div><div>There also were a couple of cases with constrait. Fixed everything.</div><div><br class=""></div><div>Best regards,</div><div>Vitaly</div><div><br class=""></div><div><blockquote type="cite" class=""><div class="">12 мая 2020 г., в 12:50, Laszlo Ersek <<a href="mailto:lersek@redhat.com" class="">lersek@redhat.com</a>> написал(а):</div><br class="Apple-interchange-newline"><div class=""><div class="">On 05/12/20 00:40, Kinney, Michael D wrote:<br class=""><blockquote type="cite" class="">Vitaly,<br class=""><br class="">Thank you for the contribution.<br class=""><br class="">There are a couple points about this approach that need to be discussed.<br class=""><br class="">You have included the <Library/DebugCommonLib.h> from <br class="">MdePkg/Include/Library/DebugLib.h.<br class=""></blockquote><br class="">Right, I've noticed it. I agree it's unusual. I didn't think it was wrong.<br class=""><br class=""><blockquote type="cite" class="">It is very rare for a <br class="">lib class to include another lib class. This means that a module<br class="">that has a dependency on the DebugLib class inherits a hidden<br class="">dependency on the DebugCommonLib class.<br class=""></blockquote><br class="">I agree.<br class=""><br class="">I think it should be fine. Any header H1 should include such further<br class="">headers H2, H3, ... Hn that are required for making the interfaces<br class="">declared in H1 usable in client modules.<br class=""><br class=""><blockquote type="cite" class="">For module INF files,<br class="">we require the INF file to list all the lib classes that the<br class="">module sources directly use.<br class=""></blockquote><br class="">Yes, keyword being "directly".<br class=""><br class=""><blockquote type="cite" class="">Since a module that uses the<br class="">DebugLib uses the ASSERT() and DEBUG() macros, all the APIs<br class="">that the ASSERT() and DEBUG() macros use are also directly<br class="">used by the module.<br class=""></blockquote><br class="">I believe this is where I disagree. The replacement texts of the<br class="">ASSERT() and DEBUG() function-like macros are internals of the<br class="">DebugLib.h lib class header, in my opinion. Those internals place<br class="">requirements on specific DebugLib instances, not on DebugLib class<br class="">consumers.<br class=""><br class="">In other words, when writing a new DebugLib instance, the implementor<br class="">has to ensure that the ASSERT() and DEBUG() macros, as defined in the<br class="">DebugLib class header, will continue working in DebugLib consumer<br class="">modules. The implementor may then choose to make the new DebugLib<br class="">instance dependent on the (singleton) DebugCommonLib instance, for<br class="">example. (This is being done in patches #3, #4, #16, maybe more.) The<br class="">DebugLib consumer module will inherit that dependency, and everything<br class="">will work.<br class=""><br class="">Just because ASSERT() and DEBUG() are function-like macros and not<br class="">actual functions, I don't think the INF file requirements in<br class="">DebugLib-consumer modules should change.<br class=""><br class=""><blockquote type="cite" class="">With this patch series, these macros<br class="">now use the DebugCommonLib class APIs, which means any module<br class="">that uses the DebugLib also directly uses the DebugCommonLib.<br class=""></blockquote><br class="">In my opinion: indirectly.<br class=""><br class="">Thanks,<br class="">Laszlo<br class=""><br class=""></div></div></blockquote></div><br class=""></div></body></html> <div width="1" style="color:white;clear:both">_._,_._,_</div> <hr> Groups.io Links:<p> You receive all messages sent to this group. <p> <a target="_blank" href="https://edk2.groups.io/g/devel/message/59336">View/Reply Online (#59336)</a> | | <a target="_blank" href="https://groups.io/mt/74138532/1813853">Mute This Topic</a> | <a href="https://edk2.groups.io/g/devel/post">New Topic</a><br> <br> <a href="https://edk2.groups.io/g/devel/editsub/1813853">Your Subscription</a> | <a href="mailto:devel+owner@edk2.groups.io">Contact Group Owner</a> | <a href="https://edk2.groups.io/g/devel/unsub">Unsubscribe</a> [edk2-devel-archive@redhat.com]<br> <div width="1" style="color:white;clear:both">_._,_._,_</div>