[libunwind] r321440 - [libunwind] Avoid using C++ headers.

Don Hinton via cfe-commits cfe-commits at lists.llvm.org
Mon Dec 25 12:54:24 PST 2017 

On Mon, Dec 25, 2017 at 12:43 PM, whitequark <whitequark at whitequark.org>
wrote:

> On 2017-12-25 20:32, Don Hinton wrote:
>
>> While beauty is in the eye of the beholder, I'm not sure introducing a
>> new template function in a header that's only used in that header is a
>> good idea.  Every file that includes DwarfParser.hpp is going to get
>> that template function -- and someone may try to use it someday.
>>
>
> That header is a header private to the libunwind implementation,
> and the template function is confined to `namespace libunwind`.


Smaller universe, but still the same issue.


>


>
> However, if you did want to do that, you should handle signed types as
>> well, which pint_max_value doesn't do.
>>
>
> `pint_t` is an unsigned type in libunwind, the signed counterpart is
> `sint_t`


The point is that a template can take any type.  Once you introduce it, it
can be used by anyone with any type.


> .
>
> It should also assert for non-integral types.
>>
>
> True, but I do not know enough C++ magic to implement that. AIUI
> std::numeric_limits implements this by specializing a template for every
> integral type, which is not something I will do here. Of course
> there's a standard template for checking whether a type is integral,
> but that also lives in libcxx.
>

Why not just use static_cast<pint_t>(~0) and avoid all these issues?

You might have a point that this doesn't apply in your case, but it's a
good habit to get into.


