Yes, that would definitely be a good idea. I was just saying that we
shouldn't gate *any* .debug_frame support on being able to read a
dwarf4 .debug_frame section.

BTW, we've had a short chat about this with Abidh on IRC, I am going
to paste the relevant part here, to make sure everyone is on the same
page:

<labath> after looking at the code, i think the eh_frame/debug_frame
choice should be per function, not per-module
<labath> that's way more useful, and it should allow more reasonable testing
<abidh> I am a little confused. You are saying that compiler should
provide this per function choice
<labath> no
<labath> right
<labath> so what the current patch does is:
<labath> if (we_have_eh_frame()) frame = eh_frame() else frame = debug_frame()
<labath> (this is in the initialization code)
<labath> and then later, when we actually try to get an unwind plan
for some function, we do:
<labath> if (frame->can_you_unwind(function)) return
frame->get_unwind_plan(function)
<labath> what I am proposing instead is for the init code to do:
<labath> frame1 = eh_frame(); frame2 = debug_frame();
<labath> and then later on (this would be the
UnwindTable::GetFuncUnwindersContainingAddress function)
<labath> if (frame1->can_you_unwind(function)) return
frame1->get_unwind_plan(function)
<labath> else if (frame2->can_you_unwind(function)) return
frame2->get_unwind_plan(function)
<labath> so the choice which unwind section you use happens on a
per-function basis


