* [PATCH] Fix reading .dwp files without .debug_tu_index
@ 2017-09-19 19:58 Alexander Shaposhnikov
2017-09-24 23:37 ` Alexander Shaposhnikov
2017-09-26 3:15 ` Doug Evans via gdb-patches
0 siblings, 2 replies; 7+ messages in thread
From: Alexander Shaposhnikov @ 2017-09-19 19:58 UTC (permalink / raw)
To: gdb-patches
[-- Attachment #1: Type: text/plain, Size: 2653 bytes --]
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, 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;
if (dwp_file->version == 2)
bfd_map_over_sections (dwp_file->dbfd, dwarf2_locate_v2_dwp_sections,
Kind regards,
Alexander Shaposhnikov
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Fix reading .dwp files without .debug_tu_index
2017-09-19 19:58 [PATCH] Fix reading .dwp files without .debug_tu_index Alexander Shaposhnikov
@ 2017-09-24 23:37 ` Alexander Shaposhnikov
2017-09-26 3:15 ` Doug Evans via gdb-patches
1 sibling, 0 replies; 7+ messages in thread
From: Alexander Shaposhnikov @ 2017-09-24 23:37 UTC (permalink / raw)
To: gdb-patches, dje, palves
cc: Doug Evans, Pedro Alves
Kind regards,
Alexander Shaposhnikov
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, 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;
>
> if (dwp_file->version == 2)
> bfd_map_over_sections (dwp_file->dbfd, dwarf2_locate_v2_dwp_sections,
>
>
> Kind regards,
> Alexander Shaposhnikov
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Fix reading .dwp files without .debug_tu_index
2017-09-19 19:58 [PATCH] Fix reading .dwp files without .debug_tu_index Alexander Shaposhnikov
2017-09-24 23:37 ` Alexander Shaposhnikov
@ 2017-09-26 3:15 ` Doug Evans via gdb-patches
2017-09-26 4:13 ` Alexander Shaposhnikov
1 sibling, 1 reply; 7+ messages in thread
From: Doug Evans via gdb-patches @ 2017-09-26 3:15 UTC (permalink / raw)
To: Alexander Shaposhnikov; +Cc: gdb-patches
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Fix reading .dwp files without .debug_tu_index
2017-09-26 3:15 ` Doug Evans via gdb-patches
@ 2017-09-26 4:13 ` Alexander Shaposhnikov
2017-09-26 17:51 ` Doug Evans via gdb-patches
0 siblings, 1 reply; 7+ messages in thread
From: Alexander Shaposhnikov @ 2017-09-26 4:13 UTC (permalink / raw)
To: Doug Evans; +Cc: gdb-patches
>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)
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.
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
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Fix reading .dwp files without .debug_tu_index
2017-09-26 4:13 ` Alexander Shaposhnikov
@ 2017-09-26 17:51 ` Doug Evans via gdb-patches
2017-09-26 18:48 ` Alexander Shaposhnikov
0 siblings, 1 reply; 7+ messages in thread
From: Doug Evans via gdb-patches @ 2017-09-26 17:51 UTC (permalink / raw)
To: Alexander Shaposhnikov; +Cc: gdb-patches
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
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Fix reading .dwp files without .debug_tu_index
2017-09-26 17:51 ` Doug Evans via gdb-patches
@ 2017-09-26 18:48 ` Alexander Shaposhnikov
0 siblings, 0 replies; 7+ messages in thread
From: Alexander Shaposhnikov @ 2017-09-26 18:48 UTC (permalink / raw)
To: Doug Evans; +Cc: gdb-patches
(updated patch + changelog)
changelog:
2017-09-26 Alexander Shaposhnikov <alexander.v.shaposhnikov@gmail.com>
* dwarf2read.c (open_and_init_dwp_file): Protect against dwp_file
having NULL cus or tus.
patch:
diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index b3c5fabfc0..4e6bf27364 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -11196,7 +11196,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
@@ -11206,7 +11207,13 @@ 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;
+
+ 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;
if (dwp_file->version == 2)
bfd_map_over_sections (dwp_file->dbfd, dwarf2_locate_v2_dwp_sections,
Kind regards,
Alexander Shaposhnikov
On Tue, Sep 26, 2017 at 10:51 AM, Doug Evans <dje@google.com> wrote:
> 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
> >
> >
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Fix reading .dwp files without .debug_tu_index
@ 2017-09-28 16:30 Doug Evans via gdb-patches
0 siblings, 0 replies; 7+ messages in thread
From: Doug Evans via gdb-patches @ 2017-09-28 16:30 UTC (permalink / raw)
To: Alexander Shaposhnikov; +Cc: gdb-patches
Alexander Shaposhnikov writes:
> (updated patch + changelog)
>
> changelog:
>
> 2017-09-26 Alexander Shaposhnikov <alexander.v.shaposhnikov@gmail.com>
>
> * dwarf2read.c (open_and_init_dwp_file): Protect against dwp_file
> having NULL cus or tus.
Thanks! Committed.
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2017-09-28 16:30 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-09-19 19:58 [PATCH] Fix reading .dwp files without .debug_tu_index Alexander Shaposhnikov
2017-09-24 23:37 ` Alexander Shaposhnikov
2017-09-26 3:15 ` Doug Evans via gdb-patches
2017-09-26 4:13 ` Alexander Shaposhnikov
2017-09-26 17:51 ` Doug Evans via gdb-patches
2017-09-26 18:48 ` Alexander Shaposhnikov
2017-09-28 16:30 Doug Evans via gdb-patches
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).