Re: [PATCH] Fix reading .dwp files without .debug_tu_index

["Doug Evans via gdb-patches" <gdb-patches at sourceware dot org>]

On Mon, Sep 25, 2017 at 9:13 PM, Alexander Shaposhnikov
<alexander.v.shaposhnikov@gmail.com> wrote:
>
>  >I'd go with something like (untested and likely improperly formatted):
>  >if (dwp_file->cus)
>  >   dwp_file->version = dwp_file-cus->version;
>  > else if (dwp_file->tus)
>  >   dwp_file->version = dwp_file->tus->version
>  > else
>  >   dwp_file->version = 2;
>
> i'm ok with that change, many thanks,
> do i need to resend the patch ? (and yeah, i can double check / test it when
> i get home)

No need, if you're confident all the nits are taken care of [ChangeLog
syntax and contents, patch whitespace, testing, and so on ... :-)]

> regarding the dependence on the order of sections - (if i am not mistaken)
> yeah, it's a separate issue - i can provide more details
> when i get to my laptop.

Thanks.


>
> Best regards,
> Alexander Shaposhnikov
>
> On Mon, Sep 25, 2017 at 8:14 PM, Doug Evans <dje@google.com> wrote:
>>
>> On Tue, Sep 19, 2017 at 12:58 PM, Alexander Shaposhnikov
>> <alexander.v.shaposhnikov@gmail.com> wrote:
>> > This patch fixes segmentation fault (due to dereferencing of a null
>> > pointer)
>> > in dwarf2read.c in the code dwp_file->cus->version !=
>> > dwp_file->tus->version
>> > by adding defensive checks similarly to how it's already done
>> > at the lines 11208 - 11215 (in the same file dwarf2read.c).
>> > The issue can be reproduced with dwp packages built by llvm-dwp utility
>> > (from the current upstream) since at the moment llvm-dwp doesn't create
>> > .debug_tu_index section, thus dwp_file->tus will be null.
>> >
>> > Test plan:
>> >
>> > BUILD:
>> > main.cpp:
>> > int f() {
>> >   int x = 0;
>> >   int y = 1;
>> >   return x + y;
>> > }
>> > g++ -fPIC -gsplit-dwarf -g -O0 main.cpp -o main.exe
>> > llvm-dwp main.dwo -o main.exe.dwp
>> > # This step is a workaround to a separate issue (unrelated to this
>> > patch).
>> > # At the moment llvm tools & clang put .strtab section first (its index
>> > is
>> > 1),
>> > # while gcc/gold/binutils put it at the end.
>> > # Unfortunately gdb (in the code reading dwps) appears to depend on the
>> > order
>> > # of sections,
>>
>> That's odd. Depends on the order of sections how?
>>
>> > to workaround this (to reproduce the issue which this patch
>> > # aims to address) we use objcopy to do the following trick:
>> > # if one asks to remove .strtab objcopy will do that but at the same
>> > time
>> > # it will create a new .shstrtab section at the end.
>> > objcopy --remove-section .strtab main.exe.dwp
>> > RUN:
>> > gdb main.exe
>> > One can observe that now there is no crash and debugging functionality
>> > works as expected (setting breakpoints, printing local variable,
>> > exploring
>> > the stack).
>> >
>> > gdb/ChangeLog:
>> > yyyy-mm-dd  Alexander Shaposhnikov <alexander.v.shaposhnikov@gmail.com>
>> >
>> > * dwarf2read.c: Fix segmentation fault on reading
>> >
>> >   .dwp files without .debug_tu_index section.
>> >
>> > Patch:
>> >
>> > diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
>> >
>> > index b1914cf876..547e3f034e 100644
>> > --- a/gdb/dwarf2read.c
>> > +++ b/gdb/dwarf2read.c
>> > @@ -11185,7 +11185,8 @@ open_and_init_dwp_file (void)
>> >    dwp_file->tus = create_dwp_hash_table (dwp_file, 1);
>> >
>> >    /* The DWP file version is stored in the hash table.  Oh well.  */
>> > -  if (dwp_file->cus->version != dwp_file->tus->version)
>> > +  if (dwp_file->cus && dwp_file->tus
>> > +      && dwp_file->cus->version != dwp_file->tus->version)
>> >      {
>> >        /* Technically speaking, we should try to limp along, but this is
>> >          pretty bizarre.  We use pulongest here because that's the
>> > established
>> > @@ -11195,7 +11196,7 @@ open_and_init_dwp_file (void)
>> >              pulongest (dwp_file->cus->version),
>> >              pulongest (dwp_file->tus->version), dwp_name.c_str ());
>> >      }
>> > -  dwp_file->version = dwp_file->cus->version;
>> > +  dwp_file->version = dwp_file->cus ? dwp_file->cus->version : 0;
>>
>> A version of 0 doesn't seem right.
>> I'd go with something like (untested and likely improperly formatted):
>>
>>   if (dwp_file->cus)
>>     dwp_file->version = dwp_file-cus->version;
>>   else if (dwp_file->tus)
>>     dwp_file->version = dwp_file->tus->version
>>   else
>>     dwp_file->version = 2;
>>
>> ok with that change.
>>
>> >
>> >    if (dwp_file->version == 2)
>> >      bfd_map_over_sections (dwp_file->dbfd,
>> > dwarf2_locate_v2_dwp_sections,
>> >
>> >
>> > Kind regards,
>> > Alexander Shaposhnikov
>
>