Re: [Libunwind-devel] [PATCH] arm: ptrace: Fix order of probing unwind tables

[Yichao Yu]

Updated patch with fallback fixed here
https://github.com/yuyichao/libunwind/commit/832b7644268cb11b1170729200309706ddf91f50

On Fri, Jan 20, 2017 at 3:40 PM, Yichao Yu <address@hidden> wrote:
>> As expected, your patchset regressed libunwind to the state when unw_step()
>> fails to unwind a function that has [cantunwind] entry in ARM EXTBL and
>> good DWARF info. Generally, this is because of your modification in
>> unw_step() (src/arm/Gstep.c) that made arm_exidx_step() to be done first
>> before dwarf_step().
>
> Sounds like the fallback does not handle that very well. Seems like
> the `ret == -UNW_ESTOPUNWIND` check should be removed. Does that make
> the fallback works?
>
>> No matter whether Ken Werner was right or not, he was the first who declared
>> it, his patch was applied to libunwind and did not cause any problems to 
>> other
>> people. When you contradict with any changes done before your work, it is 
>> very
>> important for you to provide any proof that someone (e.g. Ken Werner) was not
>> completely correct.
>> That is what I mean.
>
> I agree that there shouldn't be regression and if changing the logic
> in the error checking patch fixes it, I still think it's the right way
> to go for unwinding.
> My proof would be that "more powerful" is not the right metric for
> unwinding, reliability is.
> My claim is that as long as the extbl info is present and is not
> CANT_UNWIND, it has to be correct since c++ relies on it for exception
> handling and not just backtrace/debugging. Or in another word, a bug
> in the extbl unwind info is a much serious compiler bug than a bug in
> the debug info.
> For info's that are not present in it or for anything other than
> unwinding, the "more powerful" argument would apply.
>
>>> This is for unwinding of functions dumped to a shared object from a
>>> LLVM JIT. We generate DWARF 4 debug info which does not make libunwind
>>> too happy and I had to disable a DWARF version check so that could be
>>> why the debug info unwinding fails sometimes. It could also be that
>>> the debug info generated by LLVM is not accurate.
>>
>> It sounds like there is a bug in DWARF v4 parsing, but instead of
>> debugging and fixing it, you just added "smart" version of
>> UNW_ARM_UNWIND_METHOD environment variable.
>
> Again, I totally agree that your use case (or any other cases that has
> working unwinding with debug info) and I'm sorry that I didn't realize
> that unwinding is aborted if a function has CANT_UNWIND so I think
> that should be fixed and there shouldn't be regression with debug info
> unwinding anymore.
>
>> May be you can not use UNW_ARM_UNWIND_METHOD for some reason, ok.
>> Neither me can. Some of the functions in our example do not have DWARF info,
>> but present in ARM EXTBL.
>>
>>
>>> For lookup up IP-to-function mapping, I'll definitely **NOT** use
>>> EXTBL.
>>
>> You would not, but arm_exidx_step() actually does.
>
> `arm_exidx_step` does not need to map ip to function, it only needs to
> restore register values (unwinding).
> IP to function is done in, e.g., unw_get_proc_info_by_ip etc and I
> certainly wouldn't prefer getting extbl info for that. This is also
> precisely why I added the flag instead of changing the default order
> as in 92327a3. I believe the extbl first order only makes sense for
> unwinding and not for any other users of the function.
>
>>
>>
>> Regards,
>> Vitaly.
>>
>> _______________________________________________
>> Libunwind-devel mailing list
>> address@hidden
>> https://lists.nongnu.org/mailman/listinfo/libunwind-devel