"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 cmtice@google.com gdb/ChangeLog 2020-07-14 Caroline Tice * 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 * gdb.dwarf2/dw5-rnglist-test.cc: New file. * gdb.dwarf2/dw5-rnglist-test.exp: New file. -- Caroline cmtice@google.com On Sat, Jul 11, 2020 at 10:54 AM Simon Marchi 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 §ions->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