[PATCH] Fix issues with reading rnglists, especially from dwo files, for DWARF v5

classic Classic list List threaded Threaded
29 messages Options
12
Reply | Threaded
Open this post in threaded view
|

[PATCH] Fix issues with reading rnglists, especially from dwo files, for DWARF v5

Sourceware - gdb-patches mailing list
While experimenting with GDB on DWARF 5 with split debug (dwo files),
I discovered that GDB was not reading the rnglist index
properly (it needed to be reprocessed in the same way the loclist
index does), and that there was no code for reading rnglists out of
dwo files at all.  Also, the rnglist address reading function
(dwarf2_rnglists_process) was adding the base address to all rnglist
entries, when it's only supposed to add it to the DW_RLE_offset_pair
entries (http://dwarfstd.org/doc/DWARF5.pdf, p. 53), and was not
handling several entry types.

This patch fixes these issues.  I verified that it fixes the issues I
saw and that it does not cause any testsuite regressions (on x86_64
ubuntu linux).  Is there anything else I need to do? Is this Ok to
commit?

-- Caroline Tice
[hidden email]

gdb/ChangeList:

2020-06-01  Caroline Tice  <[hidden email]>

        * dwarf2/read.c (struct dwop_section_names): Add rnglists_dwo.
        (dwop_section_names): Add .debug_rnglists.dwo, .zdebug_rnglists.dwo.
        (struct dwo_sectionds): Add rnglists field.
        (struct virtual_v2_dwo_sections): Add loclists_offset, loclists_size,
        rnglists_offset, rnglists_size.
        (cu_debug_rnglist_section): New function (decl & definition).
        (dwarf2_locate_dwo_sections): Add code to read rnglists_dwo section.
        (dwarf2_rnglists_process): Add cases for DW_RLE_base_addressx,
        DW_RLE_startx_length, DW_RLE_startx_endx.  Also, update to only add
        the base address to DW_RLE_offset_pairs (not to all ranges).
        (read_full_die_1): Add code to read DW_AT_rnglists_base and assign to
        cu->ranges_base.
        (read_rnglist_index): New function.
        (read_attribute_reprocess):  Add code for DW_FORM_rnglistx.
        (read_attribute_value): Mark DW_FORM_rnglistx with need_reprocess.

gdb-dwo-rnglists.patch (12K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix issues with reading rnglists, especially from dwo files, for DWARF v5

Tom Tromey-2
>>>>> "Caroline" == Caroline Tice via Gdb-patches <[hidden email]> writes:

Caroline> This patch fixes these issues.  I verified that it fixes the
Caroline> issues I saw and that it does not cause any testsuite
Caroline> regressions (on x86_64 ubuntu linux).

Is there a way to reproduce the failures with the current test suite?
Say, by running some test using some board file?

Caroline> @@ -793,12 +796,18 @@ struct virtual_v2_dwo_sections
Caroline>    bfd_size_type loc_offset;
Caroline>    bfd_size_type loc_size;
 
Caroline> +  bfd_size_type loclists_offset;
Caroline> +  bfd_size_type loclists_size;
Caroline> +
Caroline>    bfd_size_type macinfo_offset;
Caroline>    bfd_size_type macinfo_size;
 
Caroline>    bfd_size_type macro_offset;
Caroline>    bfd_size_type macro_size;
 
Caroline> +  bfd_size_type rnglists_offset;
Caroline> +  bfd_size_type rnglists_size;

These new members don't seem to be used anywhere.

Caroline> +static struct dwarf2_section_info *cu_debug_rnglist_section (struct
Caroline> +     dwarf2_cu *cu);

Probably better to split before the "(" and then indent the continuation
line 2 spaces.

Caroline> - default:
Caroline> +        case DW_RLE_startx_endx:

Looks like the indentation here is incorrect.

Caroline> +
Caroline> +  attr =  die->attr (DW_AT_rnglists_base);

Extra space after the "=".

Tom
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix issues with reading rnglists, especially from dwo files, for DWARF v5

Sourceware - gdb-patches mailing list
On Mon, Jun 1, 2020 at 1:33 PM Tom Tromey <[hidden email]> wrote:
>
> >>>>> "Caroline" == Caroline Tice via Gdb-patches <[hidden email]> writes:
>
> Caroline> This patch fixes these issues.  I verified that it fixes the
> Caroline> issues I saw and that it does not cause any testsuite
> Caroline> regressions (on x86_64 ubuntu linux).
>
> Is there a way to reproduce the failures with the current test suite?
> Say, by running some test using some board file?

I have not been able to reproduce it with the current testsuite.  The
issue is largely related to processing DW_FORM_rmglistx, which I have
not been able to get GCC to produce (it generated a DW_FORM_sec_offset
instead); I get my issue using llvm.  I will attempt to attach a
gzip'ed tarball (if gmail will let me) containing my small test case.
It has 2 .cpp files, 1 .h file, the two clang-generated .dwo files,
and the final binary.  The command I used to compile the sources, and
obtain the .dwo & binary files is:

$ clang++ -gdwarf-5 -O0 -gsplit-dwarf pre-order.cpp
pre-order-common.cpp -o pre-order

To see the first of the issues, you need to enable complaints (change
the value of "stop_whining" in complaints.c to something like 10).
When you do that, rebuild gdb, and load the binary into gdb, it
immediately complains:

$ ~/fsf-gdb.clean.obj/gdb/gdb pre-order
...
Reading symbols from pre-order...
During symbol reading: Invalid .debug_rnglists data (no base address)
During symbol reading: Invalid .debug_rnglists data (no base address)
(gdb) quit

This is because of the incorrect reading of the DW_FORM_rnglistx
index.  GDB then ignores the rnglists altogether because it can't read
them.



>
> Caroline> @@ -793,12 +796,18 @@ struct virtual_v2_dwo_sections
> Caroline>    bfd_size_type loc_offset;
> Caroline>    bfd_size_type loc_size;
>
> Caroline> +  bfd_size_type loclists_offset;
> Caroline> +  bfd_size_type loclists_size;
> Caroline> +
> Caroline>    bfd_size_type macinfo_offset;
> Caroline>    bfd_size_type macinfo_size;
>
> Caroline>    bfd_size_type macro_offset;
> Caroline>    bfd_size_type macro_size;
>
> Caroline> +  bfd_size_type rnglists_offset;
> Caroline> +  bfd_size_type rnglists_size;
>
> These new members don't seem to be used anywhere.
Oh.  Those get used in my next upcoming patch (where I update GDB to
handle DWARFv5 .dwp files).  I can either leave them in this patch, or
remove them from here and put them in the next one, whichever you
prefer.

>
> Caroline> +static struct dwarf2_section_info *cu_debug_rnglist_section (struct
> Caroline> +                                                          dwarf2_cu *cu);
>
> Probably better to split before the "(" and then indent the continuation
> line 2 spaces.
>
> Caroline> -     default:
> Caroline> +        case DW_RLE_startx_endx:
>
> Looks like the indentation here is incorrect.
>
> Caroline> +
> Caroline> +  attr =  die->attr (DW_AT_rnglists_base);
>
> Extra space after the "=".
Thanks for the review!  I will fix the formatting issues, and resubmit
as soon as you indicate whether to leave in or take out the currently
unused fields.

-- Caroline

>
> Tom

gdb-rnglist-test.tar.gz (36K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix issues with reading rnglists, especially from dwo files, for DWARF v5

Tom Tromey-2
Caroline> $ clang++ -gdwarf-5 -O0 -gsplit-dwarf pre-order.cpp
Caroline> pre-order-common.cpp -o pre-order

I wonder if the test suite can be run this way.

Caroline> Oh.  Those get used in my next upcoming patch (where I update GDB to
Caroline> handle DWARFv5 .dwp files).  I can either leave them in this patch, or
Caroline> remove them from here and put them in the next one, whichever you
Caroline> prefer.

I think it's better for patches to be reasonably self-contained when
possible.

Also, there were other patches for DWARFv5 on the list in "recent" (this
year) times.  I wonder if those are relevant / related.

Tom
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix issues with reading rnglists, especially from dwo files, for DWARF v5

Sourceware - gdb-patches mailing list
I've been playing with running the testsuite with -gdwarf-5
-gsplit-dwarf and using the clang compiler. I did not find any tests
that showed the issues I was running into (but I did discover a small
bug in my patch, which my newer patch fixes).  With my fixed patch,
there are no testsuite differences.  I have created a new testsuite
test case, which does fail with current ToT and passes with my patch,
but that only works if you compile it with clang -- if you compile it
with GCC it passes in both cases (because GCC is not generating the
DW_FORM_rmglistx that my patch is handling/fixing).

I've updated the patch to include the requested format changes, remove
the pieces that are only used in an upcoming patch,  fix the small
issue I found while testing, and I've added the test to the testsuite.
To run the test and see the issue:

$  make check RUNTESTFLAGS="CC_FOR_TARGET=${CLANG_CC}
CXX_FOR_TARGET=${CLANG_CXX} dw5-rnglist-test.exp"

gdb/ChangeLog (updated)

2020-06-04  Caroline Tice  <[hidden email]>

        * dwarf2/read.c (struct dwop_section_names): Add rnglists_dwo.
        (dwop_section_names): Add .debug_rnglists.dwo, .zdebug_rnglists.dwo.
        (struct dwo_sections): Add rnglists field.
        (cu_debug_rnglist_section): New function (decl & definition).
        (dwarf2_locate_dwo_sections): Add code to read rnglists_dwo section.
        (dwarf2_rnglists_process): Add cases for DW_RLE_base_addressx,
        DW_RLE_startx_length, DW_RLE_startx_endx.  Also, update to only add
        the base address to DW_RLE_offset_pairs (not to all ranges).
        (dwarf2_get_pc_bounds): Check for DW_FORM_rnglistx when setting
        need_ranges_base.
        (dwarf2_record_block_ranges): Check for DW_FORM_rnglistx when setting
        need_ranges_base.
        (read_full_die_1): Add code to read DW_AT_rnglists_base and assign to
        cu->ranges_base.
        (partial_die_info::read): Check for DW_FORM_rnglistx when setting
        need_ranges_base.
        (read_rnglist_index): New function.
        (read_attribute_reprocess):  Add code for DW_FORM_rnglistx.
        (read_attribute_value): Mark DW_FORM_rnglistx with need_reprocess.

gdb/testsuite/ChangeLog:

2020-06-04  Caroline Tice  <[hidden email]>

        * gdb.dwarf2/dw5-rnglist-test.cc: New file.
        * gdb.dwarf2/dw5-rnglist-test.exp: New file.





On Wed, Jun 3, 2020 at 7:49 AM Tom Tromey <[hidden email]> wrote:

>
> Caroline> $ clang++ -gdwarf-5 -O0 -gsplit-dwarf pre-order.cpp
> Caroline> pre-order-common.cpp -o pre-order
>
> I wonder if the test suite can be run this way.
>
> Caroline> Oh.  Those get used in my next upcoming patch (where I update GDB to
> Caroline> handle DWARFv5 .dwp files).  I can either leave them in this patch, or
> Caroline> remove them from here and put them in the next one, whichever you
> Caroline> prefer.
>
> I think it's better for patches to be reasonably self-contained when
> possible.
>
> Also, there were other patches for DWARFv5 on the list in "recent" (this
> year) times.  I wonder if those are relevant / related.
>
> Tom

gdb-dwo-rnglists.patch (19K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix issues with reading rnglists, especially from dwo files, for DWARF v5

Sourceware - gdb-patches mailing list
While playing with this a bit more (getting ready to work on my .dwp
file patch), I noticed that there was an important bit I left out of
the original patch: dwarf2_rnglists_process, in the original code,
always reads out of the rnglist section in the main objfile, even when
there is a .rnglist section in a .dwo file that it should be reading
instead.  I had some local code that fixes this, but it didn't make it
into my previous patch (I apologize for that).  The attached patch has
been updated to contain this fix as well.  It also contains my fixes
and testcase from the previous patch.

I ran the testsuite again (adding the -gdwarf-5 and -gsplit-dwarf
flags in testsuite/lib/gdb.exp), and compiling everything with clang:

$ make check RUNTESTFLAGS="CC_FOR_TARGET=${CLANG_CC}
CXX_FOR_TARGET=${CLANG_CXX}"

This time my patched GDB passed quite a few tests that the unpatched
version failed (when compiled with clang and passed the flags):

Testsuite summary from unpatched GDB:

=== gdb Summary ===
# of expected passes 55725
# of unexpected failures 2956
# of unexpected successes 83
# of expected failures 169
# of known failures 57
# of unresolved testcases 115
# of untested testcases 175
# of unsupported tests 272
# of paths in test names 1
# of duplicate test names 269


Testsuite summary from patched GDB:

=== gdb Summary ===

# of expected passes 56173
# of unexpected failures 2523
# of unexpected successes 2
# of expected failures 250
# of known failures 57
# of unresolved testcases 111
# of untested testcases 174
# of unsupported tests 272
# of paths in test names 1
# of duplicate test names 269

Some of the tests that have more passes with the patch include:
gdb.base/break-entry.exp, gdb.base/info-fun.exp,
gdb.base/info-shared.exp,gdb.base/return-nodebug.exp,
gdb.dwarf2/fission-base.exp
(I can send you a complete list if you really want it)

Anyway below is the updated patch for review.

-- Caroline
[hidden email]

gdb/ChangeLog:

2020-06-09  Caroline Tice  <[hidden email]>

        * dwarf2/read.c (struct dwop_section_names): Add rnglists_dwo.
        (dwop_section_names): Add .debug_rnglists.dwo, .zdebug_rnglists.dwo.
        (struct dwarf2_cu): Add cu_ranges_from_skeleton field.
        (struct dwo_sections): Add rnglists field.
        (dwarf2_ranges_read): Add tag parameter.
        (cu_debug_rnglist_section): New function (decl & definition).
        (cutu_reader::cutu_reader): Before replacing the skeleton unit
        comp_unit_die with the dwo comp_unit_die, check to see if the skeleton
        unit die has a DW_AT_ranges, and if so set the cu_ranges_from_skeleton
        field in the cu.
        (dwarf2_locate_dwo_sections): Add code to read rnglists_dwo section.
        (dwarf2_rnglists_process): Add a dwarf_tag parameter, for the kind of
        die whose range is being checked; add code to read the rnglist section
        from the dwo file rather than from the main objfile, if appropriate.
        Add cases for DW_RLE_base_addressx,
        DW_RLE_startx_length, DW_RLE_startx_endx.  Also, update to only add
        the base address to DW_RLE_offset_pairs (not to all ranges).
        (dwarf2_ranges_process): Add dwarf tag parameter and pass it to
        dwarf2_rnglists_process.
        (dwarf2_ranges_read): Add dwarf tag parameter and pass it to
        dwarf2_ranges_process.
        (dwarf2_get_pc_bounds): Check for DW_FORM_rnglistx when setting
        need_ranges_base.  Also pass die tag to dwarf2_ranges_read.
        (dwarf2_record_block_ranges): Check for DW_FORM_rnglistx when setting
        need_ranges_base.  Also pass die tag to dwarf2_ranges_read.
        (read_full_die_1): Add code to read DW_AT_rnglists_base and assign to
        cu->ranges_base.
        (partial_die_info::read): Check for DW_FORM_rnglistx when setting
        need_ranges_base.  Also pass die tag to dwarf2_ranges_read.
        (read_rnglist_index): New function.
        (read_attribute_reprocess):  Add code for DW_FORM_rnglistx.
        (read_attribute_value): Mark DW_FORM_rnglistx with need_reprocess.

gdb/testsuite/ChangeLog:

2020-06-09  Caroline Tice  <[hidden email]>

        * gdb.dwarf2/dw5-rnglist-test.cc: New file.
        * gdb.dwarf2/dw5-rnglist-test.exp: New file.


On Thu, Jun 4, 2020 at 2:39 PM Caroline Tice <[hidden email]> wrote:

>
> I've been playing with running the testsuite with -gdwarf-5
> -gsplit-dwarf and using the clang compiler. I did not find any tests
> that showed the issues I was running into (but I did discover a small
> bug in my patch, which my newer patch fixes).  With my fixed patch,
> there are no testsuite differences.  I have created a new testsuite
> test case, which does fail with current ToT and passes with my patch,
> but that only works if you compile it with clang -- if you compile it
> with GCC it passes in both cases (because GCC is not generating the
> DW_FORM_rmglistx that my patch is handling/fixing).
>
> I've updated the patch to include the requested format changes, remove
> the pieces that are only used in an upcoming patch,  fix the small
> issue I found while testing, and I've added the test to the testsuite.
> To run the test and see the issue:
>
> $  make check RUNTESTFLAGS="CC_FOR_TARGET=${CLANG_CC}
> CXX_FOR_TARGET=${CLANG_CXX} dw5-rnglist-test.exp"
>
> gdb/ChangeLog (updated)
>
> 2020-06-04  Caroline Tice  <[hidden email]>
>
>         * dwarf2/read.c (struct dwop_section_names): Add rnglists_dwo.
>         (dwop_section_names): Add .debug_rnglists.dwo, .zdebug_rnglists.dwo.
>         (struct dwo_sections): Add rnglists field.
>         (cu_debug_rnglist_section): New function (decl & definition).
>         (dwarf2_locate_dwo_sections): Add code to read rnglists_dwo section.
>         (dwarf2_rnglists_process): Add cases for DW_RLE_base_addressx,
>         DW_RLE_startx_length, DW_RLE_startx_endx.  Also, update to only add
>         the base address to DW_RLE_offset_pairs (not to all ranges).
>         (dwarf2_get_pc_bounds): Check for DW_FORM_rnglistx when setting
>         need_ranges_base.
>         (dwarf2_record_block_ranges): Check for DW_FORM_rnglistx when setting
>         need_ranges_base.
>         (read_full_die_1): Add code to read DW_AT_rnglists_base and assign to
>         cu->ranges_base.
>         (partial_die_info::read): Check for DW_FORM_rnglistx when setting
>         need_ranges_base.
>         (read_rnglist_index): New function.
>         (read_attribute_reprocess):  Add code for DW_FORM_rnglistx.
>         (read_attribute_value): Mark DW_FORM_rnglistx with need_reprocess.
>
> gdb/testsuite/ChangeLog:
>
> 2020-06-04  Caroline Tice  <[hidden email]>
>
>         * gdb.dwarf2/dw5-rnglist-test.cc: New file.
>         * gdb.dwarf2/dw5-rnglist-test.exp: New file.
>
>
>
>
>
> On Wed, Jun 3, 2020 at 7:49 AM Tom Tromey <[hidden email]> wrote:
> >
> > Caroline> $ clang++ -gdwarf-5 -O0 -gsplit-dwarf pre-order.cpp
> > Caroline> pre-order-common.cpp -o pre-order
> >
> > I wonder if the test suite can be run this way.
> >
> > Caroline> Oh.  Those get used in my next upcoming patch (where I update GDB to
> > Caroline> handle DWARFv5 .dwp files).  I can either leave them in this patch, or
> > Caroline> remove them from here and put them in the next one, whichever you
> > Caroline> prefer.
> >
> > I think it's better for patches to be reasonably self-contained when
> > possible.
> >
> > Also, there were other patches for DWARFv5 on the list in "recent" (this
> > year) times.  I wonder if those are relevant / related.
> >
> > Tom

gdb-dwo-rnglists.updated.patch (26K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix issues with reading rnglists, especially from dwo files, for DWARF v5

Sourceware - gdb-patches mailing list
Ping?  Could somebody please review this?

-- Caroline
[hidden email]


On Tue, Jun 9, 2020 at 4:32 PM Caroline Tice <[hidden email]> wrote:

> While playing with this a bit more (getting ready to work on my .dwp
> file patch), I noticed that there was an important bit I left out of
> the original patch: dwarf2_rnglists_process, in the original code,
> always reads out of the rnglist section in the main objfile, even when
> there is a .rnglist section in a .dwo file that it should be reading
> instead.  I had some local code that fixes this, but it didn't make it
> into my previous patch (I apologize for that).  The attached patch has
> been updated to contain this fix as well.  It also contains my fixes
> and testcase from the previous patch.
>
> I ran the testsuite again (adding the -gdwarf-5 and -gsplit-dwarf
> flags in testsuite/lib/gdb.exp), and compiling everything with clang:
>
> $ make check RUNTESTFLAGS="CC_FOR_TARGET=${CLANG_CC}
> CXX_FOR_TARGET=${CLANG_CXX}"
>
> This time my patched GDB passed quite a few tests that the unpatched
> version failed (when compiled with clang and passed the flags):
>
> Testsuite summary from unpatched GDB:
>
> === gdb Summary ===
> # of expected passes 55725
> # of unexpected failures 2956
> # of unexpected successes 83
> # of expected failures 169
> # of known failures 57
> # of unresolved testcases 115
> # of untested testcases 175
> # of unsupported tests 272
> # of paths in test names 1
> # of duplicate test names 269
>
>
> Testsuite summary from patched GDB:
>
> === gdb Summary ===
>
> # of expected passes 56173
> # of unexpected failures 2523
> # of unexpected successes 2
> # of expected failures 250
> # of known failures 57
> # of unresolved testcases 111
> # of untested testcases 174
> # of unsupported tests 272
> # of paths in test names 1
> # of duplicate test names 269
>
> Some of the tests that have more passes with the patch include:
> gdb.base/break-entry.exp, gdb.base/info-fun.exp,
> gdb.base/info-shared.exp,gdb.base/return-nodebug.exp,
> gdb.dwarf2/fission-base.exp
> (I can send you a complete list if you really want it)
>
> Anyway below is the updated patch for review.
>
> -- Caroline
> [hidden email]
>
> gdb/ChangeLog:
>
> 2020-06-09  Caroline Tice  <[hidden email]>
>
>         * dwarf2/read.c (struct dwop_section_names): Add rnglists_dwo.
>         (dwop_section_names): Add .debug_rnglists.dwo,
> .zdebug_rnglists.dwo.
>         (struct dwarf2_cu): Add cu_ranges_from_skeleton field.
>         (struct dwo_sections): Add rnglists field.
>         (dwarf2_ranges_read): Add tag parameter.
>         (cu_debug_rnglist_section): New function (decl & definition).
>         (cutu_reader::cutu_reader): Before replacing the skeleton unit
>         comp_unit_die with the dwo comp_unit_die, check to see if the
> skeleton
>         unit die has a DW_AT_ranges, and if so set the
> cu_ranges_from_skeleton
>         field in the cu.
>         (dwarf2_locate_dwo_sections): Add code to read rnglists_dwo
> section.
>         (dwarf2_rnglists_process): Add a dwarf_tag parameter, for the kind
> of
>         die whose range is being checked; add code to read the rnglist
> section
>         from the dwo file rather than from the main objfile, if
> appropriate.
>         Add cases for DW_RLE_base_addressx,
>         DW_RLE_startx_length, DW_RLE_startx_endx.  Also, update to only add
>         the base address to DW_RLE_offset_pairs (not to all ranges).
>         (dwarf2_ranges_process): Add dwarf tag parameter and pass it to
>         dwarf2_rnglists_process.
>         (dwarf2_ranges_read): Add dwarf tag parameter and pass it to
>         dwarf2_ranges_process.
>         (dwarf2_get_pc_bounds): Check for DW_FORM_rnglistx when setting
>         need_ranges_base.  Also pass die tag to dwarf2_ranges_read.
>         (dwarf2_record_block_ranges): Check for DW_FORM_rnglistx when
> setting
>         need_ranges_base.  Also pass die tag to dwarf2_ranges_read.
>         (read_full_die_1): Add code to read DW_AT_rnglists_base and assign
> to
>         cu->ranges_base.
>         (partial_die_info::read): Check for DW_FORM_rnglistx when setting
>         need_ranges_base.  Also pass die tag to dwarf2_ranges_read.
>         (read_rnglist_index): New function.
>         (read_attribute_reprocess):  Add code for DW_FORM_rnglistx.
>         (read_attribute_value): Mark DW_FORM_rnglistx with need_reprocess.
>
> gdb/testsuite/ChangeLog:
>
> 2020-06-09  Caroline Tice  <[hidden email]>
>
>         * gdb.dwarf2/dw5-rnglist-test.cc: New file.
>         * gdb.dwarf2/dw5-rnglist-test.exp: New file.
>
>
> On Thu, Jun 4, 2020 at 2:39 PM Caroline Tice <[hidden email]> wrote:
> >
> > I've been playing with running the testsuite with -gdwarf-5
> > -gsplit-dwarf and using the clang compiler. I did not find any tests
> > that showed the issues I was running into (but I did discover a small
> > bug in my patch, which my newer patch fixes).  With my fixed patch,
> > there are no testsuite differences.  I have created a new testsuite
> > test case, which does fail with current ToT and passes with my patch,
> > but that only works if you compile it with clang -- if you compile it
> > with GCC it passes in both cases (because GCC is not generating the
> > DW_FORM_rmglistx that my patch is handling/fixing).
> >
> > I've updated the patch to include the requested format changes, remove
> > the pieces that are only used in an upcoming patch,  fix the small
> > issue I found while testing, and I've added the test to the testsuite.
> > To run the test and see the issue:
> >
> > $  make check RUNTESTFLAGS="CC_FOR_TARGET=${CLANG_CC}
> > CXX_FOR_TARGET=${CLANG_CXX} dw5-rnglist-test.exp"
> >
> > gdb/ChangeLog (updated)
> >
> > 2020-06-04  Caroline Tice  <[hidden email]>
> >
> >         * dwarf2/read.c (struct dwop_section_names): Add rnglists_dwo.
> >         (dwop_section_names): Add .debug_rnglists.dwo,
> .zdebug_rnglists.dwo.
> >         (struct dwo_sections): Add rnglists field.
> >         (cu_debug_rnglist_section): New function (decl & definition).
> >         (dwarf2_locate_dwo_sections): Add code to read rnglists_dwo
> section.
> >         (dwarf2_rnglists_process): Add cases for DW_RLE_base_addressx,
> >         DW_RLE_startx_length, DW_RLE_startx_endx.  Also, update to only
> add
> >         the base address to DW_RLE_offset_pairs (not to all ranges).
> >         (dwarf2_get_pc_bounds): Check for DW_FORM_rnglistx when setting
> >         need_ranges_base.
> >         (dwarf2_record_block_ranges): Check for DW_FORM_rnglistx when
> setting
> >         need_ranges_base.
> >         (read_full_die_1): Add code to read DW_AT_rnglists_base and
> assign to
> >         cu->ranges_base.
> >         (partial_die_info::read): Check for DW_FORM_rnglistx when setting
> >         need_ranges_base.
> >         (read_rnglist_index): New function.
> >         (read_attribute_reprocess):  Add code for DW_FORM_rnglistx.
> >         (read_attribute_value): Mark DW_FORM_rnglistx with
> need_reprocess.
> >
> > gdb/testsuite/ChangeLog:
> >
> > 2020-06-04  Caroline Tice  <[hidden email]>
> >
> >         * gdb.dwarf2/dw5-rnglist-test.cc: New file.
> >         * gdb.dwarf2/dw5-rnglist-test.exp: New file.
> >
> >
> >
> >
> >
> > On Wed, Jun 3, 2020 at 7:49 AM Tom Tromey <[hidden email]> wrote:
> > >
> > > Caroline> $ clang++ -gdwarf-5 -O0 -gsplit-dwarf pre-order.cpp
> > > Caroline> pre-order-common.cpp -o pre-order
> > >
> > > I wonder if the test suite can be run this way.
> > >
> > > Caroline> Oh.  Those get used in my next upcoming patch (where I
> update GDB to
> > > Caroline> handle DWARFv5 .dwp files).  I can either leave them in this
> patch, or
> > > Caroline> remove them from here and put them in the next one,
> whichever you
> > > Caroline> prefer.
> > >
> > > I think it's better for patches to be reasonably self-contained when
> > > possible.
> > >
> > > Also, there were other patches for DWARFv5 on the list in "recent"
> (this
> > > year) times.  I wonder if those are relevant / related.
> > >
> > > Tom
>
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix issues with reading rnglists, especially from dwo files, for DWARF v5

Tom Tromey-2
In reply to this post by Sourceware - gdb-patches mailing list
>>>>> "Caroline" == Caroline Tice via Gdb-patches <[hidden email]> writes:

Thanks for the patch.

Caroline> +          /* if (dwarf2_attr_no_follow (comp_unit_die, DW_AT_ranges)) */

No need for commented-out code.

Caroline> +          if (comp_unit_die->attr (DW_AT_ranges) != nullptr)
Caroline> +            {
Caroline> +              cu->cu_ranges_from_skeleton = true;
Caroline> +            }

You can remove the braces here.  Normally in gdb there are only braces
if there are multiple statements (or a comment).

Caroline> +  if (tag == DW_TAG_compile_unit &&
Caroline> +      cu->cu_ranges_from_skeleton)

The GNU / gdb style is to break before operators, not after (including
assignment FWIW).  There are several occurrences of this, I didn't mark
them all.

Caroline> +  if (section == nullptr)
Caroline> +    error(_("Cannot find .debug_rnglists section [in module %s]"),

Space before "(".

Caroline> diff --git a/testsuite/gdb.dwarf2/dw5-rnglist-test.cc b/testsuite/gdb.dwarf2/dw5-rnglist-test.cc
Caroline> new file mode 100644
Caroline> index 0000000..4d650e1279
Caroline> --- /dev/null
Caroline> +++ b/testsuite/gdb.dwarf2/dw5-rnglist-test.cc
Caroline> @@ -0,0 +1,71 @@
Caroline> +#include <iostream>

Tests also need the copyright header.

Caroline> diff --git a/testsuite/gdb.dwarf2/dw5-rnglist-test.exp b/testsuite/gdb.dwarf2/dw5-rnglist-test.exp
Caroline> new file mode 100644
Caroline> index 0000000..b5c6c3bfe3
Caroline> --- /dev/null
Caroline> +++ b/testsuite/gdb.dwarf2/dw5-rnglist-test.exp
Caroline> @@ -0,0 +1,40 @@
Caroline> +# Copyright 2011-2020 Free Software Foundation, Inc.

This should probably just be 2020.

Tom
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix issues with reading rnglists, especially from dwo files, for DWARF v5

Sourceware - gdb-patches mailing list
On Thu, Jun 18, 2020 at 1:27 PM Tom Tromey <[hidden email]> wrote:

> >>>>> "Caroline" == Caroline Tice via Gdb-patches <
> [hidden email]> writes:
>
> Thanks for the patch.
>
> Caroline> +          /* if (dwarf2_attr_no_follow (comp_unit_die,
> DW_AT_ranges)) */
>
> No need for commented-out code.
>
> Done.

> Caroline> +          if (comp_unit_die->attr (DW_AT_ranges) != nullptr)
> Caroline> +            {
> Caroline> +              cu->cu_ranges_from_skeleton = true;
> Caroline> +            }
>
> You can remove the braces here.  Normally in gdb there are only braces
> if there are multiple statements (or a comment).
>
> Done.


> Caroline> +  if (tag == DW_TAG_compile_unit &&
> Caroline> +      cu->cu_ranges_from_skeleton)
>
> The GNU / gdb style is to break before operators, not after (including
> assignment FWIW).  There are several occurrences of this, I didn't mark
> them all.
>
> Done.


> Caroline> +  if (section == nullptr)
> Caroline> +    error(_("Cannot find .debug_rnglists section [in module
> %s]"),
>
> Space before "(".
>
>
Done.

> Caroline> diff --git a/testsuite/gdb.dwarf2/dw5-rnglist-test.cc
> b/testsuite/gdb.dwarf2/dw5-rnglist-test.cc
> Caroline> new file mode 100644
> Caroline> index 0000000..4d650e1279
> Caroline> --- /dev/null
> Caroline> +++ b/testsuite/gdb.dwarf2/dw5-rnglist-test.cc
> Caroline> @@ -0,0 +1,71 @@
> Caroline> +#include <iostream>
>
> Tests also need the copyright header.
>
> Done.


> Caroline> diff --git a/testsuite/gdb.dwarf2/dw5-rnglist-test.exp
> b/testsuite/gdb.dwarf2/dw5-rnglist-test.exp
> Caroline> new file mode 100644
> Caroline> index 0000000..b5c6c3bfe3
> Caroline> --- /dev/null
> Caroline> +++ b/testsuite/gdb.dwarf2/dw5-rnglist-test.exp
> Caroline> @@ -0,0 +1,40 @@
> Caroline> +# Copyright 2011-2020 Free Software Foundation, Inc.
>
> This should probably just be 2020.
>
>
Done.

> Tom
>

I believe I have addressed all of your comments.  The updated patch is
attached. Please let me know if this is ok now.  Thanks!

-- Caroline Tice
[hidden email]

gdb-dwo-rnglists.updated2.patch (27K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix issues with reading rnglists, especially from dwo files, for DWARF v5

Sourceware - gdb-patches mailing list
Ping?

-- Caroline
[hidden email]

On Tue, Jun 23, 2020 at 12:04 PM Caroline Tice <[hidden email]> wrote:

>
>
>
> On Thu, Jun 18, 2020 at 1:27 PM Tom Tromey <[hidden email]> wrote:
>>
>> >>>>> "Caroline" == Caroline Tice via Gdb-patches <[hidden email]> writes:
>>
>> Thanks for the patch.
>>
>> Caroline> +          /* if (dwarf2_attr_no_follow (comp_unit_die, DW_AT_ranges)) */
>>
>> No need for commented-out code.
>>
> Done.
>
>>
>> Caroline> +          if (comp_unit_die->attr (DW_AT_ranges) != nullptr)
>> Caroline> +            {
>> Caroline> +              cu->cu_ranges_from_skeleton = true;
>> Caroline> +            }
>>
>> You can remove the braces here.  Normally in gdb there are only braces
>> if there are multiple statements (or a comment).
>>
> Done.
>
>>
>> Caroline> +  if (tag == DW_TAG_compile_unit &&
>> Caroline> +      cu->cu_ranges_from_skeleton)
>>
>> The GNU / gdb style is to break before operators, not after (including
>> assignment FWIW).  There are several occurrences of this, I didn't mark
>> them all.
>>
> Done.
>
>>
>> Caroline> +  if (section == nullptr)
>> Caroline> +    error(_("Cannot find .debug_rnglists section [in module %s]"),
>>
>> Space before "(".
>>
>
> Done.
>>
>> Caroline> diff --git a/testsuite/gdb.dwarf2/dw5-rnglist-test.cc b/testsuite/gdb.dwarf2/dw5-rnglist-test.cc
>> Caroline> new file mode 100644
>> Caroline> index 0000000..4d650e1279
>> Caroline> --- /dev/null
>> Caroline> +++ b/testsuite/gdb.dwarf2/dw5-rnglist-test.cc
>> Caroline> @@ -0,0 +1,71 @@
>> Caroline> +#include <iostream>
>>
>> Tests also need the copyright header.
>>
> Done.
>
>
>>
>> Caroline> diff --git a/testsuite/gdb.dwarf2/dw5-rnglist-test.exp b/testsuite/gdb.dwarf2/dw5-rnglist-test.exp
>> Caroline> new file mode 100644
>> Caroline> index 0000000..b5c6c3bfe3
>> Caroline> --- /dev/null
>> Caroline> +++ b/testsuite/gdb.dwarf2/dw5-rnglist-test.exp
>> Caroline> @@ -0,0 +1,40 @@
>> Caroline> +# Copyright 2011-2020 Free Software Foundation, Inc.
>>
>> This should probably just be 2020.
>>
>
> Done.
>>
>> Tom
>
>
> I believe I have addressed all of your comments.  The updated patch is attached. Please let me know if this is ok now.  Thanks!
>
> -- Caroline Tice
> [hidden email]
>
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix issues with reading rnglists, especially from dwo files, for DWARF v5

Simon Marchi-4
In reply to this post by Sourceware - gdb-patches mailing list
On 2020-06-23 3:04 p.m., Caroline Tice via Gdb-patches wrote:
> I believe I have addressed all of your comments.  The updated patch is
> attached. Please let me know if this is ok now.  Thanks!

Hi Caroline,

Just to be sure, could you please send the complete patch, including the commit message (using
either git-format-patch or git-send-email)?  It would be nice to see the patch as it will be
merged, which includes the commit message.

Simon
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix issues with reading rnglists, especially from dwo files, for DWARF v5

Simon Marchi-4
On 2020-06-30 8:34 p.m., Simon Marchi wrote:

> On 2020-06-23 3:04 p.m., Caroline Tice via Gdb-patches wrote:
>> I believe I have addressed all of your comments.  The updated patch is
>> attached. Please let me know if this is ok now.  Thanks!
>
> Hi Caroline,
>
> Just to be sure, could you please send the complete patch, including the commit message (using
> either git-format-patch or git-send-email)?  It would be nice to see the patch as it will be
> merged, which includes the commit message.
>
> Simon
>

Oh, and just a nit, git told me this when applying:

/home/simark/patches/gdb-dwo-rnglists.updated2.patch:188: space before tab in indent.
  default:
warning: 1 line adds whitespace errors.

Simon
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix issues with reading rnglists, especially from dwo files, for DWARF v5

Sourceware - gdb-patches mailing list
I created the attached patch with git format-patch; I've never used
that before, so I'm hoping I got it right. :-)

Please let me know  if there's anything else you need/want for approval.

-- Caroline
[hidden email]

gdb/ChangeLog:

2020-07-01  Caroline Tice  <[hidden email]>

        * dwarf2/read.c (struct dwop_section_names): Add rnglists_dwo.
        (dwop_section_names): Add .debug_rnglists.dwo, .zdebug_rnglists.dwo.
        (struct dwarf2_cu): Add cu_ranges_from_skeleton field.
        (struct dwo_sections): Add rnglists field.
        (dwarf2_ranges_read): Add tag parameter.
        (cu_debug_rnglist_section): New function (decl & definition).
        (cutu_reader::cutu_reader): Before replacing the skeleton unit
        comp_unit_die with the dwo comp_unit_die, check to see if the skeleton
        unit die has a DW_AT_ranges, and if so set the cu_ranges_from_skeleton
        field in the cu.
        (dwarf2_locate_dwo_sections): Add code to read rnglists_dwo section.
        (dwarf2_rnglists_process): Add a dwarf_tag parameter, for the kind of
        die whose range is being checked; add code to read the rnglist section
        from the dwo file rather than from the main objfile, if appropriate.
        Add cases for DW_RLE_base_addressx,
        DW_RLE_startx_length, DW_RLE_startx_endx.  Also, update to only add
        the base address to DW_RLE_offset_pairs (not to all ranges).
        (dwarf2_ranges_process): Add dwarf tag parameter and pass it to
        dwarf2_rnglists_process.
        (dwarf2_ranges_read): Add dwarf tag parameter and pass it to
        dwarf2_ranges_process.
        (dwarf2_get_pc_bounds): Check for DW_FORM_rnglistx when setting
        need_ranges_base.  Also pass die tag to dwarf2_ranges_read.
        (dwarf2_record_block_ranges): Check for DW_FORM_rnglistx when setting
        need_ranges_base.  Also pass die tag to dwarf2_ranges_read.
        (read_full_die_1): Add code to read DW_AT_rnglists_base and assign to
        cu->ranges_base.
        (partial_die_info::read): Check for DW_FORM_rnglistx when setting
        need_ranges_base.  Also pass die tag to dwarf2_ranges_read.
        (read_rnglist_index): New function.
        (read_attribute_reprocess):  Add code for DW_FORM_rnglistx.
        (read_attribute_value): Mark DW_FORM_rnglistx with need_reprocess.

gdb/testsuite/ChangeLog entry:

2020-07-01  Caroline Tice  <[hidden email]>
        * gdb.dwarf2/dw5-rnglist-test.cc: New file.
        * gdb.dwarf2/dw5-rnglist-test.exp: New file.



On Tue, Jun 30, 2020 at 5:36 PM Simon Marchi <[hidden email]> wrote:

>
> On 2020-06-30 8:34 p.m., Simon Marchi wrote:
> > On 2020-06-23 3:04 p.m., Caroline Tice via Gdb-patches wrote:
> >> I believe I have addressed all of your comments.  The updated patch is
> >> attached. Please let me know if this is ok now.  Thanks!
> >
> > Hi Caroline,
> >
> > Just to be sure, could you please send the complete patch, including the commit message (using
> > either git-format-patch or git-send-email)?  It would be nice to see the patch as it will be
> > merged, which includes the commit message.
> >
> > Simon
> >
>
> Oh, and just a nit, git told me this when applying:
>
> /home/simark/patches/gdb-dwo-rnglists.updated2.patch:188: space before tab in indent.
>         default:
> warning: 1 line adds whitespace errors.
>
> Simon

v2-0001-Update-GDB-to-fix-issues-with-handling-DWARF-v5-r.patch (29K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix issues with reading rnglists, especially from dwo files, for DWARF v5

Simon Marchi-4
On 2020-07-01 3:57 p.m., Caroline Tice via Gdb-patches wrote:
> I created the attached patch with git format-patch; I've never used
> that before, so I'm hoping I got it right. :-)

Very nice, thanks.

> Please let me know  if there's anything else you need/want for approval.

Thanks for that.  I'd appreciate if the commit message contained a bit more
of the rationale for the change (in addition to what you have already put
in there).  Basically, what you explained in your first message.

> @@ -1379,11 +1385,16 @@ static void read_call_site_scope (struct die_info *die, struct dwarf2_cu *cu);
>  static void read_variable (struct die_info *die, struct dwarf2_cu *cu);
>
>  static int dwarf2_ranges_read (unsigned, CORE_ADDR *, CORE_ADDR *,
> -       struct dwarf2_cu *, dwarf2_psymtab *);
> +       struct dwarf2_cu *, dwarf2_psymtab *,
> +       dwarf_tag);
>
>  /* Return the .debug_loclists section to use for cu.  */
>  static struct dwarf2_section_info *cu_debug_loc_section (struct dwarf2_cu *cu);
>
> +/* Return the .debug_rnglists section to use for cu.  */
> +static struct dwarf2_section_info *cu_debug_rnglist_section
> +  (struct dwarf2_cu *cu);

Nit, this function should probably be called cu_debug_rnglists_section (with an `s`
to rnglists).

> @@ -13802,17 +13820,36 @@ dwarf2_rnglists_process (unsigned offset, struct dwarf2_cu *cu,
>    const gdb_byte *buffer;
>    CORE_ADDR baseaddr;
>    bool overflow = false;
> +  ULONGEST addr_index;
> +  bool ignore_dwo_unit = false;
> +  struct dwarf2_section_info *rnglists_section;
>
>    base = cu->base_address;
>
> -  per_objfile->per_bfd->rnglists.read (objfile);
> -  if (offset >= per_objfile->per_bfd->rnglists.size)
> +  /* If the DW_AT_ranges attribute was part of a DW_TAG_skeleton_unit that was
> +     changed into a DW_TAG_compile_unit after calling read_cutu_die_from_dwo,
> +     we want to use the rnglist in the objfile rather than the one in the
> +     dwo_unit. */

I think that can be explained more simply as: we want to use the .debug_rnglists
section from the file the DW_AT_ranges attribute we're reading came from.  If it's
on DW_TAG_compile_unit, it was actually on the DW_TAG_skeleton_unit DIE, in the
objfile / linked program.  Otherwise, it was on some other DIE in the dwo.

> +  if (tag == DW_TAG_compile_unit
> +      && cu->cu_ranges_from_skeleton)

I'm wondering if this `cu_ranges_from_skeleton` boolean is really necessary.  There
cannot be a DW_AT_ranges attribute on a DW_TAG_compile_unit inside a dwo file (see
DWARF 5, section 3.1.3).  So if we're reading a DW_AT_ranges on a DW_TAG_compile_unit,
it's either because:

- it's a standard non-split DW_TAG_compile_unit
- it's a DW_TAG_skeleton_unit that was transformed into a DW_TAG_compile_unit by
  read_cutu_die_from_dwo, as you said (I didn't know it did that)

In both of these cases, the rnglists section to use should be the one from the objfile,
not the one from the dwo.  The DW_TAG_skeleton_unit in the main objfile can have
a DW_AT_ranges attribute, while the associated DW_TAG_compile_unit in the dwo can't.

So the condition could probably just be:

  if (cu->dwo_unit != nullptr && tag != DW_TAG_compile_unit)
    rnglists_section = &cu->dwo_unit->dwo_file->sections.rnglists;
  else
    rnglists_section = &per_objfile->per_bfd->rnglists;

If there's a dwo_unit, the DW_TAG_compile_unit (transformed from DW_TAG_skeleton_unit)
will be the only DIE in the CU, so the only one to use the rnglists from the main objfile.
All the conceptual children DIEs contained in the dwo will use the rnglists from the dwo.

> +    ignore_dwo_unit = true;
> +
> +  /* Else, if there's a dwo_unit, we want the rnglist from the dwo file.  */
> +  if (cu->dwo_unit
> +      && cu->dwo_unit->dwo_file->sections.rnglists.size > 0
> +      && !ignore_dwo_unit)
> +      rnglists_section = &cu->dwo_unit->dwo_file->sections.rnglists;
> +  else
> +      rnglists_section = &per_objfile->per_bfd->rnglists;

I'm not sure the `cu->dwo_unit->dwo_file->sections.rnglists.size > 0` part of the condition
is right.  Let's say you have some DIE inside the dwo with a DW_AT_ranges attriute, but the
rnglists section from the dwo is either missing or empty.  That code will end up reading the
rnglists from the objfile, which is wrong: a DW_AT_ranges in a dwo always refers to the
rnglists section of the dwo, right?

It would be a good thing to test for fun, strip the .debug_rnglists.dwo section from the dwo
and see what happens.

> @@ -13889,7 +13944,20 @@ dwarf2_rnglists_process (unsigned offset, struct dwarf2_cu *cu,
>    range_end = cu->header.read_address (obfd, buffer, &bytes_read);
>    buffer += bytes_read;
>    break;
> - default:
> + case DW_RLE_startx_endx:
> +          addr_index = read_unsigned_leb128 (obfd, buffer, &bytes_read);
> +          buffer += bytes_read;
> +          range_beginning = read_addr_index (cu, addr_index);
> +          if (buffer > buf_end)
> +            {
> +              overflow = true;
> +              break;
> +            }
> +          addr_index = read_unsigned_leb128 (obfd, buffer, &bytes_read);
> +          buffer += bytes_read;
> +          range_end = read_addr_index (cu, addr_index);
> +          break;
> + default:

Remove the space at the beginning of the line (before the tab).

>    complaint (_("Invalid .debug_rnglists data (no base address)"));
>    return false;
>   }
> @@ -13917,8 +13985,12 @@ dwarf2_rnglists_process (unsigned offset, struct dwarf2_cu *cu,
>        if (range_beginning == range_end)
>   continue;
>
> -      range_beginning += *base;
> -      range_end += *base;
> +      /* Only DW_RLE_offset_pair needs the base address added.  */
> +      if (rlet == DW_RLE_offset_pair)
> + {
> +  range_beginning += *base;
> +  range_end += *base;
> + }

That makes sense.  The DWARF standard says, about DW_RLE_base_address:

    ...that indicates the applicable base address used by following
    DW_RLE_offset_pair entries.

However, see this check a few lines above:

      if (!base.has_value ())
        {
          /* We have no valid base address for the ranges
             data.  */
          complaint (_("Invalid .debug_rnglists data (no base address)"));
          return false;
        }

Should that be moved inside the `if (rlet == DW_RLE_offset_pair)`?  It
only matters that we don't have a base if we're going to use it, and that's
if `rlet == DW_RLE_offset_pair`.

> @@ -18684,7 +18764,8 @@ partial_die_info::read (const struct die_reader_specs *reader,
>      /* It would be nice to reuse dwarf2_get_pc_bounds here,
>         but that requires a full DIE, so instead we just
>         reimplement it.  */
> -    int need_ranges_base = tag != DW_TAG_compile_unit;
> +    int need_ranges_base = (tag != DW_TAG_compile_unit
> +    && attr.form != DW_FORM_rnglistx);

It looks like this fix is unrelated from the rest of the patch, which deals
about reading rnglists from dwo files.  It's better to have on fix per patch.
So I think this fix would deserve its own patch, with a commit message that
clearly explains the issue and the fix.

I'm also not sure I understand it: when the form is DW_FORM_rnglistx, don't
we want to sometimes apply the base coming from DW_AT_rnglists_base?

> @@ -19094,6 +19175,49 @@ read_loclist_index (struct dwarf2_cu *cu, ULONGEST loclist_index)
>      return bfd_get_64 (abfd, info_ptr) + loclist_base;
>  }
>
> +/* Given a DW_FORM_rnglistx value rnglist_index, fetch the offset from the
> +   array of offsets in the .debug_rnglists section.  */
> +static CORE_ADDR
> +read_rnglist_index (struct dwarf2_cu *cu, ULONGEST rnglist_index)
> +{
> +  struct dwarf2_per_objfile *dwarf2_per_objfile = cu->per_objfile;
> +  struct objfile *objfile = dwarf2_per_objfile->objfile;
> +  bfd *abfd = objfile->obfd;
> +  ULONGEST rnglist_header_size =
> +    (cu->header.initial_length_size == 4 ? LOCLIST_HEADER_SIZE32
> +     : LOCLIST_HEADER_SIZE64);
> +  ULONGEST rnglist_base =
> +      (cu->dwo_unit) ? rnglist_header_size : cu->ranges_base;
> +
> +  struct dwarf2_section_info *section = cu_debug_rnglist_section (cu);
> +  if (section == nullptr)
> +    error (_("Cannot find .debug_rnglists section [in module %s]"),
> +   objfile_name(objfile));
> +  section->read (objfile);
> +  if (section->buffer == NULL)
> +    error (_("DW_FORM_rnglistx used without .debug_rnglists section "
> +     "[in module %s]"),
> +       objfile_name (objfile));
> +  struct loclist_header header;
> +  read_loclist_header (&header, section);
> +  if (rnglist_index >= header.offset_entry_count)
> +    error (_("DW_FORM_rnglistx index pointing outside of "
> +     ".debug_rnglists offset array [in module %s]"),
> +    objfile_name(objfile));
> +  if (rnglist_base + rnglist_index * cu->header.offset_size
> +        >= section->size)
> +    error (_("DW_FORM_rnglistx pointing outside of "
> +             ".debug_rnglists section [in module %s]"),
> +          objfile_name(objfile));
> +  const gdb_byte *info_ptr
> +    = section->buffer + rnglist_base + rnglist_index * cu->header.offset_size;

Factor out `rnglist_base + rnglist_index * cu->header.offset_size` into
an `offset` variable.

> +
> +  if (cu->header.offset_size == 4)
> +    return read_4_bytes (abfd, info_ptr) + rnglist_base;
> +  else
> +    return read_8_bytes (abfd, info_ptr) + rnglist_base;

Please use some empty lines in the code to separate logical block.  I find
it really hard to follow, packed like this.  At the very least, after the
`if` statements, put an empty line.  Comments that indicate what each block
does at high level are also helpful:

 +  /* Get rnglists section.  */
 +  struct dwarf2_section_info *section = cu_debug_rnglist_section (cu);
 +  if (section == nullptr)
 +    error (_("Cannot find .debug_rnglists section [in module %s]"),
 +   objfile_name(objfile));
 +
 +  /* Read rnglists section content.  */
 +  section->read (objfile);
 +  if (section->buffer == NULL)
 +    error (_("DW_FORM_rnglistx used without .debug_rnglists section "
 +     "[in module %s]"),
 +       objfile_name (objfile));
 +
 +  /* Validate that the index is with the offset list range. */
 +  struct loclist_header header;
 +  read_loclist_header (&header, section);
 +  if (rnglist_index >= header.offset_entry_count)
 +    error (_("DW_FORM_rnglistx index pointing outside of "
 +     ".debug_rnglists offset array [in module %s]"),
 +    objfile_name(objfile));
 +
 +  /* Validate that the offset is in the section's range.  */
 +  if (rnglist_base + rnglist_index * cu->header.offset_size
 +        >= section->size)
 +    error (_("DW_FORM_rnglistx pointing outside of "
 +             ".debug_rnglists section [in module %s]"),
 +          objfile_name(objfile));
 +
 +  const gdb_byte *info_ptr
 +    = section->buffer + rnglist_base + rnglist_index * cu->header.offset_size;


> @@ -23382,6 +23511,25 @@ cu_debug_loc_section (struct dwarf2_cu *cu)
>    : &per_objfile->per_bfd->loc);
>  }
>
> +/* Return the .debug_rnglists section to use for CU.  */
> +static struct dwarf2_section_info *
> +cu_debug_rnglist_section (struct dwarf2_cu *cu)
> +{
> +  if (cu->header.version < 5)
> +    error (_(".debug_rnglists section cannot be used in DWARF %d"),
> +   cu->header.version);
> +  struct dwarf2_per_objfile *dwarf2_per_objfile = cu->per_objfile;
> +
> +  if (cu->dwo_unit)

When checking pointers, use an explicit ` == nullptr` / ` != nullptr`.

> +    {
> +      struct dwo_sections *sections = &cu->dwo_unit->dwo_file->sections;
> +
> +      if (sections->rnglists.size > 0)
> + return &sections->rnglists;
> +    }
> +  return &dwarf2_per_objfile->per_bfd->rnglists;

This function is called to get the right rnglists section in the
read_rnglist_index function.  I don't understand how this can work.
When there is a dwo, the read_rnglist_index can be used to read a
range list either for the main file (the DW_AT_ranges on the
DW_TAG_skeleton_unit) or for the dwo file (the DW_AT_ranges on a
DW_TAG_lexical_block).

This function here always return the dwo section if a dwo is
present... which would be wrong for when reading the DW_AT_ranges
on the DW_TAG_skeleton_unit?

Ok, I debugged GDB to see when this gets called.  It works because
when reading the DW_TAG_skeleton_unit, `cu->dwo_unit` is still
NULL... so the first time cu_debug_rnglist_section gets called, it
returns the section in the main file, and the following times, it
returns the section in the dwo file.  That seems a bit fragile to
me.  For example, if the order of things change, like if we change
GDB to read the ranges lazily later, and `cu->dwo_unit` is not NULL
anymore when reading the DW_AT_ranges on DW_TAG_skeleton_unit, this
will return the wrong section.

Perhaps a more robust way to decide which section to return would
be to use the same `cu->dwo_unit != nullptr && tag != DW_TAG_compile_unit`
trick from earlier?  In fact, perhaps that other spot should call
cu_debug_rnglist_section instead of replicating the logic?

>  static void
> diff --git a/gdb/testsuite/gdb.dwarf2/dw5-rnglist-test.cc b/gdb/testsuite/gdb.dwarf2/dw5-rnglist-test.cc
> new file mode 100644
> index 0000000000..17f78843d2
> --- /dev/null
> +++ b/gdb/testsuite/gdb.dwarf2/dw5-rnglist-test.cc
> @@ -0,0 +1,88 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> +   Copyright 2020 Free Software Foundation, Inc.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> +
> +#include <iostream>
> +#include <vector>
> +
> +struct node {
> +  int id;
> +  node *left;
> +  node *right;
> +  bool visited;
> +};
> +
> +node  node_array[50];
> +unsigned int CUR_IDX = 0;
> +
> +node * make_node (int val)
> +{
> +  node *new_node = &(node_array[CUR_IDX++]);
> +  new_node->left = NULL;
> +  new_node->right = NULL;
> +  new_node->id = val;
> +  new_node->visited = false;
> +
> +  return new_node;
> +}
> +
> +void tree_insert (node *root, int val)
> +{
> +  if (val < root->id)
> +    {
> +      if (root->left)
> +        tree_insert (root->left, val);
> +      else
> +        root->left = make_node(val);
> +    }
> +  else if (val > root->id)
> +    {
> +      if (root->right)
> +        tree_insert (root->right, val);
> +      else
> +        root->right = make_node(val);
> +    }
> +}
> +
> +void inorder(node *root) {
> +  std::vector<node *> todo;
> +  todo.push_back(root);
> +  while (!todo.empty()){
> +    node *curr = todo.back();
> +    todo.pop_back(); /* break-here */
> +    if (curr->visited) {
> +      std::cout << curr->id << " ";
> +    } else {
> +      curr->visited = true;
> +      if (curr->right) { todo.push_back(curr->right); }
> +      todo.push_back(curr);
> +      if (curr->left) { todo.push_back(curr->left); }
> +    }
> +  }
> +}
> +
> +int main (int argc, char **argv)
> +{
> +  node *root = make_node (35);
> +
> +  tree_insert (root, 28);
> +  tree_insert (root, 20);
> +  tree_insert (root, 60);
> +
> +  inorder (root);
> +
> +  return 0;
> +}

We try to format the test cases using the same style as the rest of the
source code.  Could you please update this to follow the same rules?

Simon
Reply | Threaded
Open this post in threaded view
|

[PATCH V3] Fix issues with reading rnglists, especially from dwo files, for DWARF v5

Sourceware - gdb-patches mailing list
"

I have made most of your requested changes, which are in the attached
patch.  I disagree with one of your comments, and I had a question
about another:

First the disagreement.  You said:

>> @@ -18684,7 +18764,8 @@ partial_die_info::read (const struct die_reader_specs *reader,
>>           /* It would be nice to reuse dwarf2_get_pc_bounds here,
>>              but that requires a full DIE, so instead we just
>>              reimplement it.  */
>> -         int need_ranges_base = tag != DW_TAG_compile_unit;
>> +         int need_ranges_base = (tag != DW_TAG_compile_unit
>> +                                 && attr.form != DW_FORM_rnglistx);
>
> It looks like this fix is unrelated from the rest of the patch, which deals
> about reading rnglists from dwo files.  It's better to have on fix per patch.
> So I think this fix would deserve its own patch, with a commit message that
> clearly explains the issue and the fix.
>
This is actually a required fix for dealing with rnglists.  Without
it, when the DW_AT_ranges uses the form
DW_FORM_rnglistx, the wrong value is calculated and the entire rnglist
is ignored (if you turn on complaints this will happen).  So without
this fix, I can't test any of the other code for fixing rnglists, with
or without .dwo files.

> I'm also not sure I understand it: when the form is DW_FORM_rnglistx, don't
> we want to sometimes apply the base coming from DW_AT_rnglists_base?

It all depends on what form is being used. If the DW_AT_ranges is
using DW_FORM_sec_offset (which is what
GCC generates), then you do need to add the base value.  But if you
are using the DW_FORM_rnglisx (which is what clang generates), the
base value has already been considered/incorporated into the attribute
so adding it in (again) causes the value to be incorrect (again, turn
on complaints and see what happens if you try to add the base into
something with DW_FORM_rnglistx).  From the DWARF5 spec: "[A] rnglist
is either represented as: An index into the .debug_rnglists section
(DW_FORM_rnglistx). The ...operand identifies an offset location
relative to the base of that section...[or] An offset into the
.debug_rnglists section (DW_FORM_sec_offset).  The operand consists of
a byte offset from the beginning of the .debug_rnglists section."
Note the DW_FORM_rnglistx is already relative to the base.

Now, my question:  I'm not quite clear as to whether you really want
me to change the code in
cu_debug_rnglists_section or not.   I wrote the code mostly by copying
the code for loclists as closely as possible, and making whatever
changes to that were required to make it correct for rnglists.  Is it
better to have the code more consistent with the loclists code?  If
you really want me to make the changes there you suggest, then I will;
I just wanted confirmation that you really want that change (i.e.
adding the dwarf tag and checking vs. DW_TAG_compile_unit, & calling
it from dwarf2_rnglists_process).

The test of your requested changes are in the attached patch.

-- Caroline
[hidden email]

2020-07-03  Caroline Tice  <[hidden email]>

        * dwarf2/read.c (struct dwop_section_names): Add rnglists_dwo.
        (dwop_section_names): Add .debug_rnglists.dwo, .zdebug_rnglists.dwo.
        (struct dwo_sections): Add rnglists field.
        (dwarf2_ranges_read): Add tag parameter.
        (cu_debug_rnglist_section): New function (decl & definition).
        (dwarf2_locate_dwo_sections): Add code to read rnglists_dwo section.
        (dwarf2_rnglists_process): Add a dwarf_tag parameter, for the kind of
        die whose range is being checked; add code to read the rnglist section
        from the dwo file rather than from the main objfile, if appropriate.
        Add cases for DW_RLE_base_addressx,
        DW_RLE_startx_length, DW_RLE_startx_endx.  Also, update to only add
        the base address to DW_RLE_offset_pairs (not to all ranges).
        (dwarf2_ranges_process): Add dwarf tag parameter and pass it to
        dwarf2_rnglists_process.
        (dwarf2_ranges_read): Add dwarf tag parameter and pass it to
        dwarf2_ranges_process.
        (dwarf2_get_pc_bounds): Check for DW_FORM_rnglistx when setting
        need_ranges_base.  Also pass die tag to dwarf2_ranges_read.
        (dwarf2_record_block_ranges): Check for DW_FORM_rnglistx when setting
        need_ranges_base.  Also pass die tag to dwarf2_ranges_read.
        (read_full_die_1): Add code to read DW_AT_rnglists_base and assign to
        cu->ranges_base.
        (partial_die_info::read): Check for DW_FORM_rnglistx when setting
        need_ranges_base.  Also pass die tag to dwarf2_ranges_read.
        (read_rnglist_index): New function.
        (read_attribute_reprocess):  Add code for DW_FORM_rnglistx.
        (read_attribute_value): Mark DW_FORM_rnglistx with need_reprocess.

gdb/testsuite/ChangeLog entry:

2020-07-03  Caroline Tice  <[hidden email]>

        * gdb.dwarf2/dw5-rnglist-test.cc: New file.
        * gdb.dwarf2/dw5-rnglist-test.exp: New file.


On Wed, Jul 1, 2020 at 10:41 PM Simon Marchi <[hidden email]> wrote:

>
> On 2020-07-01 3:57 p.m., Caroline Tice via Gdb-patches wrote:
> > I created the attached patch with git format-patch; I've never used
> > that before, so I'm hoping I got it right. :-)
>
> Very nice, thanks.
>
> > Please let me know  if there's anything else you need/want for approval.
>
> Thanks for that.  I'd appreciate if the commit message contained a bit more
> of the rationale for the change (in addition to what you have already put
> in there).  Basically, what you explained in your first message.
>
> > @@ -1379,11 +1385,16 @@ static void read_call_site_scope (struct die_info *die, struct dwarf2_cu *cu);
> >  static void read_variable (struct die_info *die, struct dwarf2_cu *cu);
> >
> >  static int dwarf2_ranges_read (unsigned, CORE_ADDR *, CORE_ADDR *,
> > -                            struct dwarf2_cu *, dwarf2_psymtab *);
> > +                            struct dwarf2_cu *, dwarf2_psymtab *,
> > +                            dwarf_tag);
> >
> >  /* Return the .debug_loclists section to use for cu.  */
> >  static struct dwarf2_section_info *cu_debug_loc_section (struct dwarf2_cu *cu);
> >
> > +/* Return the .debug_rnglists section to use for cu.  */
> > +static struct dwarf2_section_info *cu_debug_rnglist_section
> > +  (struct dwarf2_cu *cu);
>
> Nit, this function should probably be called cu_debug_rnglists_section (with an `s`
> to rnglists).
>
> > @@ -13802,17 +13820,36 @@ dwarf2_rnglists_process (unsigned offset, struct dwarf2_cu *cu,
> >    const gdb_byte *buffer;
> >    CORE_ADDR baseaddr;
> >    bool overflow = false;
> > +  ULONGEST addr_index;
> > +  bool ignore_dwo_unit = false;
> > +  struct dwarf2_section_info *rnglists_section;
> >
> >    base = cu->base_address;
> >
> > -  per_objfile->per_bfd->rnglists.read (objfile);
> > -  if (offset >= per_objfile->per_bfd->rnglists.size)
> > +  /* If the DW_AT_ranges attribute was part of a DW_TAG_skeleton_unit that was
> > +     changed into a DW_TAG_compile_unit after calling read_cutu_die_from_dwo,
> > +     we want to use the rnglist in the objfile rather than the one in the
> > +     dwo_unit. */
>
> I think that can be explained more simply as: we want to use the .debug_rnglists
> section from the file the DW_AT_ranges attribute we're reading came from.  If it's
> on DW_TAG_compile_unit, it was actually on the DW_TAG_skeleton_unit DIE, in the
> objfile / linked program.  Otherwise, it was on some other DIE in the dwo.
>
> > +  if (tag == DW_TAG_compile_unit
> > +      && cu->cu_ranges_from_skeleton)
>
> I'm wondering if this `cu_ranges_from_skeleton` boolean is really necessary.  There
> cannot be a DW_AT_ranges attribute on a DW_TAG_compile_unit inside a dwo file (see
> DWARF 5, section 3.1.3).  So if we're reading a DW_AT_ranges on a DW_TAG_compile_unit,
> it's either because:
>
> - it's a standard non-split DW_TAG_compile_unit
> - it's a DW_TAG_skeleton_unit that was transformed into a DW_TAG_compile_unit by
>   read_cutu_die_from_dwo, as you said (I didn't know it did that)
>
> In both of these cases, the rnglists section to use should be the one from the objfile,
> not the one from the dwo.  The DW_TAG_skeleton_unit in the main objfile can have
> a DW_AT_ranges attribute, while the associated DW_TAG_compile_unit in the dwo can't.
>
> So the condition could probably just be:
>
>   if (cu->dwo_unit != nullptr && tag != DW_TAG_compile_unit)
>     rnglists_section = &cu->dwo_unit->dwo_file->sections.rnglists;
>   else
>     rnglists_section = &per_objfile->per_bfd->rnglists;
>
> If there's a dwo_unit, the DW_TAG_compile_unit (transformed from DW_TAG_skeleton_unit)
> will be the only DIE in the CU, so the only one to use the rnglists from the main objfile.
> All the conceptual children DIEs contained in the dwo will use the rnglists from the dwo.
>
> > +    ignore_dwo_unit = true;
> > +
> > +  /* Else, if there's a dwo_unit, we want the rnglist from the dwo file.  */
> > +  if (cu->dwo_unit
> > +      && cu->dwo_unit->dwo_file->sections.rnglists.size > 0
> > +      && !ignore_dwo_unit)
> > +      rnglists_section = &cu->dwo_unit->dwo_file->sections.rnglists;
> > +  else
> > +      rnglists_section = &per_objfile->per_bfd->rnglists;
>
> I'm not sure the `cu->dwo_unit->dwo_file->sections.rnglists.size > 0` part of the condition
> is right.  Let's say you have some DIE inside the dwo with a DW_AT_ranges attriute, but the
> rnglists section from the dwo is either missing or empty.  That code will end up reading the
> rnglists from the objfile, which is wrong: a DW_AT_ranges in a dwo always refers to the
> rnglists section of the dwo, right?
>
> It would be a good thing to test for fun, strip the .debug_rnglists.dwo section from the dwo
> and see what happens.
>
> > @@ -13889,7 +13944,20 @@ dwarf2_rnglists_process (unsigned offset, struct dwarf2_cu *cu,
> >         range_end = cu->header.read_address (obfd, buffer, &bytes_read);
> >         buffer += bytes_read;
> >         break;
> > -     default:
> > +     case DW_RLE_startx_endx:
> > +          addr_index = read_unsigned_leb128 (obfd, buffer, &bytes_read);
> > +          buffer += bytes_read;
> > +          range_beginning = read_addr_index (cu, addr_index);
> > +          if (buffer > buf_end)
> > +            {
> > +              overflow = true;
> > +              break;
> > +            }
> > +          addr_index = read_unsigned_leb128 (obfd, buffer, &bytes_read);
> > +          buffer += bytes_read;
> > +          range_end = read_addr_index (cu, addr_index);
> > +          break;
> > +     default:
>
> Remove the space at the beginning of the line (before the tab).
>
> >         complaint (_("Invalid .debug_rnglists data (no base address)"));
> >         return false;
> >       }
> > @@ -13917,8 +13985,12 @@ dwarf2_rnglists_process (unsigned offset, struct dwarf2_cu *cu,
> >        if (range_beginning == range_end)
> >       continue;
> >
> > -      range_beginning += *base;
> > -      range_end += *base;
> > +      /* Only DW_RLE_offset_pair needs the base address added.  */
> > +      if (rlet == DW_RLE_offset_pair)
> > +     {
> > +       range_beginning += *base;
> > +       range_end += *base;
> > +     }
>
> That makes sense.  The DWARF standard says, about DW_RLE_base_address:
>
>     ...that indicates the applicable base address used by following
>     DW_RLE_offset_pair entries.
>
> However, see this check a few lines above:
>
>       if (!base.has_value ())
>         {
>           /* We have no valid base address for the ranges
>              data.  */
>           complaint (_("Invalid .debug_rnglists data (no base address)"));
>           return false;
>         }
>
> Should that be moved inside the `if (rlet == DW_RLE_offset_pair)`?  It
> only matters that we don't have a base if we're going to use it, and that's
> if `rlet == DW_RLE_offset_pair`.
>
> > @@ -18684,7 +18764,8 @@ partial_die_info::read (const struct die_reader_specs *reader,
> >           /* It would be nice to reuse dwarf2_get_pc_bounds here,
> >              but that requires a full DIE, so instead we just
> >              reimplement it.  */
> > -         int need_ranges_base = tag != DW_TAG_compile_unit;
> > +         int need_ranges_base = (tag != DW_TAG_compile_unit
> > +                                 && attr.form != DW_FORM_rnglistx);
>
> It looks like this fix is unrelated from the rest of the patch, which deals
> about reading rnglists from dwo files.  It's better to have on fix per patch.
> So I think this fix would deserve its own patch, with a commit message that
> clearly explains the issue and the fix.
>
> I'm also not sure I understand it: when the form is DW_FORM_rnglistx, don't
> we want to sometimes apply the base coming from DW_AT_rnglists_base?
>
> > @@ -19094,6 +19175,49 @@ read_loclist_index (struct dwarf2_cu *cu, ULONGEST loclist_index)
> >      return bfd_get_64 (abfd, info_ptr) + loclist_base;
> >  }
> >
> > +/* Given a DW_FORM_rnglistx value rnglist_index, fetch the offset from the
> > +   array of offsets in the .debug_rnglists section.  */
> > +static CORE_ADDR
> > +read_rnglist_index (struct dwarf2_cu *cu, ULONGEST rnglist_index)
> > +{
> > +  struct dwarf2_per_objfile *dwarf2_per_objfile = cu->per_objfile;
> > +  struct objfile *objfile = dwarf2_per_objfile->objfile;
> > +  bfd *abfd = objfile->obfd;
> > +  ULONGEST rnglist_header_size =
> > +    (cu->header.initial_length_size == 4 ? LOCLIST_HEADER_SIZE32
> > +     : LOCLIST_HEADER_SIZE64);
> > +  ULONGEST rnglist_base =
> > +      (cu->dwo_unit) ? rnglist_header_size : cu->ranges_base;
> > +
> > +  struct dwarf2_section_info *section = cu_debug_rnglist_section (cu);
> > +  if (section == nullptr)
> > +    error (_("Cannot find .debug_rnglists section [in module %s]"),
> > +        objfile_name(objfile));
> > +  section->read (objfile);
> > +  if (section->buffer == NULL)
> > +    error (_("DW_FORM_rnglistx used without .debug_rnglists section "
> > +          "[in module %s]"),
> > +       objfile_name (objfile));
> > +  struct loclist_header header;
> > +  read_loclist_header (&header, section);
> > +  if (rnglist_index >= header.offset_entry_count)
> > +    error (_("DW_FORM_rnglistx index pointing outside of "
> > +          ".debug_rnglists offset array [in module %s]"),
> > +         objfile_name(objfile));
> > +  if (rnglist_base + rnglist_index * cu->header.offset_size
> > +        >= section->size)
> > +    error (_("DW_FORM_rnglistx pointing outside of "
> > +             ".debug_rnglists section [in module %s]"),
> > +          objfile_name(objfile));
> > +  const gdb_byte *info_ptr
> > +    = section->buffer + rnglist_base + rnglist_index * cu->header.offset_size;
>
> Factor out `rnglist_base + rnglist_index * cu->header.offset_size` into
> an `offset` variable.
>
> > +
> > +  if (cu->header.offset_size == 4)
> > +    return read_4_bytes (abfd, info_ptr) + rnglist_base;
> > +  else
> > +    return read_8_bytes (abfd, info_ptr) + rnglist_base;
>
> Please use some empty lines in the code to separate logical block.  I find
> it really hard to follow, packed like this.  At the very least, after the
> `if` statements, put an empty line.  Comments that indicate what each block
> does at high level are also helpful:
>
>  +  /* Get rnglists section.  */
>  +  struct dwarf2_section_info *section = cu_debug_rnglist_section (cu);
>  +  if (section == nullptr)
>  +    error (_("Cannot find .debug_rnglists section [in module %s]"),
>  +         objfile_name(objfile));
>  +
>  +  /* Read rnglists section content.  */
>  +  section->read (objfile);
>  +  if (section->buffer == NULL)
>  +    error (_("DW_FORM_rnglistx used without .debug_rnglists section "
>  +           "[in module %s]"),
>  +       objfile_name (objfile));
>  +
>  +  /* Validate that the index is with the offset list range. */
>  +  struct loclist_header header;
>  +  read_loclist_header (&header, section);
>  +  if (rnglist_index >= header.offset_entry_count)
>  +    error (_("DW_FORM_rnglistx index pointing outside of "
>  +           ".debug_rnglists offset array [in module %s]"),
>  +          objfile_name(objfile));
>  +
>  +  /* Validate that the offset is in the section's range.  */
>  +  if (rnglist_base + rnglist_index * cu->header.offset_size
>  +        >= section->size)
>  +    error (_("DW_FORM_rnglistx pointing outside of "
>  +             ".debug_rnglists section [in module %s]"),
>  +          objfile_name(objfile));
>  +
>  +  const gdb_byte *info_ptr
>  +    = section->buffer + rnglist_base + rnglist_index * cu->header.offset_size;
>
>
> > @@ -23382,6 +23511,25 @@ cu_debug_loc_section (struct dwarf2_cu *cu)
> >                                 : &per_objfile->per_bfd->loc);
> >  }
> >
> > +/* Return the .debug_rnglists section to use for CU.  */
> > +static struct dwarf2_section_info *
> > +cu_debug_rnglist_section (struct dwarf2_cu *cu)
> > +{
> > +  if (cu->header.version < 5)
> > +    error (_(".debug_rnglists section cannot be used in DWARF %d"),
> > +        cu->header.version);
> > +  struct dwarf2_per_objfile *dwarf2_per_objfile = cu->per_objfile;
> > +
> > +  if (cu->dwo_unit)
>
> When checking pointers, use an explicit ` == nullptr` / ` != nullptr`.
>
> > +    {
> > +      struct dwo_sections *sections = &cu->dwo_unit->dwo_file->sections;
> > +
> > +      if (sections->rnglists.size > 0)
> > +     return &sections->rnglists;
> > +    }
> > +  return &dwarf2_per_objfile->per_bfd->rnglists;
>
> This function is called to get the right rnglists section in the
> read_rnglist_index function.  I don't understand how this can work.
> When there is a dwo, the read_rnglist_index can be used to read a
> range list either for the main file (the DW_AT_ranges on the
> DW_TAG_skeleton_unit) or for the dwo file (the DW_AT_ranges on a
> DW_TAG_lexical_block).
>
> This function here always return the dwo section if a dwo is
> present... which would be wrong for when reading the DW_AT_ranges
> on the DW_TAG_skeleton_unit?
>
> Ok, I debugged GDB to see when this gets called.  It works because
> when reading the DW_TAG_skeleton_unit, `cu->dwo_unit` is still
> NULL... so the first time cu_debug_rnglist_section gets called, it
> returns the section in the main file, and the following times, it
> returns the section in the dwo file.  That seems a bit fragile to
> me.  For example, if the order of things change, like if we change
> GDB to read the ranges lazily later, and `cu->dwo_unit` is not NULL
> anymore when reading the DW_AT_ranges on DW_TAG_skeleton_unit, this
> will return the wrong section.
>
> Perhaps a more robust way to decide which section to return would
> be to use the same `cu->dwo_unit != nullptr && tag != DW_TAG_compile_unit`
> trick from earlier?  In fact, perhaps that other spot should call
> cu_debug_rnglist_section instead of replicating the logic?
>
> >  static void
> > diff --git a/gdb/testsuite/gdb.dwarf2/dw5-rnglist-test.cc b/gdb/testsuite/gdb.dwarf2/dw5-rnglist-test.cc
> > new file mode 100644
> > index 0000000000..17f78843d2
> > --- /dev/null
> > +++ b/gdb/testsuite/gdb.dwarf2/dw5-rnglist-test.cc
> > @@ -0,0 +1,88 @@
> > +/* This testcase is part of GDB, the GNU debugger.
> > +
> > +   Copyright 2020 Free Software Foundation, Inc.
> > +
> > +   This program is free software; you can redistribute it and/or modify
> > +   it under the terms of the GNU General Public License as published by
> > +   the Free Software Foundation; either version 3 of the License, or
> > +   (at your option) any later version.
> > +
> > +   This program is distributed in the hope that it will be useful,
> > +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> > +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > +   GNU General Public License for more details.
> > +
> > +   You should have received a copy of the GNU General Public License
> > +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> > +
> > +#include <iostream>
> > +#include <vector>
> > +
> > +struct node {
> > +  int id;
> > +  node *left;
> > +  node *right;
> > +  bool visited;
> > +};
> > +
> > +node  node_array[50];
> > +unsigned int CUR_IDX = 0;
> > +
> > +node * make_node (int val)
> > +{
> > +  node *new_node = &(node_array[CUR_IDX++]);
> > +  new_node->left = NULL;
> > +  new_node->right = NULL;
> > +  new_node->id = val;
> > +  new_node->visited = false;
> > +
> > +  return new_node;
> > +}
> > +
> > +void tree_insert (node *root, int val)
> > +{
> > +  if (val < root->id)
> > +    {
> > +      if (root->left)
> > +        tree_insert (root->left, val);
> > +      else
> > +        root->left = make_node(val);
> > +    }
> > +  else if (val > root->id)
> > +    {
> > +      if (root->right)
> > +        tree_insert (root->right, val);
> > +      else
> > +        root->right = make_node(val);
> > +    }
> > +}
> > +
> > +void inorder(node *root) {
> > +  std::vector<node *> todo;
> > +  todo.push_back(root);
> > +  while (!todo.empty()){
> > +    node *curr = todo.back();
> > +    todo.pop_back(); /* break-here */
> > +    if (curr->visited) {
> > +      std::cout << curr->id << " ";
> > +    } else {
> > +      curr->visited = true;
> > +      if (curr->right) { todo.push_back(curr->right); }
> > +      todo.push_back(curr);
> > +      if (curr->left) { todo.push_back(curr->left); }
> > +    }
> > +  }
> > +}
> > +
> > +int main (int argc, char **argv)
> > +{
> > +  node *root = make_node (35);
> > +
> > +  tree_insert (root, 28);
> > +  tree_insert (root, 20);
> > +  tree_insert (root, 60);
> > +
> > +  inorder (root);
> > +
> > +  return 0;
> > +}
>
> We try to format the test cases using the same style as the rest of the
> source code.  Could you please update this to follow the same rules?
>
> Simon

v3-0001-Update-GDB-to-fix-issues-with-handling-DWARF-v5-r.patch (30K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH V3] Fix issues with reading rnglists, especially from dwo files, for DWARF v5

Simon Marchi-4
On 2020-07-03 6:47 p.m., Caroline Tice wrote:

> "
>
> I have made most of your requested changes, which are in the attached
> patch.  I disagree with one of your comments, and I had a question
> about another:
>
> First the disagreement.  You said:
>
>>> @@ -18684,7 +18764,8 @@ partial_die_info::read (const struct die_reader_specs *reader,
>>>           /* It would be nice to reuse dwarf2_get_pc_bounds here,
>>>              but that requires a full DIE, so instead we just
>>>              reimplement it.  */
>>> -         int need_ranges_base = tag != DW_TAG_compile_unit;
>>> +         int need_ranges_base = (tag != DW_TAG_compile_unit
>>> +                                 && attr.form != DW_FORM_rnglistx);
>>
>> It looks like this fix is unrelated from the rest of the patch, which deals
>> about reading rnglists from dwo files.  It's better to have on fix per patch.
>> So I think this fix would deserve its own patch, with a commit message that
>> clearly explains the issue and the fix.
>>
> This is actually a required fix for dealing with rnglists.  Without
> it, when the DW_AT_ranges uses the form
> DW_FORM_rnglistx, the wrong value is calculated and the entire rnglist
> is ignored (if you turn on complaints this will happen).  So without
> this fix, I can't test any of the other code for fixing rnglists, with
> or without .dwo files.

Ah ok, I must have understood it wrong then.  I find all of this really confusing.

I'll summarize what I think I understand, please let me know if I say anything wrong.
We are dealing with two similar ways of dealing with address ranges, one is the
pre-standard GNU version here, which was using DW_AT_GNU_ranges_base:

  https://gcc.gnu.org/wiki/DebugFission

and the version in DWARF 5.

In the pre-standard version, only one range section was present, and it was in the
main linked file.  DW_AT_ranges attributes in the dwo, the form DW_FORM_sec_offset,
are actually pointing at the section in the main file, at offset

  CU's DW_AT_GNU_ranges_base value (found in the skeleton, in the main file) + DIE's DW_AT_ranges

If there is a DW_AT_ranges attribute in the skeleton CU, in the main linked file,
then it is absolute, it is _not_ added to the DW_AT_GNU_ranges_base value.  Hence
the `needs_range_base` thingy.  The `ranges_offset` variable in this function is
the direct offset to the range list to read.

In DWARF 5, we have a ranges section in the main linked file and one in the dwo.  DW_AT_ranges
attributes refer to the ranges section of the file they are in.  The DW_AT_rnglists_base
points to the beginning of the range table for that CU.  The actual value of DW_AT_ranges
attribute (when using the DW_FORM_rnglistx form) is an index in the offset table.  However,
due to the reprocessing you added in this patch, the attribute value is now in fact the
offset with `DW_AT_rnglists_base` already added.  And _that's_ the reason why
DW_FORM_rnglistx attributes should not have the range base applied.

I think the comment above in the code is misleading and needs to be updated, this one:

          /* DW_AT_rnglists_base does not apply to DIEs from the DWO skeleton.
             We take advantage of the fact that DW_AT_ranges does not appear
             in DW_TAG_compile_unit of DWO files.  */

In commit 18a8505e38fc ("Dwarf 5: Handle debug_str_offsets and indexed attributes
that have base offsets."), the it was changed like so:

-         /* DW_AT_ranges_base does not apply to DIEs from the DWO skeleton.
+         /* DW_AT_rnglists_base does not apply to DIEs from the DWO skeleton.

I think it was a wrong change, because it mixes two different things.  DW_AT_rnglists_base
is a DWARF 5 thing, and the range base not applying to the values from the skeleton is a
pre-standard thing.  I think it should say something like:

          /* DW_AT_GNU_ranges_base does not apply to DIEs from the DWO skeleton.
             We take advantage of the fact that DW_AT_ranges does not appear
             in DW_TAG_compile_unit of DWO files.

             Attributes of form DW_AT_rnglistx have already had their value changed
             by read_rnglist_index and already include DW_AT_rnglists_base, so don't
             add the ranges base either.  */

>
>> I'm also not sure I understand it: when the form is DW_FORM_rnglistx, don't
>> we want to sometimes apply the base coming from DW_AT_rnglists_base?
>
> It all depends on what form is being used. If the DW_AT_ranges is
> using DW_FORM_sec_offset (which is what
> GCC generates), then you do need to add the base value.But if you
> are using the DW_FORM_rnglisx (which is what clang generates), the
> base value has already been considered/incorporated into the attribute
> so adding it in (again) causes the value to be incorrect (again, turn
> on complaints and see what happens if you try to add the base into
> something with DW_FORM_rnglistx).  From the DWARF5 spec: "[A] rnglist
> is either represented as: An index into the .debug_rnglists section
> (DW_FORM_rnglistx). The ...operand identifies an offset location
> relative to the base of that section...[or] An offset into the
> .debug_rnglists section (DW_FORM_sec_offset).  The operand consists of
> a byte offset from the beginning of the .debug_rnglists section."
> Note the DW_FORM_rnglistx is already relative to the base.

Ok.  What was not clear to me in the first pass was that read_rnglist_index
already has modified the attribute value to include DW_AT_rnglists_base.

Now, about values of class rnglist and form DW_FORM_sec_offset in DWARF 5.
The spec says this about them:

  - An offset into the .debug_rnglists section (DW_FORM_sec_offset).
  The operand consists of a byte offset from the beginning of the
  .debug_rnglists section. It is relocatable in a relocatable object file,
  and relocated in an executable or shared object file. In the 32-bit
  DWARF format, this offset is a 4-byte unsigned value; in the 64-bit
  DWARF format, it is an 8-byte unsigned value (see Section 7.4 on
  page 196).

Since it says that the value is already relocated in an executable, does it
mean we should not add the base for these, in DWARF 5?

> Now, my question:  I'm not quite clear as to whether you really want
> me to change the code in
> cu_debug_rnglists_section or not.   I wrote the code mostly by copying
> the code for loclists as closely as possible, and making whatever
> changes to that were required to make it correct for rnglists.  Is it
> better to have the code more consistent with the loclists code?If
> you really want me to make the changes there you suggest, then I will;

I think the situation is a bit different than for `cu_debug_loc_section`:
when you have a dwo, I don't think there is the possibility of having an
attribute referring to a location list in the skeleton.  So if you have a
dwo, you'll always want the loc section from the dwo.  So there, the pattern

  if (there is a dwo)
    return the dwo section;

  return the main file section;

makes more sense.  But in the case of range lists, we have attributes
in both the skeleton and the dwo that require range lists, and the right
section to return depends on where the attribute is.  That's why, when I
see this pattern:

  if (there is a dwo)
    return the dwo section;

  return the main file section;

that pattern, I find it looks wrong, because the right section to return
does not depend (only) on whether there is dwo.  As we saw, it only works
"by chance" because this is only called once for the skeleton (when
reading DW_AT_ranges) and the `cu->dwo_unit` pointer happens not to be set
up yet.  It is also one refactor away from breaking, if for some reason we
reorder some code and the `cu->dwo_unit` pointer gets initialized earlier.

> I just wanted confirmation that you really want that change (i.e.
> adding the dwarf tag and checking vs. DW_TAG_compile_unit, & calling
> it from dwarf2_rnglists_process).

I think that checking DW_TAG_compile_unit is still a bit of a hack - ideally,
the caller could just tell us if the DIE comes from a dwo or not.  But at
least it's logical and based on some DWARF knowledge.  It would certainly
require a comment to explain it, because it wouldn't be obvious.

> @@ -1379,11 +1382,16 @@ static void read_call_site_scope (struct die_info *die, struct dwarf2_cu *cu);
>  static void read_variable (struct die_info *die, struct dwarf2_cu *cu);
>
>  static int dwarf2_ranges_read (unsigned, CORE_ADDR *, CORE_ADDR *,
> -       struct dwarf2_cu *, dwarf2_psymtab *);
> +       struct dwarf2_cu *, dwarf2_psymtab *,
> +       dwarf_tag);

This declaration is unneeded and can simply be removed.  Could you push an
obvious patch that does this?.


> @@ -13917,8 +13969,20 @@ dwarf2_rnglists_process (unsigned offset, struct dwarf2_cu *cu,
>        if (range_beginning == range_end)
>   continue;
>
> -      range_beginning += *base;
> -      range_end += *base;
> +      /* Only DW_RLE_offset_pair needs the base address added.  */
> +      if (rlet == DW_RLE_offset_pair)
> + {
> +  if (!base.has_value ())
> +    {
> +      /* We have no valid base address for the ranges
> + data.  */
> +      complaint (_("Invalid .debug_rnglists data (no base address)"));

The comment fits on a single line.  But it can probably be changed to be more
specific to DW_RLE_offset_pair.  Maybe the complaint could be more specific too,
and mention DW_RLE_offset_pair.  It would be valid to have .debug_rnglists without
a base address (I think?), but here it's invalid because there is not base address
_and_ we have encountered a DW_RLE_offset_pair.

> @@ -19094,6 +19167,57 @@ read_loclist_index (struct dwarf2_cu *cu, ULONGEST loclist_index)
>      return bfd_get_64 (abfd, info_ptr) + loclist_base;
>  }
>
> +/* Given a DW_FORM_rnglistx value rnglist_index, fetch the offset from the
> +   array of offsets in the .debug_rnglists section.  */
> +static CORE_ADDR
> +read_rnglist_index (struct dwarf2_cu *cu, ULONGEST rnglist_index)
> +{
> +  struct dwarf2_per_objfile *dwarf2_per_objfile = cu->per_objfile;
> +  struct objfile *objfile = dwarf2_per_objfile->objfile;
> +  bfd *abfd = objfile->obfd;
> +  ULONGEST rnglist_header_size =
> +    (cu->header.initial_length_size == 4 ? LOCLIST_HEADER_SIZE32
> +     : LOCLIST_HEADER_SIZE64);

Should this really refer to loclist header sizes?  If they are the same
sizes as range list table headers, I suppose it's just a coincidence.

> +  ULONGEST rnglist_base =
> +      (cu->dwo_unit) ? rnglist_header_size : cu->ranges_base;

cu->dwo_unit != nullptr

> +  ULONGEST start_offset =
> +    rnglist_base + rnglist_index * cu->header.offset_size;
> +
> +  /* Get rnglists section.  */
> +  struct dwarf2_section_info *section = cu_debug_rnglists_section (cu);
> +  if (section == nullptr)
> +    error (_("Cannot find .debug_rnglists section [in module %s]"),
> +   objfile_name(objfile));

Space before paren.

> +
> +  /* Read the rnglists section content.  */
> +  section->read (objfile);
> +  if (section->buffer == NULL)

In new code, I suggest using nullptr instead of NULL, just for consistency.

> +    error (_("DW_FORM_rnglistx used without .debug_rnglists section "
> +     "[in module %s]"),
> +       objfile_name (objfile));
> +
> +  /* Verify the rnglist header is valid (same as loclist header).  */
> +  struct loclist_header header;
> +  read_loclist_header (&header, section);

Ok well, if we are going to take advantage that they are the same, the name
of the function and types should reflect it.  `read_loclist_header` should
become `read_loclists_rnglists_header` (using the plural to match the section
names).

> +  if (rnglist_index >= header.offset_entry_count)
> +    error (_("DW_FORM_rnglistx index pointing outside of "
> +     ".debug_rnglists offset array [in module %s]"),
> +    objfile_name(objfile));

Space before paren.

> +
> +  /* Validate that the offset is within the section's range.  */
> +  if (start_offset >= section->size)
> +    error (_("DW_FORM_rnglistx pointing outside of "
> +             ".debug_rnglists section [in module %s]"),
> +          objfile_name(objfile));
> +
> +  const gdb_byte *info_ptr = section->buffer + start_offset;
> +
> +  if (cu->header.offset_size == 4)
> +    return read_4_bytes (abfd, info_ptr) + rnglist_base;
> +  else
> +    return read_8_bytes (abfd, info_ptr) + rnglist_base;

An idea for a subsequent cleanup, we could have a `read_offset` function
that does

  if (cu->header.offset_size == 4)
    return read_4_bytes (abfd, info_ptr);
  else
    return read_8_bytes (abfd, info_ptr);

There are a few spots that could use it.

Simon
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH V4] Fix issues with reading rnglists, especially from dwo files, for DWARF v5

Sourceware - gdb-patches mailing list
I think your description of how ranges work is correct.  I've made all
your requested changes (I think).  Attached is the latest patch for
review. :-)

-- Caroline
[hidden email]

gdb/ChangeLog:

2020-07-09  Caroline Tice  <[hidden email]>

       * dwarf2/read.c (RNGLIST_HEADER_SIZE32) New constant definition.
        (RNGLIST_HEADER_SIZE64): New constant definition.
        (struct dwop_section_names): Add rnglists_dwo.
        (dwop_section_names): Add .debug_rnglists.dwo, .zdebug_rnglists.dwo.
        (struct dwo_sections): Add rnglists field.
        (read_attribut_reprocess): Add tag parameter.
        (dwarf2_ranges_read): Add tag parameter.
        (cu_debug_rnglists_section): New function (decl & definition).
        (dwarf2_locate_dwo_sections): Add code to read rnglists_dwo section.
        (dwarf2_rnglists_process): Add a dwarf_tag parameter, for the kind of
        die whose range is being checked; add code to read the rnglist section
        from the dwo file rather than from the main objfile, if appropriate.
        Add cases for DW_RLE_base_addressx,
        DW_RLE_startx_length, DW_RLE_startx_endx.  Also, update to only add
        the base address to DW_RLE_offset_pairs (not to all ranges), moving
        test inside if-condition and updating complaint message.
        (dwarf2_ranges_process): Add dwarf tag parameter and pass it to
        dwarf2_rnglists_process.
        (dwarf2_ranges_read): Add dwarf tag parameter and pass it to
        dwarf2_ranges_process.
        (dwarf2_get_pc_bounds): Check for DW_FORM_rnglistx when setting
        need_ranges_base and update comment appropriately.  Also pass die tag
        to dwarf2_ranges_read.
        (dwarf2_record_block_ranges): Check for DW_FORM_rnglistx when setting
        need_ranges_base and update comment appropriately.  Also pass die tag
        to dwarf2_ranges_read.
        (read_full_die_1): Add code to read DW_AT_rnglists_base and assign to
        cu->ranges_base.  Also pass die tag to read_attribute_reprocess.
        (partial_die_info::read): Check for DW_FORM_rnglistx when setting
        need_ranges_base and update comment appropriately.  Also pass die tag
        to read_attribute_reprocess and dwarf2_ranges_read.
        (read_loclist_header): Rename function to read_loclists_rnglists_header,
        and update function comment appropriately.
        (read_loclist_index): Call read_loclists_rnglists_header instead of
        read_loclist_header.
        (read_rnglist_index): New function.
        (read_attribute_reprocess):  Add tag parameter. Add code for
        DW_FORM_rnglistx, passing tag to read_rnglist_index.
        (read_attribute_value): Mark DW_FORM_rnglistx with need_reprocess.

gdb/testsuite/ChangeLog:

2020-07-09  Caroline Tice  <[hidden email]>

        * gdb.dwarf2/dw5-rnglist-test.cc: New file.
        * gdb.dwarf2/dw5-rnglist-test.exp: New file.

On Fri, Jul 3, 2020 at 10:11 PM Simon Marchi <[hidden email]> wrote:

>
> On 2020-07-03 6:47 p.m., Caroline Tice wrote:
> > "
> >
> > I have made most of your requested changes, which are in the attached
> > patch.  I disagree with one of your comments, and I had a question
> > about another:
> >
> > First the disagreement.  You said:
> >
> >>> @@ -18684,7 +18764,8 @@ partial_die_info::read (const struct die_reader_specs *reader,
> >>>           /* It would be nice to reuse dwarf2_get_pc_bounds here,
> >>>              but that requires a full DIE, so instead we just
> >>>              reimplement it.  */
> >>> -         int need_ranges_base = tag != DW_TAG_compile_unit;
> >>> +         int need_ranges_base = (tag != DW_TAG_compile_unit
> >>> +                                 && attr.form != DW_FORM_rnglistx);
> >>
> >> It looks like this fix is unrelated from the rest of the patch, which deals
> >> about reading rnglists from dwo files.  It's better to have on fix per patch.
> >> So I think this fix would deserve its own patch, with a commit message that
> >> clearly explains the issue and the fix.
> >>
> > This is actually a required fix for dealing with rnglists.  Without
> > it, when the DW_AT_ranges uses the form
> > DW_FORM_rnglistx, the wrong value is calculated and the entire rnglist
> > is ignored (if you turn on complaints this will happen).  So without
> > this fix, I can't test any of the other code for fixing rnglists, with
> > or without .dwo files.
>
> Ah ok, I must have understood it wrong then.  I find all of this really confusing.
>
> I'll summarize what I think I understand, please let me know if I say anything wrong.
> We are dealing with two similar ways of dealing with address ranges, one is the
> pre-standard GNU version here, which was using DW_AT_GNU_ranges_base:
>
>   https://gcc.gnu.org/wiki/DebugFission
>
> and the version in DWARF 5.
>
> In the pre-standard version, only one range section was present, and it was in the
> main linked file.  DW_AT_ranges attributes in the dwo, the form DW_FORM_sec_offset,
> are actually pointing at the section in the main file, at offset
>
>   CU's DW_AT_GNU_ranges_base value (found in the skeleton, in the main file) + DIE's DW_AT_ranges
>
> If there is a DW_AT_ranges attribute in the skeleton CU, in the main linked file,
> then it is absolute, it is _not_ added to the DW_AT_GNU_ranges_base value.  Hence
> the `needs_range_base` thingy.  The `ranges_offset` variable in this function is
> the direct offset to the range list to read.
>
> In DWARF 5, we have a ranges section in the main linked file and one in the dwo.  DW_AT_ranges
> attributes refer to the ranges section of the file they are in.  The DW_AT_rnglists_base
> points to the beginning of the range table for that CU.  The actual value of DW_AT_ranges
> attribute (when using the DW_FORM_rnglistx form) is an index in the offset table.  However,
> due to the reprocessing you added in this patch, the attribute value is now in fact the
> offset with `DW_AT_rnglists_base` already added.  And _that's_ the reason why
> DW_FORM_rnglistx attributes should not have the range base applied.
>
> I think the comment above in the code is misleading and needs to be updated, this one:
>
>           /* DW_AT_rnglists_base does not apply to DIEs from the DWO skeleton.
>              We take advantage of the fact that DW_AT_ranges does not appear
>              in DW_TAG_compile_unit of DWO files.  */
>
> In commit 18a8505e38fc ("Dwarf 5: Handle debug_str_offsets and indexed attributes
> that have base offsets."), the it was changed like so:
>
> -         /* DW_AT_ranges_base does not apply to DIEs from the DWO skeleton.
> +         /* DW_AT_rnglists_base does not apply to DIEs from the DWO skeleton.
>
> I think it was a wrong change, because it mixes two different things.  DW_AT_rnglists_base
> is a DWARF 5 thing, and the range base not applying to the values from the skeleton is a
> pre-standard thing.  I think it should say something like:
>
>           /* DW_AT_GNU_ranges_base does not apply to DIEs from the DWO skeleton.
>              We take advantage of the fact that DW_AT_ranges does not appear
>              in DW_TAG_compile_unit of DWO files.
>
>              Attributes of form DW_AT_rnglistx have already had their value changed
>              by read_rnglist_index and already include DW_AT_rnglists_base, so don't
>              add the ranges base either.  */
>
> >
> >> I'm also not sure I understand it: when the form is DW_FORM_rnglistx, don't
> >> we want to sometimes apply the base coming from DW_AT_rnglists_base?
> >
> > It all depends on what form is being used. If the DW_AT_ranges is
> > using DW_FORM_sec_offset (which is what
> > GCC generates), then you do need to add the base value.But if you
> > are using the DW_FORM_rnglisx (which is what clang generates), the
> > base value has already been considered/incorporated into the attribute
> > so adding it in (again) causes the value to be incorrect (again, turn
> > on complaints and see what happens if you try to add the base into
> > something with DW_FORM_rnglistx).  From the DWARF5 spec: "[A] rnglist
> > is either represented as: An index into the .debug_rnglists section
> > (DW_FORM_rnglistx). The ...operand identifies an offset location
> > relative to the base of that section...[or] An offset into the
> > .debug_rnglists section (DW_FORM_sec_offset).  The operand consists of
> > a byte offset from the beginning of the .debug_rnglists section."
> > Note the DW_FORM_rnglistx is already relative to the base.
>
> Ok.  What was not clear to me in the first pass was that read_rnglist_index
> already has modified the attribute value to include DW_AT_rnglists_base.
>
> Now, about values of class rnglist and form DW_FORM_sec_offset in DWARF 5.
> The spec says this about them:
>
>   - An offset into the .debug_rnglists section (DW_FORM_sec_offset).
>   The operand consists of a byte offset from the beginning of the
>   .debug_rnglists section. It is relocatable in a relocatable object file,
>   and relocated in an executable or shared object file. In the 32-bit
>   DWARF format, this offset is a 4-byte unsigned value; in the 64-bit
>   DWARF format, it is an 8-byte unsigned value (see Section 7.4 on
>   page 196).
>
> Since it says that the value is already relocated in an executable, does it
> mean we should not add the base for these, in DWARF 5?
>
> > Now, my question:  I'm not quite clear as to whether you really want
> > me to change the code in
> > cu_debug_rnglists_section or not.   I wrote the code mostly by copying
> > the code for loclists as closely as possible, and making whatever
> > changes to that were required to make it correct for rnglists.  Is it
> > better to have the code more consistent with the loclists code?If
> > you really want me to make the changes there you suggest, then I will;
>
> I think the situation is a bit different than for `cu_debug_loc_section`:
> when you have a dwo, I don't think there is the possibility of having an
> attribute referring to a location list in the skeleton.  So if you have a
> dwo, you'll always want the loc section from the dwo.  So there, the pattern
>
>   if (there is a dwo)
>     return the dwo section;
>
>   return the main file section;
>
> makes more sense.  But in the case of range lists, we have attributes
> in both the skeleton and the dwo that require range lists, and the right
> section to return depends on where the attribute is.  That's why, when I
> see this pattern:
>
>   if (there is a dwo)
>     return the dwo section;
>
>   return the main file section;
>
> that pattern, I find it looks wrong, because the right section to return
> does not depend (only) on whether there is dwo.  As we saw, it only works
> "by chance" because this is only called once for the skeleton (when
> reading DW_AT_ranges) and the `cu->dwo_unit` pointer happens not to be set
> up yet.  It is also one refactor away from breaking, if for some reason we
> reorder some code and the `cu->dwo_unit` pointer gets initialized earlier.
>
> > I just wanted confirmation that you really want that change (i.e.
> > adding the dwarf tag and checking vs. DW_TAG_compile_unit, & calling
> > it from dwarf2_rnglists_process).
>
> I think that checking DW_TAG_compile_unit is still a bit of a hack - ideally,
> the caller could just tell us if the DIE comes from a dwo or not.  But at
> least it's logical and based on some DWARF knowledge.  It would certainly
> require a comment to explain it, because it wouldn't be obvious.
>
> > @@ -1379,11 +1382,16 @@ static void read_call_site_scope (struct die_info *die, struct dwarf2_cu *cu);
> >  static void read_variable (struct die_info *die, struct dwarf2_cu *cu);
> >
> >  static int dwarf2_ranges_read (unsigned, CORE_ADDR *, CORE_ADDR *,
> > -                            struct dwarf2_cu *, dwarf2_psymtab *);
> > +                            struct dwarf2_cu *, dwarf2_psymtab *,
> > +                            dwarf_tag);
>
> This declaration is unneeded and can simply be removed.  Could you push an
> obvious patch that does this?.
>
>
> > @@ -13917,8 +13969,20 @@ dwarf2_rnglists_process (unsigned offset, struct dwarf2_cu *cu,
> >        if (range_beginning == range_end)
> >       continue;
> >
> > -      range_beginning += *base;
> > -      range_end += *base;
> > +      /* Only DW_RLE_offset_pair needs the base address added.  */
> > +      if (rlet == DW_RLE_offset_pair)
> > +     {
> > +       if (!base.has_value ())
> > +         {
> > +           /* We have no valid base address for the ranges
> > +              data.  */
> > +           complaint (_("Invalid .debug_rnglists data (no base address)"));
>
> The comment fits on a single line.  But it can probably be changed to be more
> specific to DW_RLE_offset_pair.  Maybe the complaint could be more specific too,
> and mention DW_RLE_offset_pair.  It would be valid to have .debug_rnglists without
> a base address (I think?), but here it's invalid because there is not base address
> _and_ we have encountered a DW_RLE_offset_pair.
>
> > @@ -19094,6 +19167,57 @@ read_loclist_index (struct dwarf2_cu *cu, ULONGEST loclist_index)
> >      return bfd_get_64 (abfd, info_ptr) + loclist_base;
> >  }
> >
> > +/* Given a DW_FORM_rnglistx value rnglist_index, fetch the offset from the
> > +   array of offsets in the .debug_rnglists section.  */
> > +static CORE_ADDR
> > +read_rnglist_index (struct dwarf2_cu *cu, ULONGEST rnglist_index)
> > +{
> > +  struct dwarf2_per_objfile *dwarf2_per_objfile = cu->per_objfile;
> > +  struct objfile *objfile = dwarf2_per_objfile->objfile;
> > +  bfd *abfd = objfile->obfd;
> > +  ULONGEST rnglist_header_size =
> > +    (cu->header.initial_length_size == 4 ? LOCLIST_HEADER_SIZE32
> > +     : LOCLIST_HEADER_SIZE64);
>
> Should this really refer to loclist header sizes?  If they are the same
> sizes as range list table headers, I suppose it's just a coincidence.
>
> > +  ULONGEST rnglist_base =
> > +      (cu->dwo_unit) ? rnglist_header_size : cu->ranges_base;
>
> cu->dwo_unit != nullptr
>
> > +  ULONGEST start_offset =
> > +    rnglist_base + rnglist_index * cu->header.offset_size;
> > +
> > +  /* Get rnglists section.  */
> > +  struct dwarf2_section_info *section = cu_debug_rnglists_section (cu);
> > +  if (section == nullptr)
> > +    error (_("Cannot find .debug_rnglists section [in module %s]"),
> > +        objfile_name(objfile));
>
> Space before paren.
>
> > +
> > +  /* Read the rnglists section content.  */
> > +  section->read (objfile);
> > +  if (section->buffer == NULL)
>
> In new code, I suggest using nullptr instead of NULL, just for consistency.
>
> > +    error (_("DW_FORM_rnglistx used without .debug_rnglists section "
> > +          "[in module %s]"),
> > +       objfile_name (objfile));
> > +
> > +  /* Verify the rnglist header is valid (same as loclist header).  */
> > +  struct loclist_header header;
> > +  read_loclist_header (&header, section);
>
> Ok well, if we are going to take advantage that they are the same, the name
> of the function and types should reflect it.  `read_loclist_header` should
> become `read_loclists_rnglists_header` (using the plural to match the section
> names).
>
> > +  if (rnglist_index >= header.offset_entry_count)
> > +    error (_("DW_FORM_rnglistx index pointing outside of "
> > +          ".debug_rnglists offset array [in module %s]"),
> > +         objfile_name(objfile));
>
> Space before paren.
>
> > +
> > +  /* Validate that the offset is within the section's range.  */
> > +  if (start_offset >= section->size)
> > +    error (_("DW_FORM_rnglistx pointing outside of "
> > +             ".debug_rnglists section [in module %s]"),
> > +          objfile_name(objfile));
> > +
> > +  const gdb_byte *info_ptr = section->buffer + start_offset;
> > +
> > +  if (cu->header.offset_size == 4)
> > +    return read_4_bytes (abfd, info_ptr) + rnglist_base;
> > +  else
> > +    return read_8_bytes (abfd, info_ptr) + rnglist_base;
>
> An idea for a subsequent cleanup, we could have a `read_offset` function
> that does
>
>   if (cu->header.offset_size == 4)
>     return read_4_bytes (abfd, info_ptr);
>   else
>     return read_8_bytes (abfd, info_ptr);
>
> There are a few spots that could use it.
>
> Simon

v4-0001-Update-GDB-to-fix-issues-with-handling-DWARF-v5-r.patch (36K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH V4] Fix issues with reading rnglists, especially from dwo files, for DWARF v5

Simon Marchi-4
> @@ -1378,12 +1387,13 @@ static void read_call_site_scope (struct die_info *die, struct dwarf2_cu *cu);
>
>  static void read_variable (struct die_info *die, struct dwarf2_cu *cu);
>
> -static int dwarf2_ranges_read (unsigned, CORE_ADDR *, CORE_ADDR *,
> -       struct dwarf2_cu *, dwarf2_psymtab *);
> -
>  /* Return the .debug_loclists section to use for cu.  */
>  static struct dwarf2_section_info *cu_debug_loc_section (struct dwarf2_cu *cu);
>
> +/* Return the .debug_rnglists section to use for cu.  */
> +static struct dwarf2_section_info *cu_debug_rnglists_section
> +(struct dwarf2_cu *cu, dwarf_tag tag);

Indent this last line with two spaces.

> @@ -13802,17 +13817,33 @@ dwarf2_rnglists_process (unsigned offset, struct dwarf2_cu *cu,
>    const gdb_byte *buffer;
>    CORE_ADDR baseaddr;
>    bool overflow = false;
> +  ULONGEST addr_index;
> +  bool ignore_dwo_unit = false;
> +  struct dwarf2_section_info *rnglists_section;
>
>    base = cu->base_address;
>
> -  per_objfile->per_bfd->rnglists.read (objfile);
> -  if (offset >= per_objfile->per_bfd->rnglists.size)
> +  /* Make sure we read the .debug_rnglists section from the file that
> +     contains the DW_AT_ranges attribute we are reading.  Normally that
> +     would be the .dwo file, if there is one.  However for DW_TAG_compile_unit
> +     we always want to read from objfile/linked program (which would be the
> +     DW_TAG_skeleton_unit DIE if we're using split dwarf).  */
> +  if (tag == DW_TAG_compile_unit)
> +    ignore_dwo_unit = true;
> +
> +  if (cu->dwo_unit != nullptr && !ignore_dwo_unit)
> +      rnglists_section = &cu->dwo_unit->dwo_file->sections.rnglists;
> +  else
> +      rnglists_section = &per_objfile->per_bfd->rnglists;

If think you can omit the `ignore_dwo_unit` variable and write this directly,
without loss of readability:

  if (cu->dwo_unit != nullptr && tag != DW_TAG_compile_unit)

Also, should this use cu_debug_rnglists_section?

> @@ -19094,13 +19184,65 @@ read_loclist_index (struct dwarf2_cu *cu, ULONGEST loclist_index)
>      return bfd_get_64 (abfd, info_ptr) + loclist_base;
>  }
>
> +/* Given a DW_FORM_rnglistx value rnglist_index, fetch the offset from the

rnglist_index -> RNGLIST_INDEX

> +   array of offsets in the .debug_rnglists section.  */
> +static CORE_ADDR
> +read_rnglist_index (struct dwarf2_cu *cu, ULONGEST rnglist_index,
> +    dwarf_tag tag)
> +{
> +  struct dwarf2_per_objfile *dwarf2_per_objfile = cu->per_objfile;
> +  struct objfile *objfile = dwarf2_per_objfile->objfile;
> +  bfd *abfd = objfile->obfd;
> +  ULONGEST rnglist_header_size =
> +    (cu->header.initial_length_size == 4 ? RNGLIST_HEADER_SIZE32
> +     : RNGLIST_HEADER_SIZE64);
> +  ULONGEST rnglist_base =
> +      (cu->dwo_unit != nullptr) ? rnglist_header_size : cu->ranges_base;
> +  ULONGEST start_offset =
> +    rnglist_base + rnglist_index * cu->header.offset_size;
> +
> +  /* Get rnglists section.  */
> +  struct dwarf2_section_info *section = cu_debug_rnglists_section (cu, tag);
> +  if (section == nullptr)
> +    error (_("Cannot find .debug_rnglists section [in module %s]"),
> +   objfile_name (objfile));

As of now, cu_debug_rnglists_section can't return nullptr.  And I think it should
stay this way, it throws an error if it can't return a valid section.

> +
> +  /* Read the rnglists section content.  */
> +  section->read (objfile);
> +  if (section->buffer == nullptr)
> +    error (_("DW_FORM_rnglistx used without .debug_rnglists section "
> +     "[in module %s]"),
> +       objfile_name (objfile));

Align `objfile_name` with the opening parenthesis.

> +
> +  /* Verify the rnglist header is valid (same as loclist header).  */

Is this comment accurate?  The code validates rnglist_index, not the header, right?

> +  struct loclist_header header;

Rename this type `loclists_rnglists_header`, to match the function.

> +  read_loclists_rnglists_header (&header, section);
> +  if (rnglist_index >= header.offset_entry_count)
> +    error (_("DW_FORM_rnglistx index pointing outside of "
> +     ".debug_rnglists offset array [in module %s]"),
> +    objfile_name (objfile));

Same here.

> +
> +  /* Validate that the offset is within the section's range.  */
> +  if (start_offset >= section->size)

Pendantically, I suppose we should verify not only that the start_offset
is within the section's range, but also that there are enough bytes to read?

For example, if we have a 100 bytes-long section, and start_offset is 99, we
won't be able to read 4 or 8 bytes.

> +    error (_("DW_FORM_rnglistx pointing outside of "
> +             ".debug_rnglists section [in module %s]"),
> +          objfile_name (objfile));

Same here.


> @@ -23383,6 +23530,30 @@ cu_debug_loc_section (struct dwarf2_cu *cu)
>    : &per_objfile->per_bfd->loc);
>  }
>
> +/* Return the .debug_rnglists section to use for CU.  */
> +static struct dwarf2_section_info *
> +cu_debug_rnglists_section (struct dwarf2_cu *cu, dwarf_tag tag)
> +{
> +  if (cu->header.version < 5)
> +    error (_(".debug_rnglists section cannot be used in DWARF %d"),
> +   cu->header.version);
> +  struct dwarf2_per_objfile *dwarf2_per_objfile = cu->per_objfile;
> +
> +  /* Make sure we read the .debug_rnglists section from the file that
> +     contains the DW_AT_ranges attribute we are reading.  Normally that
> +     would be the .dwo file, if there is one.  However for DW_TAG_compile_unit
> +     we always want to read from objfile/linked program (which would be the
> +     DW_TAG_skeleton_unit DIE if we're using split dwarf).  */
> +  if (cu->dwo_unit != nullptr && tag != DW_TAG_compile_unit)
> +    {
> +      struct dwo_sections *sections = &cu->dwo_unit->dwo_file->sections;
> +
> +      if (sections->rnglists.size > 0)
> + return &sections->rnglists;
> +    }
> +  return &dwarf2_per_objfile->per_bfd->rnglists;

I expressed a concern about this earlier: if an attribute in the .dwo is of form
DW_FORM_rnglistx, but for some reason the .dwo has no .debug_rnglist section, what
happens?  It seems to me like we're going to return the section from the executable,
which would be wrong.

Also, I just debugged this function, and saw that it is called in this context:

#0  cu_debug_rnglists_section (cu=0x615000026c80, tag=DW_TAG_skeleton_unit) at /home/simark/src/binutils-gdb/gdb/dwarf2/read.c:23557
#1  0x0000557c4612cde5 in read_rnglist_index (cu=0x615000026c80, rnglist_index=0, tag=DW_TAG_skeleton_unit) at /home/simark/src/binutils-gdb/gdb/dwarf2/read.c:19205
#2  0x0000557c4612d66e in read_attribute_reprocess (reader=0x7fffb47cf240, attr=0x621000186198, tag=DW_TAG_skeleton_unit) at /home/simark/src/binutils-gdb/gdb/dwarf2/read.c:19258
#3  0x0000557c4612152d in read_full_die_1 (reader=0x7fffb47cf240, diep=0x7fffb47cf280, info_ptr=0x60400001e8c0 "", num_extra_attrs=0) at /home/simark/src/binutils-gdb/gdb/dwarf2/read.c:18253
#4  0x0000557c461217d9 in read_full_die (reader=0x7fffb47cf240, diep=0x7fffb47cf280, info_ptr=0x60400001e8a4 "\001") at /home/simark/src/binutils-gdb/gdb/dwarf2/read.c:18268
#5  0x0000557c46094af2 in cutu_reader::cutu_reader (this=0x7fffb47cf240, this_cu=0x621000183910, per_objfile=0x61300000a840, abbrev_table=0x60b00006ff30, existing_cu=0x0, skip_partial=false) at /
home/simark/src/binutils-gdb/gdb/dwarf2/read.c:7202
#6  0x0000557c4609b0ca in process_psymtab_comp_unit (this_cu=0x621000183910, per_objfile=0x61300000a840, want_partial_unit=false, pretend_language=language_minimal) at /home/simark/src/binutils-gdb/gdb/dwarf2/read.c:7667
#7  0x0000557c460a0fd9 in dwarf2_build_psymtabs_hard (per_objfile=0x61300000a840) at /home/simark/src/binutils-gdb/gdb/dwarf2/read.c:8051
#8  0x0000557c46086ae3 in dwarf2_build_psymtabs (objfile=0x614000006440) at /home/simark/src/binutils-gdb/gdb/dwarf2/read.c:6106

As you can see, tag == DW_TAG_skeleton_unit.

I thought that somehow, when reading a CU that uses a DWO, we were creating
a "logical" DIE tree by combining the DW_TAG_skeleton_unit DIE and the
children of the DWO's DW_TAG_compile_unit DIE, and while doing this,
overwriting the DW_TAG_skeleton_unit's DIE to use the DW_TAG_compile_unit
tag instead.  Therefore making it appear to the rest of the DWARF reader
as if it was a "standard" DW_TAG_compile_unit DIE.  But no, maybe I just dreamed
all of this, or I can't find it anymore.

In fact, the reason the code was checking for DW_TAG_compile_unit must be that
in the GCC/pre-standard version, the skeleton DIE in the executable is a
DW_TAG_compile_unit.  With DWARF5, we'll see DW_TAG_skeleton_unit here.

So I believe we should use

  (tag != DW_TAG_compile_unit && tag != DW_TAG_skeleton_unit)

to cover both versions, GCC pre-standard and DWARF 5.  Does that make sense?

Wherever we use the logic:

          int need_ranges_base = (die->tag != DW_TAG_compile_unit
                                  && attr->form != DW_FORM_rnglistx);

we should maybe check for DW_TAG_skeleton_unit as well?

Simon
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH V5] Fix issues with reading rnglists, especially from dwo files, for DWARF v5

Sourceware - gdb-patches mailing list
"This time for sure!" -- Bullwinkle Moose

I think I've got all of your requested changes in now, and I've
attached the updated patch.   About what you said at the very end of
your last message:

> I thought that somehow, when reading a CU that uses a DWO, we were creating
> a "logical" DIE tree by combining the DW_TAG_skeleton_unit DIE and the
> children of the DWO's DW_TAG_compile_unit DIE, and while doing this,
> overwriting the DW_TAG_skeleton_unit's DIE to use the DW_TAG_compile_unit
> tag instead.  Therefore making it appear to the rest of the DWARF reader
> as if it was a "standard" DW_TAG_compile_unit DIE.  But no, maybe I just dreamed
> all of this, or I can't find it anymore.
>

Actually your first thought was absolutely correct.  This is done in
cutu_reader::cutu_reader.  In my patched
read.c this is at line 7244:

comp_unit_die = dwo_comp_unit_die;

It's 4 lines above the comment:

         /* Yikes, we couldn't find the rest of the DIE, we only have
             the stub.  A complaint has already been logged.  There's
             not much more we can do except pass on the stub DIE to
             die_reader_func.  We don't want to throw an error on bad
             debug info.  */

> In fact, the reason the code was checking for DW_TAG_compile_unit must be that
> in the GCC/pre-standard version, the skeleton DIE in the executable is a
> DW_TAG_compile_unit.  With DWARF5, we'll see DW_TAG_skeleton_unit here.
>
> So I believe we should use
>
>  (tag != DW_TAG_compile_unit && tag != DW_TAG_skeleton_unit)
>
> to cover both versions, GCC pre-standard and DWARF 5.  Does that make sense?

I agree that we need to check both cases in cu_debug_rnglists_section,
because sometimes it gets called before the line above in cutu_reader,
and sometimes it gets called after (now that I'm also calling it in
dwarf2_rnglists_process).

> Wherever we use the logic:
>
>          int need_ranges_base = (die->tag != DW_TAG_compile_unit
>                                  && attr->form != DW_FORM_rnglistx);
>
> we should maybe check for DW_TAG_skeleton_unit as well?"

I don't think there's any point in checking for DW_TAG_skeleton_unit
in the need_ranges_base checks, because I believe that all of those
checks are called after the call to cutu_reader, so we never have a
DW_TAG_skeleton_unit by the time we get to those checks.

Please review the updated, attached patch and let me know if it is ok
now.  Thanks! :-)


-- Caroline
[hidden email]

gdb/ChangeLog

2020-07-14  Caroline Tice  <[hidden email]>

        * dwarf2/read.c (RNGLIST_HEADER_SIZE32) New constant definition.
        (RNGLIST_HEADER_SIZE64): New constant definition.
        (struct dwop_section_names): Add rnglists_dwo.
        (dwop_section_names): Add .debug_rnglists.dwo, .zdebug_rnglists.dwo.
        (struct loclist_header): Rename to 'loclists_rnglists_header'.
        (struct dwo_sections): Add rnglists field.
        (read_attribut_reprocess): Add tag parameter.
        (dwarf2_ranges_read): Add tag parameter & remove forward function decl.
        (cu_debug_rnglists_section): New function (decl & definition).
        (dwarf2_locate_dwo_sections): Add code to read rnglists_dwo section.
        (dwarf2_rnglists_process): Add a dwarf_tag parameter, for the kind of
        die whose range is being checked; get rnglist section from
        cu_debug_rnglists_section, to get from either objfile or dwo file as
        appropriate.  Add cases for DW_RLE_base_addressx,
        DW_RLE_startx_length, DW_RLE_startx_endx.  Also, update to only add
        the base address to DW_RLE_offset_pairs (not to all ranges), moving
        test inside if-condition and updating complaint message.
        (dwarf2_ranges_process): Add dwarf tag parameter and pass it to
        dwarf2_rnglists_process.
        (dwarf2_ranges_read): Add dwarf tag parameter and pass it to
        dwarf2_ranges_process.
        (dwarf2_get_pc_bounds): Check for DW_FORM_rnglistx when setting
        need_ranges_base and update comment appropriately.  Also pass die tag
        to dwarf2_ranges_read.
        (dwarf2_record_block_ranges): Check for DW_FORM_rnglistx when setting
        need_ranges_base and update comment appropriately.  Also pass die tag
        to dwarf2_ranges_process.
        (read_full_die_1): Add code to read DW_AT_rnglists_base and assign to
        cu->ranges_base.  Also pass die tag to read_attribute_reprocess.
        (partial_die_info::read): Check for DW_FORM_rnglistx when setting
        need_ranges_base and update comment appropriately.  Also pass die tag
        to read_attribute_reprocess and dwarf2_ranges_read.
        (read_loclist_header): Rename function to read_loclists_rnglists_header,
        and update function comment appropriately.
        (read_loclist_index): Call read_loclists_rnglists_header instead of
        read_loclist_header.
        (read_rnglist_index): New function.
        (read_attribute_reprocess):  Add tag parameter. Add code for
        DW_FORM_rnglistx, passing tag to read_rnglist_index.
        (read_attribute_value): Mark DW_FORM_rnglistx with need_reprocess.

gdb/testsuite/ChangeLog:

2020-07-14  Caroline Tice  <[hidden email]>

        * gdb.dwarf2/dw5-rnglist-test.cc: New file.
        * gdb.dwarf2/dw5-rnglist-test.exp: New file.




-- Caroline
[hidden email]


On Sat, Jul 11, 2020 at 10:54 AM Simon Marchi <[hidden email]> wrote:

>
> > @@ -1378,12 +1387,13 @@ static void read_call_site_scope (struct die_info *die, struct dwarf2_cu *cu);
> >
> >  static void read_variable (struct die_info *die, struct dwarf2_cu *cu);
> >
> > -static int dwarf2_ranges_read (unsigned, CORE_ADDR *, CORE_ADDR *,
> > -                            struct dwarf2_cu *, dwarf2_psymtab *);
> > -
> >  /* Return the .debug_loclists section to use for cu.  */
> >  static struct dwarf2_section_info *cu_debug_loc_section (struct dwarf2_cu *cu);
> >
> > +/* Return the .debug_rnglists section to use for cu.  */
> > +static struct dwarf2_section_info *cu_debug_rnglists_section
> > +(struct dwarf2_cu *cu, dwarf_tag tag);
>
> Indent this last line with two spaces.
>
> > @@ -13802,17 +13817,33 @@ dwarf2_rnglists_process (unsigned offset, struct dwarf2_cu *cu,
> >    const gdb_byte *buffer;
> >    CORE_ADDR baseaddr;
> >    bool overflow = false;
> > +  ULONGEST addr_index;
> > +  bool ignore_dwo_unit = false;
> > +  struct dwarf2_section_info *rnglists_section;
> >
> >    base = cu->base_address;
> >
> > -  per_objfile->per_bfd->rnglists.read (objfile);
> > -  if (offset >= per_objfile->per_bfd->rnglists.size)
> > +  /* Make sure we read the .debug_rnglists section from the file that
> > +     contains the DW_AT_ranges attribute we are reading.  Normally that
> > +     would be the .dwo file, if there is one.  However for DW_TAG_compile_unit
> > +     we always want to read from objfile/linked program (which would be the
> > +     DW_TAG_skeleton_unit DIE if we're using split dwarf).  */
> > +  if (tag == DW_TAG_compile_unit)
> > +    ignore_dwo_unit = true;
> > +
> > +  if (cu->dwo_unit != nullptr && !ignore_dwo_unit)
> > +      rnglists_section = &cu->dwo_unit->dwo_file->sections.rnglists;
> > +  else
> > +      rnglists_section = &per_objfile->per_bfd->rnglists;
>
> If think you can omit the `ignore_dwo_unit` variable and write this directly,
> without loss of readability:
>
>   if (cu->dwo_unit != nullptr && tag != DW_TAG_compile_unit)
>
> Also, should this use cu_debug_rnglists_section?
>
> > @@ -19094,13 +19184,65 @@ read_loclist_index (struct dwarf2_cu *cu, ULONGEST loclist_index)
> >      return bfd_get_64 (abfd, info_ptr) + loclist_base;
> >  }
> >
> > +/* Given a DW_FORM_rnglistx value rnglist_index, fetch the offset from the
>
> rnglist_index -> RNGLIST_INDEX
>
> > +   array of offsets in the .debug_rnglists section.  */
> > +static CORE_ADDR
> > +read_rnglist_index (struct dwarf2_cu *cu, ULONGEST rnglist_index,
> > +                 dwarf_tag tag)
> > +{
> > +  struct dwarf2_per_objfile *dwarf2_per_objfile = cu->per_objfile;
> > +  struct objfile *objfile = dwarf2_per_objfile->objfile;
> > +  bfd *abfd = objfile->obfd;
> > +  ULONGEST rnglist_header_size =
> > +    (cu->header.initial_length_size == 4 ? RNGLIST_HEADER_SIZE32
> > +     : RNGLIST_HEADER_SIZE64);
> > +  ULONGEST rnglist_base =
> > +      (cu->dwo_unit != nullptr) ? rnglist_header_size : cu->ranges_base;
> > +  ULONGEST start_offset =
> > +    rnglist_base + rnglist_index * cu->header.offset_size;
> > +
> > +  /* Get rnglists section.  */
> > +  struct dwarf2_section_info *section = cu_debug_rnglists_section (cu, tag);
> > +  if (section == nullptr)
> > +    error (_("Cannot find .debug_rnglists section [in module %s]"),
> > +        objfile_name (objfile));
>
> As of now, cu_debug_rnglists_section can't return nullptr.  And I think it should
> stay this way, it throws an error if it can't return a valid section.
>
> > +
> > +  /* Read the rnglists section content.  */
> > +  section->read (objfile);
> > +  if (section->buffer == nullptr)
> > +    error (_("DW_FORM_rnglistx used without .debug_rnglists section "
> > +          "[in module %s]"),
> > +       objfile_name (objfile));
>
> Align `objfile_name` with the opening parenthesis.
>
> > +
> > +  /* Verify the rnglist header is valid (same as loclist header).  */
>
> Is this comment accurate?  The code validates rnglist_index, not the header, right?
>
> > +  struct loclist_header header;
>
> Rename this type `loclists_rnglists_header`, to match the function.
>
> > +  read_loclists_rnglists_header (&header, section);
> > +  if (rnglist_index >= header.offset_entry_count)
> > +    error (_("DW_FORM_rnglistx index pointing outside of "
> > +          ".debug_rnglists offset array [in module %s]"),
> > +         objfile_name (objfile));
>
> Same here.
>
> > +
> > +  /* Validate that the offset is within the section's range.  */
> > +  if (start_offset >= section->size)
>
> Pendantically, I suppose we should verify not only that the start_offset
> is within the section's range, but also that there are enough bytes to read?
>
> For example, if we have a 100 bytes-long section, and start_offset is 99, we
> won't be able to read 4 or 8 bytes.
>
> > +    error (_("DW_FORM_rnglistx pointing outside of "
> > +             ".debug_rnglists section [in module %s]"),
> > +          objfile_name (objfile));
>
> Same here.
>
>
> > @@ -23383,6 +23530,30 @@ cu_debug_loc_section (struct dwarf2_cu *cu)
> >                                 : &per_objfile->per_bfd->loc);
> >  }
> >
> > +/* Return the .debug_rnglists section to use for CU.  */
> > +static struct dwarf2_section_info *
> > +cu_debug_rnglists_section (struct dwarf2_cu *cu, dwarf_tag tag)
> > +{
> > +  if (cu->header.version < 5)
> > +    error (_(".debug_rnglists section cannot be used in DWARF %d"),
> > +        cu->header.version);
> > +  struct dwarf2_per_objfile *dwarf2_per_objfile = cu->per_objfile;
> > +
> > +  /* Make sure we read the .debug_rnglists section from the file that
> > +     contains the DW_AT_ranges attribute we are reading.  Normally that
> > +     would be the .dwo file, if there is one.  However for DW_TAG_compile_unit
> > +     we always want to read from objfile/linked program (which would be the
> > +     DW_TAG_skeleton_unit DIE if we're using split dwarf).  */
> > +  if (cu->dwo_unit != nullptr && tag != DW_TAG_compile_unit)
> > +    {
> > +      struct dwo_sections *sections = &cu->dwo_unit->dwo_file->sections;
> > +
> > +      if (sections->rnglists.size > 0)
> > +     return &sections->rnglists;
> > +    }
> > +  return &dwarf2_per_objfile->per_bfd->rnglists;
>
> I expressed a concern about this earlier: if an attribute in the .dwo is of form
> DW_FORM_rnglistx, but for some reason the .dwo has no .debug_rnglist section, what
> happens?  It seems to me like we're going to return the section from the executable,
> which would be wrong.
>
> Also, I just debugged this function, and saw that it is called in this context:
>
> #0  cu_debug_rnglists_section (cu=0x615000026c80, tag=DW_TAG_skeleton_unit) at /home/simark/src/binutils-gdb/gdb/dwarf2/read.c:23557
> #1  0x0000557c4612cde5 in read_rnglist_index (cu=0x615000026c80, rnglist_index=0, tag=DW_TAG_skeleton_unit) at /home/simark/src/binutils-gdb/gdb/dwarf2/read.c:19205
> #2  0x0000557c4612d66e in read_attribute_reprocess (reader=0x7fffb47cf240, attr=0x621000186198, tag=DW_TAG_skeleton_unit) at /home/simark/src/binutils-gdb/gdb/dwarf2/read.c:19258
> #3  0x0000557c4612152d in read_full_die_1 (reader=0x7fffb47cf240, diep=0x7fffb47cf280, info_ptr=0x60400001e8c0 "", num_extra_attrs=0) at /home/simark/src/binutils-gdb/gdb/dwarf2/read.c:18253
> #4  0x0000557c461217d9 in read_full_die (reader=0x7fffb47cf240, diep=0x7fffb47cf280, info_ptr=0x60400001e8a4 "\001") at /home/simark/src/binutils-gdb/gdb/dwarf2/read.c:18268
> #5  0x0000557c46094af2 in cutu_reader::cutu_reader (this=0x7fffb47cf240, this_cu=0x621000183910, per_objfile=0x61300000a840, abbrev_table=0x60b00006ff30, existing_cu=0x0, skip_partial=false) at /
> home/simark/src/binutils-gdb/gdb/dwarf2/read.c:7202
> #6  0x0000557c4609b0ca in process_psymtab_comp_unit (this_cu=0x621000183910, per_objfile=0x61300000a840, want_partial_unit=false, pretend_language=language_minimal) at /home/simark/src/binutils-gdb/gdb/dwarf2/read.c:7667
> #7  0x0000557c460a0fd9 in dwarf2_build_psymtabs_hard (per_objfile=0x61300000a840) at /home/simark/src/binutils-gdb/gdb/dwarf2/read.c:8051
> #8  0x0000557c46086ae3 in dwarf2_build_psymtabs (objfile=0x614000006440) at /home/simark/src/binutils-gdb/gdb/dwarf2/read.c:6106
>
> As you can see, tag == DW_TAG_skeleton_unit.
>
> I thought that somehow, when reading a CU that uses a DWO, we were creating
> a "logical" DIE tree by combining the DW_TAG_skeleton_unit DIE and the
> children of the DWO's DW_TAG_compile_unit DIE, and while doing this,
> overwriting the DW_TAG_skeleton_unit's DIE to use the DW_TAG_compile_unit
> tag instead.  Therefore making it appear to the rest of the DWARF reader
> as if it was a "standard" DW_TAG_compile_unit DIE.  But no, maybe I just dreamed
> all of this, or I can't find it anymore.
>
> In fact, the reason the code was checking for DW_TAG_compile_unit must be that
> in the GCC/pre-standard version, the skeleton DIE in the executable is a
> DW_TAG_compile_unit.  With DWARF5, we'll see DW_TAG_skeleton_unit here.
>
> So I believe we should use
>
>   (tag != DW_TAG_compile_unit && tag != DW_TAG_skeleton_unit)
>
> to cover both versions, GCC pre-standard and DWARF 5.  Does that make sense?
>
> Wherever we use the logic:
>
>           int need_ranges_base = (die->tag != DW_TAG_compile_unit
>                                   && attr->form != DW_FORM_rnglistx);
>
> we should maybe check for DW_TAG_skeleton_unit as well?
>
> Simon

v5-0001-Update-GDB-to-fix-issues-with-handling-DWARF-v5-r.patch (36K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH V5] Fix issues with reading rnglists, especially from dwo files, for DWARF v5

Simon Marchi-4
On 2020-07-14 11:47 a.m., Caroline Tice wrote:

> "This time for sure!" -- Bullwinkle Moose
>
> I think I've got all of your requested changes in now, and I've
> attached the updated patch.   About what you said at the very end of
> your last message:
>
>> I thought that somehow, when reading a CU that uses a DWO, we were creating
>> a "logical" DIE tree by combining the DW_TAG_skeleton_unit DIE and the
>> children of the DWO's DW_TAG_compile_unit DIE, and while doing this,
>> overwriting the DW_TAG_skeleton_unit's DIE to use the DW_TAG_compile_unit
>> tag instead.  Therefore making it appear to the rest of the DWARF reader
>> as if it was a "standard" DW_TAG_compile_unit DIE.  But no, maybe I just dreamed
>> all of this, or I can't find it anymore.
>>
>
> Actually your first thought was absolutely correct.  This is done in
> cutu_reader::cutu_reader.  In my patched
> read.c this is at line 7244:
>
> comp_unit_die = dwo_comp_unit_die;

Ah ok, we just the full tree from the dwo.  And in read_cutu_die_from_dwo we copy
over some useful attributes from the skeleton to the dwo's root DIE, like the ranges.

Looks like I need to read and understand something a few times before it stays :).

>> In fact, the reason the code was checking for DW_TAG_compile_unit must be that
>> in the GCC/pre-standard version, the skeleton DIE in the executable is a
>> DW_TAG_compile_unit.  With DWARF5, we'll see DW_TAG_skeleton_unit here.
>>
>> So I believe we should use
>>
>>  (tag != DW_TAG_compile_unit && tag != DW_TAG_skeleton_unit)
>>
>> to cover both versions, GCC pre-standard and DWARF 5.  Does that make sense?
>
> I agree that we need to check both cases in cu_debug_rnglists_section,
> because sometimes it gets called before the line above in cutu_reader,
> and sometimes it gets called after (now that I'm also calling it in
> dwarf2_rnglists_process).

Ok.

>> Wherever we use the logic:
>>
>>          int need_ranges_base = (die->tag != DW_TAG_compile_unit
>>                                  && attr->form != DW_FORM_rnglistx);
>>
>> we should maybe check for DW_TAG_skeleton_unit as well?"
>
> I don't think there's any point in checking for DW_TAG_skeleton_unit
> in the need_ranges_base checks, because I believe that all of those
> checks are called after the call to cutu_reader, so we never have a
> DW_TAG_skeleton_unit by the time we get to those checks.

Makes sense, I think.

I don't think I have any more comments.  Tom, are you ok with this?

Simon
12