On 25 May 2017 at 22:42, Robinson, Paul <paul.robin...@sony.com> wrote:
> IANA lldb developer, but I should think lldb would want to understand a
> DWARF 4 .debug_frame section.  (And it didn't change in DWARF 5, either!)
> --paulr
>
>> -----Original Message-----
>> From: lldb-commits [mailto:lldb-commits-boun...@lists.llvm.org] On Behalf
>> Of Pavel Labath via lldb-commits
>> Sent: Thursday, May 25, 2017 9:17 AM
>> To: Tatyana Krasnukha
>> Cc: lldb-commits@lists.llvm.org
>> Subject: Re: [Lldb-commits] [PATCH] D33504: Fix FDE indexing while scan
>> debug_info section
>>
>> +lldb-commits
>>
>> Yes, that is certainly a viable approach. If there is a subset of
>> dwarf that we are capable of parsing correctly then it would be great
>> to enable that.
>>
>> On 25 May 2017 at 16:46, Tatyana Krasnukha
>> <tatyana.krasnu...@synopsys.com> wrote:
>> > This binary uses DWARF 4, that introduces two additional fields in CIE -
>> address_size and segment_size just after augmentation field. That’s why
>> values are parsed  incorrectly.
>> > May be we should discard unwind plan if there is unsupported dwarf
>> version for now?
>> >
>> > Thanks,
>> > Tatyana
>> >
>> > -----Original Message-----
>> > From: Pavel Labath [mailto:lab...@google.com]
>> > Sent: Thursday, 25 May, 2017 5:45 PM
>> > To: tatyana.krasnu...@synopsys.com
>> > Subject: Re: [PATCH] D33504: Fix FDE indexing while scan debug_info
>> section
>> >
>> > Hey, try the attached binary for size.
>> >
>> > It was generated from the source file in
>> lldb/test/testcases/functionalities/breakpoint/breakpoint_conditions/main.
>> c,
>> > while targetting android arm64.
>> >
>> > You should be able to create a test binary for yourself locally
>> (assuming you're on linux) like this:
>> > $ cat a.c
>> > int a(char *x) { return *x; }
>> >
>> > int f() {
>> >   return a(0);
>> > }
>> >
>> > int g() {
>> >   return 1+f();
>> > }
>> >
>> > int _start() { return 2+g(); }
>> >
>> > $ clang -fno-exceptions -fno-unwind-tables -g a.c -nostdlib
>> >
>> > If you debug the binary with and stop in the ParseCIE function, you will
>> see that it parses the entry incorrectly:
>> > Process 39688 stopped
>> > * thread #1, name = 'lldb', stop reason = step over
>> >     frame #0: 0x00007ffff074bb53
>> >
>> liblldb.so.5`lldb_private::DWARFCallFrameInfo::ParseCIE(this=0x00000000006
>> 13280,
>> > cie_offset=0) at DWARFCallFrameInfo.cpp:303
>> >    300    cie_sp->data_align = (int32_t)m_cfi_data.GetSLEB128(&offset);
>> >    301    cie_sp->return_addr_reg_num = m_cfi_data.GetU8(&offset);
>> >    302
>> > -> 303    if (cie_sp->augmentation[0]) {
>> >    304      // Get the length of the eh_frame augmentation data
>> >    305      // which starts with a ULEB128 length in bytes
>> >    306      const size_t aug_data_len =
>> (size_t)m_cfi_data.GetULEB128(&offset);
>> >
>> > (lldb) fr var cie_sp._M_ptr[0]
>> > (lldb_private::DWARFCallFrameInfo::CIE) cie_sp._M_ptr[0] = {
>> >   cie_offset = 0
>> >   version = '\x04'
>> >   augmentation = ""
>> >   code_align = 8 <====== wrong
>> >   data_align = 0 <====== wrong
>> >   return_addr_reg_num = 1 <===== wrong
>> >   inst_offset = 0
>> >   inst_length = 0
>> >   ptr_encoding = '\0'
>> >   lsda_addr_encoding = '\xff'
>> >   personality_loc = 18446744073709551615
>> >   initial_row = {
>> >     m_offset = 0
>> >     m_cfa_value = {
>> >       m_type = unspecified
>> >       m_value = {
>> >         reg = (reg_num = 0, offset = 0)
>> >         expr = (opcodes = 0x0000000000000000, length = 0)
>> >       }
>> >     }
>> >     m_register_locations = size=0 {}
>> >   }
>> > }
>> >
>> > Correct parsing can be seen by running
>> > $ llvm-dwarfdump a.out
>> > .....
>> > .debug_frame contents:
>> >
>> > 00000000 00000014 ffffffff CIE
>> >   Version:               4
>> >   Augmentation:          ""
>> >   Address size:          8  <=== this shows up in code_align
>> >   Segment desc size:     0 <=== this shows up in data_align
>> >   Code alignment factor: 1 <=== this shows up in return_addr_reg_num
>> >   Data alignment factor: -8
>> >   Return address column: 16
>> >
>> >   DW_CFA_def_cfa: reg7 +8
>> >   DW_CFA_offset: reg16 -8
>> >   DW_CFA_nop:
>> >   DW_CFA_nop:
>> >   DW_CFA_nop:
>> >   DW_CFA_nop:
>> >
>> > 00000018 0000001c 00000000 FDE cie=00000000 pc=004000b0...004000c1
>> >   DW_CFA_advance_loc: 1
>> >   DW_CFA_def_cfa_offset: +16
>> >   DW_CFA_offset: reg6 -16
>> >   DW_CFA_advance_loc: 3
>> >   DW_CFA_def_cfa_register: reg6
>> > ....
>> >
>> >
>> > Interestingly, in the x86_64 linux case, lldb still manages to unwind
>> correctly despite the bad parsing.
>> >
>> >
>> >
>> >
>> > On 25 May 2017 at 14:40, Tatyana Krasnukha via Phabricator
>> <revi...@reviews.llvm.org> wrote:
>> >> tatyana-krasnukha added a comment.
>> >>
>> >> Yes, give those binaries, please.
>> >>
>> >>
>> >> Repository:
>> >>   rL LLVM
>> >>
>> >> https://urldefense.proofpoint.com/v2/url?u=https-3A__reviews.llvm.org_
>> >> D33504&d=DwIBaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=8NZfjV_ZLY_S7gZyQMq8mj7tiN4
>> >> vlymPiSt0Wl0jegw&m=Ze6ANiK7goV6lSPJcY8QglIJ_tZC0gXeUuqpQ8n4K40&s=1vfSh
>> >> WELC8pd8-DQmAeW3QJDQgdMAJvn905_U-jVphM&e=
>> >>
>> >>
>> >>
>> _______________________________________________
>> lldb-commits mailing list
>> lldb-commits@lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Reply via email to