[PATCH] Fix issues with reading rnglists, especially from dwo files, for DWARF v5
Simon Marchi
simark@simark.ca
Thu Jul 2 05:41:06 GMT 2020
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 §ions->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
More information about the Gdb-patches
mailing list