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

[Caroline Tice <cmtice@google.com>]

[-- Attachment #1: Type: text/plain, Size: 21808 bytes --]

"

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
cmtice@google.com

2020-07-03  Caroline Tice  <cmtice@google.com>

        * 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  <cmtice@google.com>

        * 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 <simark@simark.ca> 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

[-- Attachment #2: v3-0001-Update-GDB-to-fix-issues-with-handling-DWARF-v5-r.patch --]
[-- Type: application/octet-stream, Size: 22558 bytes --]

From f75fbfd7a800cea7a4f1c5a43d40948061afdb82 Mon Sep 17 00:00:00 2001
From: Caroline Tice <cmtice@google.com>
Date: Wed, 1 Jul 2020 12:39:08 -0700
Subject: [PATCH v3] Update GDB to fix issues with handling DWARF v5 rnglists &
 .dwo files.

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.

- Added 'reprocessing' for reading rnglist index (as is done for loclist
  index).
- Added code for reading rnglists out of .dwo files.
- Added several missing rnglist forms to dwarf2_rnglists_process.
- Fixed bug that was alwayas adding base address for rnglists (only
  one form needs that).
- Updated dwarf2_rnglists_process to read rnglist out of dwo file when
  appropriate.
- Added new functions cu_debug_rnglist_section & read_rnglist_index.
- Added new testcase, dw5-rnglist-test.{cc,exp}
---
 gdb/dwarf2/read.c                             | 204 +++++++++++++++---
 gdb/testsuite/gdb.dwarf2/dw5-rnglist-test.cc  |  97 +++++++++
 gdb/testsuite/gdb.dwarf2/dw5-rnglist-test.exp |  40 ++++
 3 files changed, 313 insertions(+), 28 deletions(-)
 create mode 100644 gdb/testsuite/gdb.dwarf2/dw5-rnglist-test.cc
 create mode 100644 gdb/testsuite/gdb.dwarf2/dw5-rnglist-test.exp

diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index 4622d14a05..1d89c81e0d 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -340,6 +340,7 @@ static const struct dwop_section_names
   struct dwarf2_section_names loclists_dwo;
   struct dwarf2_section_names macinfo_dwo;
   struct dwarf2_section_names macro_dwo;
+  struct dwarf2_section_names rnglists_dwo;
   struct dwarf2_section_names str_dwo;
   struct dwarf2_section_names str_offsets_dwo;
   struct dwarf2_section_names types_dwo;
@@ -355,6 +356,7 @@ dwop_section_names =
   { ".debug_loclists.dwo", ".zdebug_loclists.dwo" },
   { ".debug_macinfo.dwo", ".zdebug_macinfo.dwo" },
   { ".debug_macro.dwo", ".zdebug_macro.dwo" },
+  { ".debug_rnglists.dwo", ".zdebug_rnglists.dwo" },
   { ".debug_str.dwo", ".zdebug_str.dwo" },
   { ".debug_str_offsets.dwo", ".zdebug_str_offsets.dwo" },
   { ".debug_types.dwo", ".zdebug_types.dwo" },
@@ -650,6 +652,7 @@ struct dwo_sections
   struct dwarf2_section_info loclists;
   struct dwarf2_section_info macinfo;
   struct dwarf2_section_info macro;
+  struct dwarf2_section_info rnglists;
   struct dwarf2_section_info str;
   struct dwarf2_section_info str_offsets;
   /* In the case of a virtual DWO file, these two are unused.  */
@@ -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);
 
 /* 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);
+
 /* How dwarf2_get_pc_bounds constructed its *LOWPC and *HIGHPC return
    values.  Keep the items ordered with increasing constraints compliance.  */
 enum pc_bounds_kind
@@ -12448,6 +12456,11 @@ dwarf2_locate_dwo_sections (bfd *abfd, asection *sectp, void *dwo_sections_ptr)
       dwo_sections->macro.s.section = sectp;
       dwo_sections->macro.size = bfd_section_size (sectp);
     }
