<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Mon, Dec 25, 2017 at 12:43 PM, whitequark <span dir="ltr"><<a href="mailto:whitequark@whitequark.org" target="_blank">whitequark@whitequark.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">On 2017-12-25 20:32, Don Hinton wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
While beauty is in the eye of the beholder, I'm not sure introducing a<br>
new template function in a header that's only used in that header is a<br>
good idea.  Every file that includes DwarfParser.hpp is going to get<br>
that template function -- and someone may try to use it someday.<br>
</blockquote>
<br></span>
That header is a header private to the libunwind implementation,<br>
and the template function is confined to `namespace libunwind`.</blockquote><div><br></div><div>Smaller universe, but still the same issue.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> </blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class=""><br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
However, if you did want to do that, you should handle signed types as<br>
well, which pint_max_value doesn't do.<br>
</blockquote>
<br></span>
`pint_t` is an unsigned type in libunwind, the signed counterpart is `sint_t`</blockquote><div><br></div><div>The point is that a template can take any type.  Once you introduce it, it can be used by anyone with any type.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">.<span class=""><br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
It should also assert for non-integral types.<br>
</blockquote>
<br></span>
True, but I do not know enough C++ magic to implement that. AIUI<br>
std::numeric_limits implements this by specializing a template for every<br>
integral type, which is not something I will do here. Of course<br>
there's a standard template for checking whether a type is integral,<br>
but that also lives in libcxx.<br></blockquote><div><br></div><div>Why not just use static_cast<pint_t>(~0) and avoid all these issues?</div><div><br></div><div>You might have a point that this doesn't apply in your case, but it's a good habit to get into.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div class="h5">
<br>
On Mon, Dec 25, 2017 at 12:06 PM, whitequark<br>
<<a href="mailto:whitequark@whitequark.org" target="_blank">whitequark@whitequark.org</a>> wrote:<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
On 2017-12-25 19:43, Don Hinton wrote:<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Here's the patch I applied locally.<br>
<br>
hth...<br>
don<br>
[snip]<br>
</blockquote>
<br>
I've committed a slightly beautified version of the patch (below)<br>
as r321446. Cheers!<br>
<br>
>From 8a4760bafc1123f09438587ee5432e<wbr>abdec3d33d Mon Sep 17 00:00:00<br>
2001<br>
From: whitequark <<a href="mailto:whitequark@whitequark.org" target="_blank">whitequark@whitequark.org</a>><br>
Date: Mon, 25 Dec 2017 20:03:40 +0000<br>
Subject: [PATCH] [libunwind] Unbreak debug builds after r321440.<br>
<br>
---<br>
src/DwarfParser.hpp | 11 +++++++----<br>
1 file changed, 7 insertions(+), 4 deletions(-)<br>
<br>
diff --git a/src/DwarfParser.hpp b/src/DwarfParser.hpp<br>
index 518101e..645ac21 100644<br>
--- a/src/DwarfParser.hpp<br>
+++ b/src/DwarfParser.hpp<br>
@@ -17,7 +17,6 @@<br>
#include <stdint.h><br>
#include <stdio.h><br>
#include <stdlib.h><br>
-#include <limits.h><br>
<br>
#include "libunwind.h"<br>
#include "dwarf2.h"<br>
@@ -26,6 +25,10 @@<br>
<br>
namespace libunwind {<br>
<br>
+// Avoid relying on C++ headers.<br>
+template <class T><br>
+static constexpr T pint_max_value() { return ~0; }<br>
+<br>
/// CFI_Parser does basic parsing of a CFI (Call Frame Information)<br>
records.<br>
/// See DWARF Spec for details:<br>
///<br>
<br>
</blockquote>
<a href="http://refspecs.linuxbase.org/LSB_3.1.0/LSB-Core-generic/LSB-Core-generic/ehframechpt.html" rel="noreferrer" target="_blank">http://refspecs.linuxbase.org/<wbr>LSB_3.1.0/LSB-Core-generic/LSB<wbr>-Core-generic/ehframechpt.html</a><br>
</div></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
[1]<div><div class="h5"><br>
@@ -540,7 +543,7 @@ bool CFI_Parser<A>::parseInstructio<wbr>ns(A<br>
&addressSpace, pint_t instructions,<br>
results->cfaRegister = 0;<br>
results->cfaExpression = (int64_t)p;<br>
length = addressSpace.getULEB128(p, instructionsEnd);<br>
-      assert(length < std::numeric_limits<pint_t>::m<wbr>ax() &&<br>
"pointer overflow");<br>
+      assert(length < pint_max_value<pint_t>() && "pointer<br>
overflow");<br>
p += static_cast<pint_t>(length);<br>
<br>
_LIBUNWIND_TRACE_DWARF("DW_CFA<wbr>_def_cfa_expression(expression<wbr>=0x%"<br>
PRIx64<br>
", length=%" PRIu64 ")\n",<br>
@@ -556,7 +559,7 @@ bool CFI_Parser<A>::parseInstructio<wbr>ns(A<br>
&addressSpace, pint_t instructions,<br>
results->savedRegisters[reg].l<wbr>ocation =<br>
kRegisterAtExpression;<br>
results->savedRegisters[reg].v<wbr>alue = (int64_t)p;<br>
length = addressSpace.getULEB128(p, instructionsEnd);<br>
-      assert(length < std::numeric_limits<pint_t>::m<wbr>ax() &&<br>
"pointer overflow");<br>
+      assert(length < pint_max_value<pint_t>() && "pointer<br>
overflow");<br>
p += static_cast<pint_t>(length);<br>
_LIBUNWIND_TRACE_DWARF("DW_CFA<wbr>_expression(reg=%" PRIu64 ", "<br>
"expression=0x%" PRIx64 ", "<br>
@@ -642,7 +645,7 @@ bool CFI_Parser<A>::parseInstructio<wbr>ns(A<br>
&addressSpace, pint_t instructions,<br>
results->savedRegisters[reg].l<wbr>ocation =<br>
kRegisterIsExpression;<br>
results->savedRegisters[reg].v<wbr>alue = (int64_t)p;<br>
length = addressSpace.getULEB128(p, instructionsEnd);<br>
-      assert(length < std::numeric_limits<pint_t>::m<wbr>ax() &&<br>
"pointer overflow");<br>
+      assert(length < pint_max_value<pint_t>() && "pointer<br>
overflow");<br>
p += static_cast<pint_t>(length);<br>
_LIBUNWIND_TRACE_DWARF("DW_CFA<wbr>_val_expression(reg=%" PRIu64<br>
", "<br>
"expression=0x%" PRIx64 ", length=%"<br>
PRIu64 ")\n",<br>
--<br>
2.11.0<br>
<br>
On Mon, Dec 25, 2017 at 11:26 AM, Don Hinton <<a href="mailto:hintonda@gmail.com" target="_blank">hintonda@gmail.com</a>><br>
wrote:<br>
<br>
On Mon, Dec 25, 2017 at 11:09 AM, whitequark<br>
<<a href="mailto:whitequark@whitequark.org" target="_blank">whitequark@whitequark.org</a>> wrote:<br>
On 2017-12-25 19:04, Don Hinton wrote:<br>
Hi:<br>
<br>
This change breaks in a local debug build, e.g.,:<br>
<br>
<br>
</div></div></blockquote>
/Users/dhinton/projects/llvm_p<wbr>roject/libunwind/src/DwarfPars<wbr>er.hpp:559:28:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div class="h5">
error: no member named 'numeric_limits' in namespace 'std'<br>
assert(length < std::numeric_limits<pint_t>::m<wbr>ax() && "pointer<br>
overflow");<br>
~~~~~^<br>
<br>
Sorry, I missed this. Any idea on reformulating the assert in a way<br>
that does not require libcxx headers? Not having them significantly<br>
simplifies bare-metal builds...<br>
<br>
Well, assuming pint_t is some unsigned integer type, the max can be<br>
found like this:<br>
<br>
pint_t max_pint_t = ~0;<br>
<br>
So, that could be used in a pinch.<br>
<br>
thanks...<br>
don<br>
<br>
On Mon, Dec 25, 2017 at 5:06 AM, whitequark via cfe-commits<br>
<<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a>> wrote:<br>
<br>
Author: whitequark<br>
Date: Mon Dec 25 05:06:09 2017<br>
New Revision: 321440<br>
<br></div></div>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=321440&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-pr<wbr>oject?rev=321440&view=rev</a> [2] [1]<span class=""><br>
[1]<br>
Log:<br>
[libunwind] Avoid using C++ headers.<br>
<br>
This is useful for building libunwind on libcxx-free systems.<br>
<br>
Modified:<br>
libunwind/trunk/src/DwarfParse<wbr>r.hpp<br>
<br>
Modified: libunwind/trunk/src/DwarfParse<wbr>r.hpp<br>
URL:<br>
<br>
<br>
</span></blockquote>
<a href="http://llvm.org/viewvc/llvm-project/libunwind/trunk/src/DwarfParser.hpp?rev=321440&r1=321439&r2=321440&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-pr<wbr>oject/libunwind/trunk/src/Dwar<wbr>fParser.hpp?rev=321440&r1=3214<wbr>39&r2=321440&view=diff</a><br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
[3]<br>
[2]<br>
[2]<br>
<br>
<br>
</blockquote>
==============================<wbr>==============================<wbr>==================<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">
<br>
--- libunwind/trunk/src/DwarfParse<wbr>r.hpp (original)<br>
+++ libunwind/trunk/src/DwarfParse<wbr>r.hpp Mon Dec 25 05:06:09 2017<br>
@@ -17,7 +17,7 @@<br>
#include <stdint.h><br>
#include <stdio.h><br>
#include <stdlib.h><br>
-#include <limits><br>
+#include <limits.h><br>
<br>
#include "libunwind.h"<br>
#include "dwarf2.h"<br>
<br>
______________________________<wbr>_________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a><br>
</span><a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/<wbr>mailman/listinfo/cfe-commits</a> [4] [3]<br>
[3]<br>
<br>
Links:<br>
------<br>
[1] <a href="http://llvm.org/viewvc/llvm-project?rev=321440&amp;view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-pr<wbr>oject?rev=321440&amp;view=rev</a> [5]<br>
[4]<br>
[2]<br>
<br>
<br>
</blockquote><span class="">
<a href="http://llvm.org/viewvc/llvm-project/libunwind/trunk/src/DwarfParser.hpp?rev=321440&amp;r1=321439&amp;r2=321440&amp;view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-pr<wbr>oject/libunwind/trunk/src/Dwar<wbr>fParser.hpp?rev=321440&amp;r1=<wbr>321439&amp;r2=321440&amp;view=<wbr>diff</a><br>
</span><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
[6]<br>
[5]<br>
[3] <a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/<wbr>mailman/listinfo/cfe-commits</a> [4]<span class=""><br>
[3]<br>
<br>
--<br>
whitequark<br>
<br>
Links:<br>
------<br></span>
[1] <a href="http://llvm.org/viewvc/llvm-project?rev=321440&amp;view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-pr<wbr>oject?rev=321440&amp;view=rev</a> [5]<br>
[2]<br>
<br>
</blockquote><span class="">
<a href="http://llvm.org/viewvc/llvm-project/libunwind/trunk/src/DwarfParser.hpp?rev=321440&amp;r1=321439&amp;r2=321440&amp;view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-pr<wbr>oject/libunwind/trunk/src/Dwar<wbr>fParser.hpp?rev=321440&amp;r1=<wbr>321439&amp;r2=321440&amp;view=<wbr>diff</a><br>
</span><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
[6]<br>
[3] <a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/<wbr>mailman/listinfo/cfe-commits</a> [4]<span class=""><br>
[4] <a href="http://llvm.org/viewvc/llvm-project?rev=321440&amp;amp;view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-pr<wbr>oject?rev=321440&amp;amp;view=<wbr>rev</a><br></span>
[7]<br>
[5]<br>
<br>
</blockquote><span class="">
<a href="http://llvm.org/viewvc/llvm-project/libunwind/trunk/src/DwarfParser.hpp?rev=321440&amp;amp;r1=321439&amp;amp;r2=321440&amp;amp;view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-pr<wbr>oject/libunwind/trunk/src/Dwar<wbr>fParser.hpp?rev=321440&amp;<wbr>amp;r1=321439&amp;amp;r2=32144<wbr>0&amp;amp;view=diff</a><br>
</span><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
[8]<br>
</blockquote><span class="">
<br>
--<br>
whitequark<br>
<br>
<br>
<br>
Links:<br>
------<br>
[1]<br>
</span><a href="http://refspecs.linuxbase.org/LSB_3.1.0/LSB-Core-generic/LSB-Core-generic/ehframechpt.html" rel="noreferrer" target="_blank">http://refspecs.linuxbase.org/<wbr>LSB_3.1.0/LSB-Core-generic/LSB<wbr>-Core-generic/ehframechpt.html</a><br>
[2] <a href="http://llvm.org/viewvc/llvm-project?rev=321440&amp;view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-pr<wbr>oject?rev=321440&amp;view=rev</a><br>
[3]<span class=""><br>
<a href="http://llvm.org/viewvc/llvm-project/libunwind/trunk/src/DwarfParser.hpp?rev=321440&amp;r1=321439&amp;r2=321440&amp;view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-pr<wbr>oject/libunwind/trunk/src/Dwar<wbr>fParser.hpp?rev=321440&amp;r1=<wbr>321439&amp;r2=321440&amp;view=<wbr>diff</a><br></span>
[4] <a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/<wbr>mailman/listinfo/cfe-commits</a><br>
[5] <a href="http://llvm.org/viewvc/llvm-project?rev=321440&amp;amp;view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-pr<wbr>oject?rev=321440&amp;amp;view=<wbr>rev</a><br>
[6]<span class=""><br>
<a href="http://llvm.org/viewvc/llvm-project/libunwind/trunk/src/DwarfParser.hpp?rev=321440&amp;amp;r1=321439&amp;amp;r2=321440&amp;amp;view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-pr<wbr>oject/libunwind/trunk/src/Dwar<wbr>fParser.hpp?rev=321440&amp;<wbr>amp;r1=321439&amp;amp;r2=32144<wbr>0&amp;amp;view=diff</a><br></span>
[7] <a href="http://llvm.org/viewvc/llvm-project?rev=321440&amp;amp;amp;view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-pr<wbr>oject?rev=321440&amp;amp;amp;<wbr>view=rev</a><br>
[8]<br>
<a href="http://llvm.org/viewvc/llvm-project/libunwind/trunk/src/DwarfParser.hpp?rev=321440&amp;amp;amp;r1=321439&amp;amp;amp;r2=321440&amp;amp;amp;view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-pr<wbr>oject/libunwind/trunk/src/Dwar<wbr>fParser.hpp?rev=321440&amp;<wbr>amp;amp;r1=321439&amp;amp;amp;<wbr>r2=321440&amp;amp;amp;view=<wbr>diff</a><span class="HOEnZb"><font color="#888888"><br>
</font></span></blockquote><span class="HOEnZb"><font color="#888888">
<br>
-- <br>
whitequark<br>
</font></span></blockquote></div><br></div></div>