<html xmlns:v="urn:schemas-microsoft-com:vml" xmlns:o="urn:schemas-microsoft-com:office:office" xmlns:w="urn:schemas-microsoft-com:office:word" xmlns:m="http://schemas.microsoft.com/office/2004/12/omml" xmlns="http://www.w3.org/TR/REC-html40"> <head> <meta http-equiv="Content-Type" content="text/html; charset=utf-8"> <meta name="ProgId" content="Word.Document"> <meta name="Generator" content="Microsoft Word 15"> <meta name="Originator" content="Microsoft Word 15"> <link rel="File-List" href="cid:filelist.xml@01D5FD2C.DF8A2A60"><link rel="Edit-Time-Data" href="cid:editdata.mso"><style></style> </head> <body lang="EN-US" link="blue" vlink="purple" style="tab-interval:.5in"> <div class="WordSection1"> <p class="MsoNormal"><span style="mso-ascii-font-family:Calibri;mso-hansi-font-family:Calibri;mso-bidi-font-family:"Times New Roman"">Vitaly,<o:p></o:p></span></p> <p class="MsoNormal"><span style="mso-ascii-font-family:Calibri;mso-hansi-font-family:Calibri;mso-bidi-font-family:"Times New Roman""><o:p> </o:p></span></p> <p class="MsoNormal"><span style="mso-ascii-font-family:Calibri;mso-hansi-font-family:Calibri;mso-bidi-font-family:"Times New Roman"">It has to do with where PCDs are declared in INF files.<o:p></o:p></span></p> <p class="MsoNormal"><span style="mso-ascii-font-family:Calibri;mso-hansi-font-family:Calibri;mso-bidi-font-family:"Times New Roman""><o:p> </o:p></span></p> <p class="MsoNormal"><span style="mso-ascii-font-family:Calibri;mso-hansi-font-family:Calibri;mso-bidi-font-family:"Times New Roman"">If you access a PCD from a macro like you have added to a library class, the module using that library class does not know there is a macro that uses a PCD.<span style="mso-spacerun:yes"> </span>So the PCD declaration in the Module INF is missing.<span style="mso-spacerun:yes"> </span>By only using the PCDs from the library implementation, the library implementation INF declares the PCDs it uses and the module inherits the PCDs from the library instances.<span style="mso-spacerun:yes"> </span>We do not have a feature that allows a library class (which only has a .h file and a one line declaration in a DEC file) to provide extra information such as PCDs that the library class uses.<span style="mso-spacerun:yes"> </span>We would need a significant extension to the DEC file format and build tools for a library class declaration to provide more information.<o:p></o:p></span></p> <p class="MsoNormal"><span style="mso-ascii-font-family:Calibri;mso-hansi-font-family:Calibri;mso-bidi-font-family:"Times New Roman""><o:p> </o:p></span></p> <p class="MsoNormal"><span style="mso-ascii-font-family:Calibri;mso-hansi-font-family:Calibri;mso-bidi-font-family:"Times New Roman"">Mike<o:p></o:p></span></p> <p class="MsoNormal"><span style="mso-ascii-font-family:Calibri;mso-hansi-font-family:Calibri;mso-bidi-font-family:"Times New Roman""><o:p> </o:p></span></p> <div style="border:none;border-left:solid blue 1.5pt;padding:0in 0in 0in 4.0pt"> <div> <div style="border:none;border-top:solid #E1E1E1 1.0pt;padding:3.0pt 0in 0in 0in"> <p class="MsoNormal"><b><span style="mso-fareast-font-family:"Times New Roman"">From:</span></b><span style="mso-fareast-font-family:"Times New Roman""> Vitaly Cheptsov <cheptsov@ispras.ru> <br> <b>Sent:</b> Wednesday, March 18, 2020 1:43 PM<br> <b>To:</b> Kinney, Michael D <michael.d.kinney@intel.com><br> <b>Cc:</b> devel@edk2.groups.io; Laszlo Ersek <lersek@redhat.com>; Andrew Fish <afish@apple.com>; Marvin Häuser <mhaeuser@outlook.de>; Gao, Liming <liming.gao@intel.com>; Gao, Zhichao <zhichao.gao@intel.com><br> <b>Subject:</b> Re: [edk2-devel] Disabling safe string constraint assertions<o:p></o:p></span></p> </div> </div> <p class="MsoNormal"><o:p> </o:p></p> <p class="MsoNormal"><span style="mso-fareast-font-family:"Times New Roman"">Mike,<o:p></o:p></span></p> <div> <p class="MsoNormal"><span style="mso-fareast-font-family:"Times New Roman""><o:p> </o:p></span></p> </div> <div> <p class="MsoNormal"><span style="mso-fareast-font-family:"Times New Roman"">Thanks for the clarification. I failed to find it in the specs, but the code of the BaseTools kind of gave me such a suspect.<o:p></o:p></span></p> </div> <div> <p class="MsoNormal"><span style="mso-fareast-font-family:"Times New Roman"">Is there any particular reason why this limitation was added? At the moment I do not see a good reason why this is done.<o:p></o:p></span></p> </div> <div> <p class="MsoNormal"><span style="mso-fareast-font-family:"Times New Roman""><o:p> </o:p></span></p> </div> <div> <p class="MsoNormal"><span style="mso-fareast-font-family:"Times New Roman"">If there is one, I guess we could consider some other approach, for example, we can factor out these functions to a separate DebugHelperLib/DebugBaseLib/DebugCommonLib, which every DebugLib will depend on. This will make sense to me as a workaround of such limitation, as neither us, nor Andrew, as he mentioned previously, are happy of having to duplicate code in DebugLib implementations and update them for a minor Pcd change.<o:p></o:p></span></p> </div> <div> <p class="MsoNormal"><span style="mso-fareast-font-family:"Times New Roman""><o:p> </o:p></span></p> </div> <div> <p class="MsoNormal"><span style="mso-fareast-font-family:"Times New Roman"">If there is no good reason, to be honest, it feels like we should just fix this. After reading the spec I do not see what kind of compiler issue could arise here with normal PCDs.<o:p></o:p></span></p> </div> <div> <p class="MsoNormal"><span style="mso-fareast-font-family:"Times New Roman""><o:p> </o:p></span></p> </div> <div> <p class="MsoNormal"><span style="mso-fareast-font-family:"Times New Roman"">Best regards,<o:p></o:p></span></p> </div> <div> <p class="MsoNormal"><span style="mso-fareast-font-family:"Times New Roman"">Vitaly<o:p></o:p></span></p> <div> <p class="MsoNormal"><span style="mso-fareast-font-family:"Times New Roman""><br style="mso-special-character:line-break"> <![if !supportLineBreakNewLine]><br style="mso-special-character:line-break"> <![endif]><o:p></o:p></span></p> <blockquote style="margin-top:5.0pt;margin-bottom:5.0pt"> <div> <p class="MsoNormal"><span style="mso-fareast-font-family:"Times New Roman"">18 марта 2020 г., в 23:35, Kinney, Michael D <<a href="mailto:michael.d.kinney@intel.com">michael.d.kinney@intel.com</a>> написал(а):<o:p></o:p></span></p> </div> <p class="MsoNormal"><span style="mso-fareast-font-family:"Times New Roman""><o:p> </o:p></span></p> <div> <div> <p class="MsoNormal"><span style="mso-fareast-font-family:"Times New Roman"">Vitaly,<o:p></o:p></span></p> </div> <div> <p class="MsoNormal"><span style="mso-fareast-font-family:"Times New Roman""> <o:p></o:p></span></p> </div> <div> <p class="MsoNormal"><span style="mso-fareast-font-family:"Times New Roman"">The break you are seeing is because you are not using functions to eval the PCD. <span class="apple-converted-space"> </span>This is a known restriction in how PCDs work between libs and modules and is why the current design uses the<span class="apple-converted-space"> </span><span class="spelle">XxxEnabled</span>() functions.<o:p></o:p></span></p> </div> <div> <p class="MsoNormal"><span style="mso-fareast-font-family:"Times New Roman""> <o:p></o:p></span></p> </div> <div> <p class="MsoNormal"><span style="mso-fareast-font-family:"Times New Roman"">I have not reviewed this issue in a very long time, so I do not know if there are any attributes of newer compilers that would allow a different design now.<o:p></o:p></span></p> </div> <div> <p class="MsoNormal"><span style="mso-fareast-font-family:"Times New Roman""> <o:p></o:p></span></p> </div> <div> <p class="MsoNormal"><span style="mso-fareast-font-family:"Times New Roman"">Best regards,<o:p></o:p></span></p> </div> <div> <p class="MsoNormal"><span style="mso-fareast-font-family:"Times New Roman""> <o:p></o:p></span></p> </div> <div> <p class="MsoNormal"><span style="mso-fareast-font-family:"Times New Roman"">Mike<o:p></o:p></span></p> </div> <div> <p class="MsoNormal"><span style="mso-fareast-font-family:"Times New Roman""> <o:p></o:p></span></p> </div> <div> <div style="border:none;border-top:solid #E1E1E1 1.0pt;padding:3.0pt 0in 0in 0in"> <div> <p class="MsoNormal" style="mso-outline-level:1"><b><span style="mso-fareast-font-family:"Times New Roman"">From:</span></b><span class="apple-converted-space"><span style="mso-fareast-font-family:"Times New Roman""> </span></span><span style="mso-fareast-font-family:"Times New Roman""><a href="mailto:devel@edk2.groups.io"><span style="color:purple">devel@edk2.groups.io</span></a><span class="apple-converted-space"> </span><<a href="mailto:devel@edk2.groups.io"><span style="color:purple">devel@edk2.groups.io</span></a>><span class="apple-converted-space"> </span><b>On Behalf Of<span class="apple-converted-space"> </span></b>Vitaly Cheptsov<br> <b>Sent:</b><span class="apple-converted-space"> </span>Wednesday, March 18, 2020 12:36 PM<br> <b>To:</b><span class="apple-converted-space"> </span>Laszlo Ersek <<a href="mailto:lersek@redhat.com"><span style="color:purple">lersek@redhat.com</span></a>>; Andrew Fish <<a href="mailto:afish@apple.com"><span style="color:purple">afish@apple.com</span></a>>; Kinney, Michael D <<a href="mailto:michael.d.kinney@intel.com"><span style="color:purple">michael.d.kinney@intel.com</span></a>>; Marvin Häuser <<a href="mailto:mhaeuser@outlook.de"><span style="color:purple">mhaeuser@outlook.de</span></a>>; Gao, Liming <<a href="mailto:liming.gao@intel.com"><span style="color:purple">liming.gao@intel.com</span></a>>; Gao, Zhichao <<a href="mailto:zhichao.gao@intel.com"><span style="color:purple">zhichao.gao@intel.com</span></a>><br> <b>Cc:</b><span class="apple-converted-space"> </span><a href="mailto:devel@edk2.groups.io"><span style="color:purple">devel@edk2.groups.io</span></a><br> <b>Subject:</b><span class="apple-converted-space"> </span>Re: [edk2-devel] Disabling safe string constraint assertions<o:p></o:p></span></p> </div> </div> </div> <div> <p class="MsoNormal"><span style="mso-fareast-font-family:"Times New Roman""> <o:p></o:p></span></p> </div> <div> <div> <p class="MsoNormal"><span style="mso-fareast-font-family:"Times New Roman"">Hello!<o:p></o:p></span></p> </div> </div> <div> <div> <p class="MsoNormal"><span style="mso-fareast-font-family:"Times New Roman""> <o:p></o:p></span></p> </div> </div> <div> <div> <p class="MsoNormal"><span style="mso-fareast-font-family:"Times New Roman"">I have a prototype of the patch, but there currently is an issue with the current EDK II build system.<o:p></o:p></span></p> </div> </div> <div> <div> <p class="MsoNormal"><span style="mso-fareast-font-family:"Times New Roman"">I attached the patch to this e-mail, however, it will not compile for reasonably obscure causes.<o:p></o:p></span></p> </div> </div> <div> <div> <p class="MsoNormal"><span style="mso-fareast-font-family:"Times New Roman""> <o:p></o:p></span></p> </div> </div> <div> <div> <p class="MsoNormal"><span style="mso-fareast-font-family:"Times New Roman"">From what I understand:<o:p></o:p></span></p> </div> </div> <div> <div> <p class="MsoNormal"><span style="mso-fareast-font-family:"Times New Roman"">- DebugLib header now directly uses PCDs from DebugLib, like PcdDebugPropertyMask.<o:p></o:p></span></p> </div> </div> <div> <div> <p class="MsoNormal"><span style="mso-fareast-font-family:"Times New Roman"">- Any library implementing DebugLib should now depend on these PCDs, which seems fairly natural (and I fixed that in BaseDebugLibNull).<o:p></o:p></span></p> </div> </div> <div> <div> <p class="MsoNormal"><span style="mso-fareast-font-family:"Times New Roman"">- Any library using DebugLib header should depend on DebugLib, which also depend on DebugLib to get its PCDs (that already looks fine).<o:p></o:p></span></p> </div> </div> <div> <div> <p class="MsoNormal"><span style="mso-fareast-font-family:"Times New Roman""> <o:p></o:p></span></p> </div> </div> <div> <div> <p class="MsoNormal"><span style="mso-fareast-font-family:"Times New Roman"">However, for some reason DebugLib PCDs are not included in Autogen.h header for other libraries some reason, and we get errors like:<o:p></o:p></span></p> </div> </div> <div> <div> <p class="MsoNormal"><span style="mso-fareast-font-family:"Times New Roman"">MdePkg/Library/BaseOrderedCollectionRedBlackTreeLib/BaseOrderedCollectionRedBlackTreeLib.c:1151:9: error: use of undeclared identifier '_PCD_GET_MODE_8_PcdDebugPropertyMask'<o:p></o:p></span></p> </div> </div> <div> <div> <p class="MsoNormal"><span style="mso-fareast-font-family:"Times New Roman""> <o:p></o:p></span></p> </div> </div> <div> <div> <p class="MsoNormal"><span style="mso-fareast-font-family:"Times New Roman"">I am not familiar with the build system well enough to resolve this, so I either need guidance on where to look first or it will be great if somebody else handles that.<o:p></o:p></span></p> </div> </div> <div> <div> <p class="MsoNormal"><span style="mso-fareast-font-family:"Times New Roman"">I do not believe it is a great idea to abandon the idea of dropping DebugAssertEnabled-like functions, so I suggest us to focus on resolving the build system limitation rather than trying a new approach.<o:p></o:p></span></p> </div> </div> <div> <div> <p class="MsoNormal"><span style="mso-fareast-font-family:"Times New Roman""> <o:p></o:p></span></p> </div> </div> <div> <div> <p class="MsoNormal"><span style="mso-fareast-font-family:"Times New Roman"">Best regards,<o:p></o:p></span></p> </div> </div> <div> <div> <p class="MsoNormal"><span style="mso-fareast-font-family:"Times New Roman"">Vitaly<o:p></o:p></span></p> </div> </div> <div> <div> <p class="MsoNormal"><span style="mso-fareast-font-family:"Times New Roman""> <o:p></o:p></span></p> </div> </div> <div> <div> <p class="MsoNormal"><span style="mso-fareast-font-family:"Times New Roman""> <o:p></o:p></span></p> </div> </div> <div> <p class="MsoNormal"><span style="mso-fareast-font-family:"Times New Roman""> <o:p></o:p></span></p> </div> <div> <div> <p class="MsoNormal"><span style="mso-fareast-font-family:"Times New Roman""><br> <br style="mso-special-character:line-break"> <![if !supportLineBreakNewLine]><br style="mso-special-character:line-break"> <![endif]><o:p></o:p></span></p> </div> <blockquote style="margin-top:5.0pt;margin-bottom:5.0pt"> <div> <div> <p class="MsoNormal"><span style="mso-fareast-font-family:"Times New Roman"">11 Ð¼Ð°Ñ€Ñ‚Ð° 2020 Ð³., Ð² 16:14, Laszlo Ersek <<a href="mailto:lersek@redhat.com"><span style="color:purple">lersek@redhat.com</span></a>> Ð½Ð°Ð¿Ð¸ÑÐ°Ð»(Ð°):<o:p></o:p></span></p> </div> </div> <div> <p class="MsoNormal"><span style="mso-fareast-font-family:"Times New Roman""> <o:p></o:p></span></p> </div> <div> <div> <p class="MsoNormal"><span style="font-size:9.0pt;font-family:"Helvetica",sans-serif;mso-fareast-font-family:"Times New Roman"">On 03/11/20 14:09, Vitaly Cheptsov wrote:<br> <br style="mso-special-character:line-break"> <![if !supportLineBreakNewLine]><br style="mso-special-character:line-break"> <![endif]></span><span style="mso-fareast-font-family:"Times New Roman""><o:p></o:p></span></p> </div> <blockquote style="margin-top:5.0pt;margin-bottom:5.0pt"> <div> <p class="MsoNormal"><span style="font-size:9.0pt;font-family:"Helvetica",sans-serif;mso-fareast-font-family:"Times New Roman"">Hi everyone,<br> <br> So, I believe that by now we mostly agreed to let the original<br> proposition land as a short-term solution. We end up with:<br> <br> 1. A PCD condition within SAFE_STRING_COSTRAINT_CHECK macro.<br> 2. Make this condition evaluate to TRUE by default (i.e. ASSERT).<br> 3. Update documentation for BaseLib functions to include the information<br> about this behaviour.<br> <br> The only thing in question is whether this should be a separate PCD or<br> an extra bit in PcdDebugPropertyMask. I believe that we almost agreed on<br> two things:<br> <br> 1. Adding an extra bit to PcdDebugPropertyMask is cleaner.<br> 2. Extending DebugLib interface with a new function is not a good idea.<br> <br> Therefore I suggest:<br> <br> 1.Add #define DEBUG_PROPERTY_ASSERT_CONSTRAINT_ENABLED 0x40.<br> 2. Create header-only macros to replace functions like<br> DebugAssertEnabled. We can then use these macros in new code and<br> deprecate the original functions.<br> 3. Enable DEBUG_PROPERTY_ASSERT_CONSTRAINT_ENABLED bit in MdePkg by default.<br> <br> I will submit the new version of the patch soon unless there is an<br> immediate opposing opinion.</span><span style="mso-fareast-font-family:"Times New Roman""><o:p></o:p></span></p> </div> </blockquote> <div> <p class="MsoNormal"><span style="font-size:9.0pt;font-family:"Helvetica",sans-serif;mso-fareast-font-family:"Times New Roman""><br> Not sure about any particular deprecation timeline, but to me the above<br> certainly sounds worth submitting for review.<br> <br> (NB I don't plan to review in detail -- I just meant to comment on the<br> design, since I was asked to.)<br> <br> Thanks!<br> Laszlo</span><span style="mso-fareast-font-family:"Times New Roman""><o:p></o:p></span></p> </div> </div> </blockquote> </div> <div> <p class="MsoNormal"><span style="mso-fareast-font-family:"Times New Roman""> <o:p></o:p></span></p> </div> <div> <div> <p class="MsoNormal"><span style="mso-fareast-font-family:"Times New Roman""><o:p></o:p></span></p> </div> </div> </div> </blockquote> </div> <p class="MsoNormal"><span style="mso-fareast-font-family:"Times New Roman""><o:p> </o:p></span></p> </div> </div> </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/55965">View/Reply Online (#55965)</a> | | <a target="_blank" href="https://groups.io/mt/71711587/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>