+  else if (section_is_p (sectp->name, &names->rnglists_dwo))
+    {
+      dwo_sections->rnglists.s.section = sectp;
+      dwo_sections->rnglists.size = bfd_section_size (sectp);
+    }
   else if (section_is_p (sectp->name, &names->str_dwo))
     {
       dwo_sections->str.s.section = sectp;
@@ -13792,7 +13805,7 @@ read_variable (struct die_info *die, struct dwarf2_cu *cu)
 template <typename Callback>
 static bool
 dwarf2_rnglists_process (unsigned offset, struct dwarf2_cu *cu,
-			 Callback &&callback)
+			 dwarf_tag tag, Callback &&callback)
 {
   dwarf2_per_objfile *per_objfile = cu->per_objfile;
   struct objfile *objfile = per_objfile->objfile;
@@ -13802,17 +13815,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;
+
+  rnglists_section->read (objfile);
+  if (offset >= rnglists_section->size)
     {
       complaint (_("Offset %d out of bounds for DW_AT_ranges attribute"),
 		 offset);
       return false;
     }
-  buffer = per_objfile->per_bfd->rnglists.buffer + offset;
+  buffer = rnglists_section->buffer + offset;
 
   baseaddr = objfile->text_section_offset ();
 
@@ -13820,8 +13849,8 @@ dwarf2_rnglists_process (unsigned offset, struct dwarf2_cu *cu,
     {
       /* Initialize it due to a false compiler warning.  */
       CORE_ADDR range_beginning = 0, range_end = 0;
-      const gdb_byte *buf_end = (per_objfile->per_bfd->rnglists.buffer
-				 + per_objfile->per_bfd->rnglists.size);
+      const gdb_byte *buf_end = (rnglists_section->buffer
+				 + rnglists_section->size);
       unsigned int bytes_read;
 
       if (buffer == buf_end)
@@ -13843,6 +13872,11 @@ dwarf2_rnglists_process (unsigned offset, struct dwarf2_cu *cu,
 	  base = cu->header.read_address (obfd, buffer, &bytes_read);
 	  buffer += bytes_read;
 	  break;
+        case DW_RLE_base_addressx:
+          addr_index = read_unsigned_leb128 (obfd, buffer, &bytes_read);
+          buffer += bytes_read;
+          base = read_addr_index (cu, addr_index);
+          break;
 	case DW_RLE_start_length:
 	  if (buffer + cu->header.addr_size > buf_end)
 	    {
@@ -13861,6 +13895,19 @@ dwarf2_rnglists_process (unsigned offset, struct dwarf2_cu *cu,
 	      break;
 	    }
 	  break;
+	case DW_RLE_startx_length:
+          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;
+            }
+          range_end = (range_beginning
+                       + read_unsigned_leb128 (obfd, buffer, &bytes_read));
+          buffer += bytes_read;
+          break;
 	case DW_RLE_offset_pair:
 	  range_beginning = read_unsigned_leb128 (obfd, buffer, &bytes_read);
 	  buffer += bytes_read;
@@ -13889,6 +13936,19 @@ dwarf2_rnglists_process (unsigned offset, struct dwarf2_cu *cu,
 	  range_end = cu->header.read_address (obfd, buffer, &bytes_read);
 	  buffer += bytes_read;
 	  break;
+	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:
 	  complaint (_("Invalid .debug_rnglists data (no base address)"));
 	  return false;
@@ -13898,14 +13958,6 @@ dwarf2_rnglists_process (unsigned offset, struct dwarf2_cu *cu,
       if (rlet == DW_RLE_base_address)
 	continue;
 
-      if (!base.has_value ())
-	{
-	  /* We have no valid base address for the ranges
-	     data.  */
-	  complaint (_("Invalid .debug_rnglists data (no base address)"));
-	  return false;
-	}
-
       if (range_beginning > range_end)
 	{
 	  /* Inverted range entries are invalid.  */
@@ -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)"));
+	      return false;
+	    }
+
+	  range_beginning += *base;
+	  range_end += *base;
+	}
 
       /* A not-uncommon case of bad debug info.
 	 Don't pollute the addrmap with bad data.  */
@@ -13951,7 +14015,7 @@ dwarf2_rnglists_process (unsigned offset, struct dwarf2_cu *cu,
 
 template <typename Callback>
 static int
-dwarf2_ranges_process (unsigned offset, struct dwarf2_cu *cu,
+dwarf2_ranges_process (unsigned offset, struct dwarf2_cu *cu, dwarf_tag tag,
 		       Callback &&callback)
 {
   dwarf2_per_objfile *per_objfile = cu->per_objfile;
@@ -13967,7 +14031,7 @@ dwarf2_ranges_process (unsigned offset, struct dwarf2_cu *cu,
   CORE_ADDR baseaddr;
 
   if (cu_header->version >= 5)
-    return dwarf2_rnglists_process (offset, cu, callback);
+    return dwarf2_rnglists_process (offset, cu, tag, callback);
 
   base = cu->base_address;
 
@@ -14053,7 +14117,7 @@ dwarf2_ranges_process (unsigned offset, struct dwarf2_cu *cu,
 static int
 dwarf2_ranges_read (unsigned offset, CORE_ADDR *low_return,
 		    CORE_ADDR *high_return, struct dwarf2_cu *cu,
-		    dwarf2_psymtab *ranges_pst)
+		    dwarf2_psymtab *ranges_pst, dwarf_tag tag)
 {
   struct objfile *objfile = cu->per_objfile->objfile;
   struct gdbarch *gdbarch = objfile->arch ();
@@ -14063,7 +14127,7 @@ dwarf2_ranges_read (unsigned offset, CORE_ADDR *low_return,
   CORE_ADDR high = 0;
   int retval;
 
-  retval = dwarf2_ranges_process (offset, cu,
+  retval = dwarf2_ranges_process (offset, cu, tag,
     [&] (CORE_ADDR range_beginning, CORE_ADDR range_end)
     {
       if (ranges_pst != NULL)
@@ -14156,7 +14220,8 @@ dwarf2_get_pc_bounds (struct die_info *die, CORE_ADDR *lowpc,
 	  /* 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.  */
-	  int need_ranges_base = die->tag != DW_TAG_compile_unit;
+	  int need_ranges_base = (die->tag != DW_TAG_compile_unit
+				  && attr->form != DW_FORM_rnglistx);
 	  unsigned int ranges_offset = (DW_UNSND (attr)
 					+ (need_ranges_base
 					   ? cu->ranges_base
@@ -14164,7 +14229,8 @@ dwarf2_get_pc_bounds (struct die_info *die, CORE_ADDR *lowpc,
 
 	  /* Value of the DW_AT_ranges attribute is the offset in the
 	     .debug_ranges section.  */
-	  if (!dwarf2_ranges_read (ranges_offset, &low, &high, cu, pst))
+	  if (!dwarf2_ranges_read (ranges_offset, &low, &high, cu, pst,
+				   die->tag))
 	    return PC_BOUNDS_INVALID;
 	  /* Found discontinuous range of addresses.  */
 	  ret = PC_BOUNDS_RANGES;
@@ -14327,7 +14393,8 @@ dwarf2_record_block_ranges (struct die_info *die, struct block *block,
       /* 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.  */
-      int need_ranges_base = die->tag != DW_TAG_compile_unit;
+      int need_ranges_base = (die->tag != DW_TAG_compile_unit
+			      && attr->form != DW_FORM_rnglistx);
 
       /* The value of the DW_AT_ranges attribute is the offset of the
          address range list in the .debug_ranges section.  */
@@ -14335,7 +14402,7 @@ dwarf2_record_block_ranges (struct die_info *die, struct block *block,
 			      + (need_ranges_base ? cu->ranges_base : 0));
 
       std::vector<blockrange> blockvec;
-      dwarf2_ranges_process (offset, cu,
+      dwarf2_ranges_process (offset, cu, die->tag,
 	[&] (CORE_ADDR start, CORE_ADDR end)
 	{
 	  start += baseaddr;
@@ -18165,6 +18232,11 @@ read_full_die_1 (const struct die_reader_specs *reader,
   auto maybe_addr_base = die->addr_base ();
   if (maybe_addr_base.has_value ())
     cu->addr_base = *maybe_addr_base;
+
+  attr = die->attr (DW_AT_rnglists_base);
+  if (attr != nullptr)
+    cu->ranges_base = DW_UNSND (attr);
+
   for (int index : indexes_that_need_reprocess)
     read_attribute_reprocess (reader, &die->attrs[index]);
   *diep = die;
@@ -18684,7 +18756,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);
 	    unsigned int ranges_offset = (DW_UNSND (&attr)
 					  + (need_ranges_base
 					     ? cu->ranges_base
@@ -18693,7 +18766,7 @@ partial_die_info::read (const struct die_reader_specs *reader,
 	    /* Value of the DW_AT_ranges attribute is the offset in the
 	       .debug_ranges section.  */
 	    if (dwarf2_ranges_read (ranges_offset, &lowpc, &highpc, cu,
-				    nullptr))
+				    nullptr, tag))
 	      has_pc_info = 1;
 	  }
 	  break;
@@ -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);
+  ULONGEST rnglist_base =
+      (cu->dwo_unit) ? 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);
+  if (section == nullptr)
+    error (_("Cannot find .debug_rnglists section [in module %s]"),
+	   objfile_name(objfile));
+
+  /* Read the 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));
+
+  /* Verify the rnglist header is valid (same as loclist header).  */
+  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 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;
+}
+
 /* Process the attributes that had to be skipped in the first round. These
    attributes are the ones that need str_offsets_base or addr_base attributes.
    They could not have been processed in the first round, because at the time
@@ -19112,6 +19236,9 @@ read_attribute_reprocess (const struct die_reader_specs *reader,
       case DW_FORM_loclistx:
 	 DW_UNSND (attr) = read_loclist_index (cu, DW_UNSND (attr));
 	 break;
+      case DW_FORM_rnglistx:
+        DW_UNSND (attr) = read_rnglist_index (cu, DW_UNSND (attr));
+        break;
       case DW_FORM_strx:
       case DW_FORM_strx1:
       case DW_FORM_strx2:
@@ -19293,8 +19420,10 @@ read_attribute_value (const struct die_reader_specs *reader,
       DW_SND (attr) = read_signed_leb128 (abfd, info_ptr, &bytes_read);
       info_ptr += bytes_read;
       break;
-    case DW_FORM_udata:
     case DW_FORM_rnglistx:
+      *need_reprocess = true;
+      /* FALLTHROUGH */
+    case DW_FORM_udata:
       DW_UNSND (attr) = read_unsigned_leb128 (abfd, info_ptr, &bytes_read);
       info_ptr += bytes_read;
       break;
@@ -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_rnglists_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 != 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;
+}
+
 /* A helper function that fills in a dwarf2_loclist_baton.  */
 
 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..81693f569d
--- /dev/null
+++ b/gdb/testsuite/gdb.dwarf2/dw5-rnglist-test.cc
@@ -0,0 +1,97 @@
+/* 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;
+}
diff --git a/gdb/testsuite/gdb.dwarf2/dw5-rnglist-test.exp b/gdb/testsuite/gdb.dwarf2/dw5-rnglist-test.exp
new file mode 100644
index 0000000000..af6c34b5dd
--- /dev/null
+++ b/gdb/testsuite/gdb.dwarf2/dw5-rnglist-test.exp
@@ -0,0 +1,40 @@
+# 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/>.
+
+# Check that GDB can find the variables in a lexical block with a
+# DW_FORM_rnglistx DW_AT_ranges field.  This test is intended for DWARF-5,
+# compiled with clang++.
+
+standard_testfile .cc
+
+# This test is intended for targets which support DWARF-5.
+# Since we pass an explicit -gdwarf-5 to the compiler,
+# we let that be the test of whether the target supports it.
+
+if { [prepare_for_testing "failed to prepare" "${testfile}" \
+          $srcfile {debug c++ additional_flags=-gdwarf-5 \
+                        additional_flags=-O0}] } {
+    return -1
+}
+
+if ![runto_main] {
+    return -1
+}
+
+gdb_breakpoint [gdb_get_line_number "break-here"]
+gdb_continue_to_breakpoint "break-here" ".* break-here .*"
+
+gdb_test "print curr" "\\\(node \\\*\\\) $hex <node_array>"
+gdb_test "print *curr" "= {id = 35, left = $hex <node_array\\+$decimal>, right = $hex <node_array\\+$decimal>, visited = false}"
-- 
2.27.0.212.ge8ba1cc988-goog