Re: GDB 10.2 release (respin) -- 2021-01-31 Update

[Simon Marchi via Gdb-patches <gdb-patches@sourceware.org>]

On 2021-02-03 12:50 a.m., Joel Brobecker wrote:
> Generally speaking, anything that touches purely on rnglist
> seems OK to backport because I understand this support is entirely
> new. I'm a little less sure about the changes to liclist support,
> though. Is there something I don't know that puts the changes to
> loclist handling in the same category as the rnglist changes?

I think the loc and rng changes fall in the same category, and kind of
mirror of each other.

Although there are loclist attributes in DWARF 4, the .debug_loclists
section is new to DWARF 5 (DWARF 4 had .debug_loc).  In the end, the
format of one location list hasn't changed much, but the format of the
section (the container of the location lists) changed quite a bit.  My
fixes have to do with parsing of the container, which is new stuff.

And it's pretty much the same with ranges, in DWARF 4, you had
.debug_ranges, in DWARF 5 you have .debug_rnglists.  My fixes have to do
with parsing the headers and finding the right list, not parsing the
lists themselves.

> Is it possible to skip some patches that are not strictly necessary,
> and if yes, would that actually be a good idea?
> 
> With that in mind, my best thoughts on the matter so far:
> 
>   [PATCH 01/13] gdb/dwarf: change read_loclist_index complaints into errors
> 
>         Although unnecessary, I think this one is fine, and perhaps
>         even desirable to avoid some weird behavior in GDB...

Yes, and if I am not mistaken, this is new DWARF 5-only code to read the
DW_FORM_loclistx form, so it shouldn't affect DWARF 4.

> 
>   [PATCH 02/13] gdb/dwarf: fix bound check in read_rnglist_index
> 
>         OK for gdb-10-branch

Same, only affects new DWARF 5 stuff.

> 
>   [PATCH 03/13] gdb/dwarf: add missing bound check to read_loclist_index
> 
>         Seems straightforward and adds safety; OK for gdb-10-branch.

Same.

> 
>   [PATCH 04/13] gdb/dwarf: remove unnecessary check in read_{rng,loc}list_index
> 
>         Maybe drop this patch, on the basis that this is just
>         a cleanup that should, in fine, be a no-op in practice.

Again, it only affects new DWARF 5 stuff.  But indeed it's really not
necessary, I'll skip it.

> 
>   [PATCH 05/13] gdb/dwarf: few fixes for handling DW_FORM_{rng,loc}listx
> 
>         After verification in the DWARF 5 standard that those are
>         unsigned ULEBs, this one looks good to me for gdb-10-branch.

Actually, this patch fixes a problem that was introduced by a refactor
that happened after GDB 10, the one that added methods on struct
attribute, 529908cbd0af ("Remove DW_UNSND").  So it's not relevant on
the 10 branch.

> 
>   [PATCH 06/13] gdb/dwarf: read correct rnglist/loclist header in read_{rng, loc}list_index
> 
>         I will need to trust you on that one, as I think I would need
>         to delve more deeply into DWARF 5, and I'm lacking the time
>         (at least until this weekend).

If that makes it feel safer, this code is only on the code path when
reading a DW_FORM_rnglistx or DW_FORM_loclistx attribute, which is new
DWARF 5 stuff.  It should not be invoked when reading DWARF 4.

And it is an important one that can't be skipped, it's the main fix of
the series.  Without it, let's say we have two compilation units, each
with a contribution to .debug_rnglists, when reading a DW_FORM_rnglistx
attribute from the second compilation unit, we read the header of the
.debug_rnglists contribution of the first compilation unit.

>   [PATCH 07/13] gdb/dwarf: read DW_AT_ranges value as unsigned in partial_die_info::read
> 
>         A little scarier for me, but the justification that we are
>         already doing this in dwarf2_get_pc_bounds is convincing.
>         OK for gdb-10-branch.

This one is also not relavant on the gdb-10-branch, because it fixes a
regression also introduced by the "Remove DW_UNSND" commit.


>   [PATCH 08/13] gdb/testsuite: add .debug_rnglists tests
>   [PATCH 09/13] gdb/testsuite: DWARF assembler: add context parameters to _location
>   [PATCH 10/13] gdb/testsuite: add .debug_loclists tests
> 
>         No problem for me.

Yeah, it's all tests, so it helps ensure that the backport works fine.

>   [PATCH 11/13] gdb/dwarf: split dwarf2_cu::ranges_base in two
> 
>         Do we need this? Looks like you are saying that this is
>         an enhancement for a case that is unlikely to happen
>         in practice?

Indeed, probably not relevant for a backport since it fixes a corner
case not likely to happen.

>   [PATCH 12/13] gdb/dwarf: make read_{loc, rng}list_index return sect_offset
> 
>         OK for gdb-10-branch, with perhaps a question regarding
>         the gains-vs-risks ratio?

I'll skip it, it doesn't change anything in practice.

> 
>   [PATCH 13/13] gdb/testsuite: add test for .debug_{rng, loc}lists section without offset array
> 
>         No problem for me.
> 

Ok.

So this is what I'll push after regtesting a bit:

  - gdb/dwarf: change read_loclist_index complaints into errors
  - gdb/dwarf: fix bound check in read_rnglist_index
  - gdb/dwarf: add missing bound check to read_loclist_index
  - gdb/dwarf: read correct rnglist/loclist header in read_{rng,loc}list_index
  - gdb/testsuite: add .debug_rnglists tests
  - gdb/testsuite: DWARF assembler: add context parameters to _location
  - gdb/testsuite: add .debug_loclists tests
  - gdb/testsuite: add test for .debug_{rng,loc}lists section without offset array

Thanks!

Simon