>
>
>> On Mon, Dec 25, 2017 at 12:06 PM, whitequark
>> <whitequark at whitequark.org> wrote:
>>
>> On 2017-12-25 19:43, Don Hinton wrote:
>>>
>>> Here's the patch I applied locally.
>>>>
>>>> hth...
>>>> don
>>>> [snip]
>>>>
>>>
>>> I've committed a slightly beautified version of the patch (below)
>>> as r321446. Cheers!
>>>
>>> From 8a4760bafc1123f09438587ee5432eabdec3d33d Mon Sep 17 00:00:00
>>> 2001
>>> From: whitequark <whitequark at whitequark.org>
>>> Date: Mon, 25 Dec 2017 20:03:40 +0000
>>> Subject: [PATCH] [libunwind] Unbreak debug builds after r321440.
>>>
>>> ---
>>> src/DwarfParser.hpp | 11 +++++++----
>>> 1 file changed, 7 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/src/DwarfParser.hpp b/src/DwarfParser.hpp
>>> index 518101e..645ac21 100644
>>> --- a/src/DwarfParser.hpp
>>> +++ b/src/DwarfParser.hpp
>>> @@ -17,7 +17,6 @@
>>> #include <stdint.h>
>>> #include <stdio.h>
>>> #include <stdlib.h>
>>> -#include <limits.h>
>>>
>>> #include "libunwind.h"
>>> #include "dwarf2.h"
>>> @@ -26,6 +25,10 @@
>>>
>>> namespace libunwind {
>>>
>>> +// Avoid relying on C++ headers.
>>> +template <class T>
>>> +static constexpr T pint_max_value() { return ~0; }
>>> +
>>> /// CFI_Parser does basic parsing of a CFI (Call Frame Information)
>>> records.
>>> /// See DWARF Spec for details:
>>> ///
>>>
>>> http://refspecs.linuxbase.org/LSB_3.1.0/LSB-Core-generic/LSB
>> -Core-generic/ehframechpt.html
>>
>>> [1]
>>>
>>> @@ -540,7 +543,7 @@ bool CFI_Parser<A>::parseInstructions(A
>>> &addressSpace, pint_t instructions,
>>> results->cfaRegister = 0;
>>> results->cfaExpression = (int64_t)p;
>>> length = addressSpace.getULEB128(p, instructionsEnd);
>>> -      assert(length < std::numeric_limits<pint_t>::max() &&
>>> "pointer overflow");
>>> +      assert(length < pint_max_value<pint_t>() && "pointer
>>> overflow");
>>> p += static_cast<pint_t>(length);
>>>
>>> _LIBUNWIND_TRACE_DWARF("DW_CFA_def_cfa_expression(expression=0x%"
>>> PRIx64
>>> ", length=%" PRIu64 ")\n",
>>> @@ -556,7 +559,7 @@ bool CFI_Parser<A>::parseInstructions(A
>>> &addressSpace, pint_t instructions,
>>> results->savedRegisters[reg].location =
>>> kRegisterAtExpression;
>>> results->savedRegisters[reg].value = (int64_t)p;
>>> length = addressSpace.getULEB128(p, instructionsEnd);
>>> -      assert(length < std::numeric_limits<pint_t>::max() &&
>>> "pointer overflow");
>>> +      assert(length < pint_max_value<pint_t>() && "pointer
>>> overflow");
>>> p += static_cast<pint_t>(length);
>>> _LIBUNWIND_TRACE_DWARF("DW_CFA_expression(reg=%" PRIu64 ", "
>>> "expression=0x%" PRIx64 ", "
>>> @@ -642,7 +645,7 @@ bool CFI_Parser<A>::parseInstructions(A
>>> &addressSpace, pint_t instructions,
>>> results->savedRegisters[reg].location =
>>> kRegisterIsExpression;
>>> results->savedRegisters[reg].value = (int64_t)p;
>>> length = addressSpace.getULEB128(p, instructionsEnd);
>>> -      assert(length < std::numeric_limits<pint_t>::max() &&
>>> "pointer overflow");
>>> +      assert(length < pint_max_value<pint_t>() && "pointer
>>> overflow");
>>> p += static_cast<pint_t>(length);
>>> _LIBUNWIND_TRACE_DWARF("DW_CFA_val_expression(reg=%" PRIu64
>>> ", "
>>> "expression=0x%" PRIx64 ", length=%"
>>> PRIu64 ")\n",
>>> --
>>> 2.11.0
>>>
>>> On Mon, Dec 25, 2017 at 11:26 AM, Don Hinton <hintonda at gmail.com>
>>> wrote:
>>>
>>> On Mon, Dec 25, 2017 at 11:09 AM, whitequark
>>> <whitequark at whitequark.org> wrote:
>>> On 2017-12-25 19:04, Don Hinton wrote:
>>> Hi:
>>>
>>> This change breaks in a local debug build, e.g.,:
>>>
>>>
>>> /Users/dhinton/projects/llvm_project/libunwind/src/DwarfPars
>> er.hpp:559:28:
>>
>>> error: no member named 'numeric_limits' in namespace 'std'
>>> assert(length < std::numeric_limits<pint_t>::max() && "pointer
>>> overflow");
>>> ~~~~~^
>>>
>>> Sorry, I missed this. Any idea on reformulating the assert in a way
>>> that does not require libcxx headers? Not having them significantly
>>> simplifies bare-metal builds...
>>>
>>> Well, assuming pint_t is some unsigned integer type, the max can be
>>> found like this:
>>>
>>> pint_t max_pint_t = ~0;
>>>
>>> So, that could be used in a pinch.
>>>
>>> thanks...
>>> don
>>>
>>> On Mon, Dec 25, 2017 at 5:06 AM, whitequark via cfe-commits
>>> <cfe-commits at lists.llvm.org> wrote:
>>>
>>> Author: whitequark
>>> Date: Mon Dec 25 05:06:09 2017
>>> New Revision: 321440
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=321440&view=rev [2] [1]
>>> [1]
>>> Log:
>>> [libunwind] Avoid using C++ headers.
>>>
>>> This is useful for building libunwind on libcxx-free systems.
>>>
>>> Modified:
>>> libunwind/trunk/src/DwarfParser.hpp
>>>
>>> Modified: libunwind/trunk/src/DwarfParser.hpp
>>> URL:
>>>
>>>
>>> http://llvm.org/viewvc/llvm-project/libunwind/trunk/src/Dwar
>> fParser.hpp?rev=321440&r1=321439&r2=321440&view=diff
>>
>>> [3]
>>> [2]
>>> [2]
>>>
>>>
>>> ============================================================
>> ==================
>>
>>>
>>> --- libunwind/trunk/src/DwarfParser.hpp (original)
>>> +++ libunwind/trunk/src/DwarfParser.hpp Mon Dec 25 05:06:09 2017
>>> @@ -17,7 +17,7 @@
>>> #include <stdint.h>
>>> #include <stdio.h>
>>> #include <stdlib.h>
>>> -#include <limits>
>>> +#include <limits.h>
>>>
>>> #include "libunwind.h"
>>> #include "dwarf2.h"
>>>
>>> _______________________________________________
>>> cfe-commits mailing list
>>> cfe-commits at lists.llvm.org
>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits [4] [3]
>>> [3]
>>>
>>> Links:
>>> ------
>>> [1] http://llvm.org/viewvc/llvm-project?rev=321440&view=rev [5]
>>> [4]
>>> [2]
>>>
>>>
>>> http://llvm.org/viewvc/llvm-project/libunwind/trunk/src/Dwar
>> fParser.hpp?rev=321440&r1=321439&r2=321440&view=diff
>>
>>> [6]
>>> [5]
>>> [3] http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits [4]
>>> [3]
>>>
>>> --
>>> whitequark
>>>
>>> Links:
>>> ------
>>> [1] http://llvm.org/viewvc/llvm-project?rev=321440&view=rev [5]
>>> [2]
>>>
>>> http://llvm.org/viewvc/llvm-project/libunwind/trunk/src/Dwar
>> fParser.hpp?rev=321440&r1=321439&r2=321440&view=diff
>>
>>> [6]
>>> [3] http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits [4]
>>> [4] http://llvm.org/viewvc/llvm-project?rev=321440&amp;view=rev
>>> [7]
>>> [5]
>>>
>>> http://llvm.org/viewvc/llvm-project/libunwind/trunk/src/Dwar
>> fParser.hpp?rev=321440&amp;r1=321439&amp;r2=32144
>> 0&amp;view=diff
>>
>>> [8]
>>>
>>
>> --
>> whitequark
>>
>>
>>
>> Links:
>> ------
>> [1]
>> http://refspecs.linuxbase.org/LSB_3.1.0/LSB-Core-generic/LSB
>> -Core-generic/ehframechpt.html
>> [2] http://llvm.org/viewvc/llvm-project?rev=321440&view=rev
>> [3]
>> http://llvm.org/viewvc/llvm-project/libunwind/trunk/src/Dwar
>> fParser.hpp?rev=321440&r1=321439&r2=321440&view=diff
>> [4] http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>> [5] http://llvm.org/viewvc/llvm-project?rev=321440&amp;view=rev
>> [6]
>> http://llvm.org/viewvc/llvm-project/libunwind/trunk/src/Dwar
>> fParser.hpp?rev=321440&amp;r1=321439&amp;r2=32144
>> 0&amp;view=diff
>> [7] http://llvm.org/viewvc/llvm-project?rev=321440&amp;amp;view=rev
>> [8]
>> http://llvm.org/viewvc/llvm-project/libunwind/trunk/src/Dwar
>> fParser.hpp?rev=321440&amp;amp;r1=321439&amp;amp;
>> r2=321440&amp;amp;view=diff
>>
>
> --
> whitequark
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20171225/a24fa82d/attachment-0001.html>

More information about the cfe-commits mailing list