[RFC PATCH] Introduce dwarf2_cu::get_builder

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

[RFC PATCH] Introduce dwarf2_cu::get_builder

Keith Seitz
This patch is an attempt to deal with a variety of bugs reported where
GDB segfaults attempting to access a dwarf2_cu's builder.  In certain
circumstances, this builder can be NULL.

The approach taken here is to save the ancestor CU into the dwarf2_cu of
all CUs with DIEs that are "imported."  This can happen whenever
follow_die_offset and friends are called.  This essentially introduces a
chain of CUs that caused the importation of a DIE from a CU.  Whenever
a builder is requested of a CU that has none, the ancestors are searched
for the first one with a builder.

The bulk of the patch is relatively mindless text conversion from
"cu->builder" to "cu->get_builder ()".  I've included one test which
was derived from one (of the many) bugs reported on the issue in both
sourceware and Fedora bugzillas.

I'm submitting this as an RFC rather than an actual patch because of the
lack of coverage testing for all the places where get_builder() is used.

gdb/ChangeLog:

        PR gdb/23773
        * dwarf2read.c (dwarf2_cu) <ancestor>: New field.
        (dwarf2_cu::get_builder): New method.  Change all users of
        `builder' to use this method.
        (follow_die_offset): Record the ancestor CU if it is different
        from the followed DIE's CU.
        (follow_die_sig_1): Likewise.

gdb/testsuite/ChangeLog:

        PR gdb/23773
        * gdb.dwarf2/inline_subroutine-inheritance.exp: New file.
---
 gdb/ChangeLog                                 |  10 +
 gdb/dwarf2read.c                              | 154 ++++++++-----
 gdb/testsuite/ChangeLog                       |   5 +
 .../inline_subroutine-inheritance.exp         | 213 ++++++++++++++++++
 4 files changed, 321 insertions(+), 61 deletions(-)
 create mode 100644 gdb/testsuite/gdb.dwarf2/inline_subroutine-inheritance.exp

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 0e9b0a1a5f..bfc2554688 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,13 @@
+YYYY-MM-DD  Keith Seitz  <[hidden email]>
+
+ PR gdb/23773
+ * dwarf2read.c (dwarf2_cu) <ancestor>: New field.
+ (dwarf2_cu::get_builder): New method.  Change all users of
+ `builder' to use this method.
+ (follow_die_offset): Record the ancestor CU if it is different
+ from the followed DIE's CU.
+ (follow_die_sig_1): Likewise.
+
 2018-10-21  Simon Marchi  <[hidden email]>
 
  * gdbarch.sh (gdbarch_num_cooked_regs): New.
diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index 2a1b805c9a..8981bbbedf 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -561,6 +561,24 @@ struct dwarf2_cu
   unsigned int processing_has_namespace_info : 1;
 
   struct partial_die_info *find_partial_die (sect_offset sect_off);
+
+  /* If this CU was inherited by another CU (via specification,
+     abstract_origin, etc), this is the ancestor CU.  */
+  dwarf2_cu *ancestor;
+
+  /* Get the buildsym_compunit for this CU.  */
+  buildsym_compunit *get_builder ()
+  {
+    /* If this CU has a builder associated with it, use that.  */
+    if (builder != nullptr)
+      return builder.get ();
+
+    /* Otherwise, search ancestors for a valid builder.  */
+    if (ancestor != nullptr)
+      return ancestor->get_builder ();
+
+    return nullptr;
+  }
 };
 
 /* A struct that can be used as a hash key for tables based on DW_AT_stmt_list.
@@ -9822,7 +9840,7 @@ fixup_go_packaging (struct dwarf2_cu *cu)
   struct pending *list;
   int i;
 
-  for (list = *cu->builder->get_global_symbols ();
+  for (list = *cu->get_builder ()->get_global_symbols ();
        list != NULL;
        list = list->next)
     {
@@ -10377,7 +10395,7 @@ process_full_comp_unit (struct dwarf2_per_cu_data *per_cu,
   get_scope_pc_bounds (cu->dies, &lowpc, &highpc, cu);
 
   addr = gdbarch_adjust_dwarf2_addr (gdbarch, highpc + baseaddr);
-  static_block = cu->builder->end_symtab_get_static_block (addr, 0, 1);
+  static_block = cu->get_builder ()->end_symtab_get_static_block (addr, 0, 1);
 
   /* If the comp unit has DW_AT_ranges, it may have discontiguous ranges.
      Also, DW_AT_ranges may record ranges not belonging to any child DIEs
@@ -10386,7 +10404,7 @@ process_full_comp_unit (struct dwarf2_per_cu_data *per_cu,
      this comp unit.  */
   dwarf2_record_block_ranges (cu->dies, static_block, baseaddr, cu);
 
-  cust = cu->builder->end_symtab_from_static_block (static_block,
+  cust = cu->get_builder ()->end_symtab_from_static_block (static_block,
     SECT_OFF_TEXT (objfile),
     0);
 
@@ -10481,7 +10499,8 @@ process_full_type_unit (struct dwarf2_per_cu_data *per_cu,
      this TU's symbols to the existing symtab.  */
   if (sig_type->type_unit_group->compunit_symtab == NULL)
     {
-      cust = cu->builder->end_expandable_symtab (0, SECT_OFF_TEXT (objfile));
+      buildsym_compunit *builder = cu->get_builder ();
+      cust = builder->end_expandable_symtab (0, SECT_OFF_TEXT (objfile));
       sig_type->type_unit_group->compunit_symtab = cust;
 
       if (cust != NULL)
@@ -10497,7 +10516,7 @@ process_full_type_unit (struct dwarf2_per_cu_data *per_cu,
     }
   else
     {
-      cu->builder->augment_type_symtab ();
+      cu->get_builder ()->augment_type_symtab ();
       cust = sig_type->type_unit_group->compunit_symtab;
     }
 
@@ -11206,10 +11225,11 @@ read_namespace_alias (struct die_info *die, struct dwarf2_cu *cu)
 static struct using_direct **
 using_directives (struct dwarf2_cu *cu)
 {
-  if (cu->language == language_ada && cu->builder->outermost_context_p ())
-    return cu->builder->get_global_using_directives ();
+  if (cu->language == language_ada
+      && cu->get_builder ()->outermost_context_p ())
+    return cu->get_builder ()->get_global_using_directives ();
   else
-    return cu->builder->get_local_using_directives ();
+    return cu->get_builder ()->get_local_using_directives ();
 }
 
 /* Read the import statement specified by the given die and record it.  */
@@ -11681,20 +11701,20 @@ setup_type_unit_groups (struct die_info *die, struct dwarf2_cu *cu)
   file_entry &fe = cu->line_header->file_names[i];
 
   dwarf2_start_subfile (cu, fe.name, fe.include_dir (cu->line_header));
-
-  if (cu->builder->get_current_subfile ()->symtab == NULL)
+  buildsym_compunit *builder = cu->get_builder ();
+  if (builder->get_current_subfile ()->symtab == NULL)
     {
       /* NOTE: start_subfile will recognize when it's been
  passed a file it has already seen.  So we can't
  assume there's a simple mapping from
  cu->line_header->file_names to subfiles, plus
  cu->line_header->file_names may contain dups.  */
-      cu->builder->get_current_subfile ()->symtab
+      builder->get_current_subfile ()->symtab
  = allocate_symtab (cust,
-   cu->builder->get_current_subfile ()->name);
+   builder->get_current_subfile ()->name);
     }
 
-  fe.symtab = cu->builder->get_current_subfile ()->symtab;
+  fe.symtab = builder->get_current_subfile ()->symtab;
   tu_group->symtabs[i] = fe.symtab;
  }
     }
@@ -13731,7 +13751,7 @@ read_func_scope (struct die_info *die, struct dwarf2_cu *cu)
  }
     }
 
-  newobj = cu->builder->push_context (0, lowpc);
+  newobj = cu->get_builder ()->push_context (0, lowpc);
   newobj->name = new_symbol (die, read_type_die (die, cu), cu,
      (struct symbol *) templ_func);
 
@@ -13751,7 +13771,7 @@ read_func_scope (struct die_info *die, struct dwarf2_cu *cu)
       attr_to_dynamic_prop (attr, die, cu, newobj->static_link);
     }
 
-  cu->list_in_scope = cu->builder->get_local_symbols ();
+  cu->list_in_scope = cu->get_builder ()->get_local_symbols ();
 
   if (die->child != NULL)
     {
@@ -13799,9 +13819,9 @@ read_func_scope (struct die_info *die, struct dwarf2_cu *cu)
  }
     }
 
-  struct context_stack cstk = cu->builder->pop_context ();
+  struct context_stack cstk = cu->get_builder ()->pop_context ();
   /* Make a block for the local symbols within.  */
-  block = cu->builder->finish_block (cstk.name, cstk.old_blocks,
+  block = cu->get_builder ()->finish_block (cstk.name, cstk.old_blocks,
      cstk.static_link, lowpc, highpc);
 
   /* For C++, set the block's scope.  */
@@ -13843,13 +13863,13 @@ read_func_scope (struct die_info *die, struct dwarf2_cu *cu)
      a function declares a class that has methods).  This means that
      when we finish processing a function scope, we may need to go
      back to building a containing block's symbol lists.  */
-  *cu->builder->get_local_symbols () = cstk.locals;
-  cu->builder->set_local_using_directives (cstk.local_using_directives);
+  *cu->get_builder ()->get_local_symbols () = cstk.locals;
+  cu->get_builder ()->set_local_using_directives (cstk.local_using_directives);
 
   /* If we've finished processing a top-level function, subsequent
      symbols go in the file symbol list.  */
-  if (cu->builder->outermost_context_p ())
-    cu->list_in_scope = cu->builder->get_file_symbols ();
+  if (cu->get_builder ()->outermost_context_p ())
+    cu->list_in_scope = cu->get_builder ()->get_file_symbols ();
 }
 
 /* Process all the DIES contained within a lexical block scope.  Start
@@ -13888,7 +13908,7 @@ read_lexical_block_scope (struct die_info *die, struct dwarf2_cu *cu)
   lowpc = gdbarch_adjust_dwarf2_addr (gdbarch, lowpc + baseaddr);
   highpc = gdbarch_adjust_dwarf2_addr (gdbarch, highpc + baseaddr);
 
-  cu->builder->push_context (0, lowpc);
+  cu->get_builder ()->push_context (0, lowpc);
   if (die->child != NULL)
     {
       child_die = die->child;
@@ -13899,13 +13919,13 @@ read_lexical_block_scope (struct die_info *die, struct dwarf2_cu *cu)
  }
     }
   inherit_abstract_dies (die, cu);
-  struct context_stack cstk = cu->builder->pop_context ();
+  struct context_stack cstk = cu->get_builder ()->pop_context ();
 
-  if (*cu->builder->get_local_symbols () != NULL
-      || (*cu->builder->get_local_using_directives ()) != NULL)
+  if (*cu->get_builder ()->get_local_symbols () != NULL
+      || (*cu->get_builder ()->get_local_using_directives ()) != NULL)
     {
       struct block *block
-        = cu->builder->finish_block (0, cstk.old_blocks, NULL,
+        = cu->get_builder ()->finish_block (0, cstk.old_blocks, NULL,
      cstk.start_addr, highpc);
 
       /* Note that recording ranges after traversing children, as we
@@ -13920,8 +13940,8 @@ read_lexical_block_scope (struct die_info *die, struct dwarf2_cu *cu)
          to do.  */
       dwarf2_record_block_ranges (die, block, baseaddr, cu);
     }
-  *cu->builder->get_local_symbols () = cstk.locals;
-  cu->builder->set_local_using_directives (cstk.local_using_directives);
+  *cu->get_builder ()->get_local_symbols () = cstk.locals;
+  cu->get_builder ()->set_local_using_directives (cstk.local_using_directives);
 }
 
 /* Read in DW_TAG_call_site and insert it to CU->call_site_htab.  */
@@ -14844,7 +14864,7 @@ dwarf2_record_block_ranges (struct die_info *die, struct block *block,
 
   low = gdbarch_adjust_dwarf2_addr (gdbarch, low + baseaddr);
   high = gdbarch_adjust_dwarf2_addr (gdbarch, high + baseaddr);
-  cu->builder->record_block_range (block, low, high - 1);
+  cu->get_builder ()->record_block_range (block, low, high - 1);
         }
     }
 
@@ -14869,7 +14889,7 @@ dwarf2_record_block_ranges (struct die_info *die, struct block *block,
   end += baseaddr;
   start = gdbarch_adjust_dwarf2_addr (gdbarch, start);
   end = gdbarch_adjust_dwarf2_addr (gdbarch, end);
-  cu->builder->record_block_range (block, start, end - 1);
+  cu->get_builder ()->record_block_range (block, start, end - 1);
   blockvec.emplace_back (start, end);
  });
 
@@ -20666,7 +20686,7 @@ lnp_state_machine::handle_set_file (file_name_index file)
     {
       const char *dir = fe->include_dir (m_line_header);
 
-      m_last_subfile = m_cu->builder->get_current_subfile ();
+      m_last_subfile = m_cu->get_builder ()->get_current_subfile ();
       m_line_has_non_zero_discriminator = m_discriminator != 0;
       dwarf2_start_subfile (m_cu, fe->name, dir);
     }
@@ -20724,7 +20744,7 @@ dwarf_record_line_p (struct dwarf2_cu *cu,
      int line_has_non_zero_discriminator,
      struct subfile *last_subfile)
 {
-  if (cu->builder->get_current_subfile () != last_subfile)
+  if (cu->get_builder ()->get_current_subfile () != last_subfile)
     return 1;
   if (line != last_line)
     return 1;
@@ -20755,7 +20775,7 @@ dwarf_record_line_1 (struct gdbarch *gdbarch, struct subfile *subfile,
     }
 
   if (cu != nullptr)
-    cu->builder->record_line (subfile, line, addr);
+    cu->get_builder ()->record_line (subfile, line, addr);
 }
 
 /* Subroutine of dwarf_decode_lines_1 to simplify it.
@@ -20806,7 +20826,7 @@ lnp_state_machine::record_line (bool end_sequence)
       fe->included_p = 1;
       if (m_record_lines_p && (producer_is_codewarrior (m_cu) || m_is_stmt))
  {
-  if (m_last_subfile != m_cu->builder->get_current_subfile ()
+  if (m_last_subfile != m_cu->get_builder ()->get_current_subfile ()
       || end_sequence)
     {
       dwarf_finish_line (m_gdbarch, m_last_subfile, m_address,
@@ -20819,12 +20839,13 @@ lnp_state_machine::record_line (bool end_sequence)
        m_line_has_non_zero_discriminator,
        m_last_subfile))
  {
+  buildsym_compunit *builder = m_cu->get_builder ();
   dwarf_record_line_1 (m_gdbarch,
-       m_cu->builder->get_current_subfile (),
+       builder->get_current_subfile (),
        m_line, m_address,
        m_currently_recording_lines ? m_cu : nullptr);
  }
-      m_last_subfile = m_cu->builder->get_current_subfile ();
+      m_last_subfile = m_cu->get_builder ()->get_current_subfile ();
       m_last_line = m_line;
     }
  }
@@ -21147,7 +21168,8 @@ dwarf_decode_lines (struct line_header *lh, const char *comp_dir,
       /* Make sure a symtab is created for every file, even files
  which contain only variables (i.e. no code with associated
  line numbers).  */
-      struct compunit_symtab *cust = cu->builder->get_compunit_symtab ();
+      buildsym_compunit *builder = cu->get_builder ();
+      struct compunit_symtab *cust = builder->get_compunit_symtab ();
       int i;
 
       for (i = 0; i < lh->file_names.size (); i++)
@@ -21156,13 +21178,13 @@ dwarf_decode_lines (struct line_header *lh, const char *comp_dir,
 
   dwarf2_start_subfile (cu, fe.name, fe.include_dir (lh));
 
-  if (cu->builder->get_current_subfile ()->symtab == NULL)
+  if (builder->get_current_subfile ()->symtab == NULL)
     {
-      cu->builder->get_current_subfile ()->symtab
+      builder->get_current_subfile ()->symtab
  = allocate_symtab (cust,
-   cu->builder->get_current_subfile ()->name);
+   builder->get_current_subfile ()->name);
     }
-  fe.symtab = cu->builder->get_current_subfile ()->symtab;
+  fe.symtab = builder->get_current_subfile ()->symtab;
  }
     }
 }
@@ -21209,7 +21231,7 @@ dwarf2_start_subfile (struct dwarf2_cu *cu, const char *filename,
       filename = copy;
     }
 
-  cu->builder->start_subfile (filename);
+  cu->get_builder ()->start_subfile (filename);
 
   if (copy != NULL)
     xfree (copy);
@@ -21228,14 +21250,14 @@ dwarf2_start_symtab (struct dwarf2_cu *cu,
      (cu->per_cu->dwarf2_per_objfile->objfile,
       name, comp_dir, cu->language, low_pc));
 
-  cu->list_in_scope = cu->builder->get_file_symbols ();
+  cu->list_in_scope = cu->get_builder ()->get_file_symbols ();
 
-  cu->builder->record_debugformat ("DWARF 2");
-  cu->builder->record_producer (cu->producer);
+  cu->get_builder ()->record_debugformat ("DWARF 2");
+  cu->get_builder ()->record_producer (cu->producer);
 
   cu->processing_has_namespace_info = 0;
 
-  return cu->builder->get_compunit_symtab ();
+  return cu->get_builder ()->get_compunit_symtab ();
 }
 
 static void
@@ -21421,7 +21443,7 @@ new_symbol (struct die_info *die, struct type *type, struct dwarf2_cu *cu,
                  access them globally.  For instance, we want to be able
                  to break on a nested subprogram without having to
                  specify the context.  */
-      list_to_add = cu->builder->get_global_symbols ();
+      list_to_add = cu->get_builder ()->get_global_symbols ();
     }
   else
     {
@@ -21464,7 +21486,7 @@ new_symbol (struct die_info *die, struct type *type, struct dwarf2_cu *cu,
       if (!suppress_add)
  {
   if (attr2 && (DW_UNSND (attr2) != 0))
-    list_to_add = cu->builder->get_global_symbols ();
+    list_to_add = cu->get_builder ()->get_global_symbols ();
   else
     list_to_add = cu->list_in_scope;
  }
@@ -21510,8 +21532,9 @@ new_symbol (struct die_info *die, struct type *type, struct dwarf2_cu *cu,
   /* A variable with DW_AT_external is never static,
      but it may be block-scoped.  */
   list_to_add
-    = (cu->list_in_scope == cu->builder->get_file_symbols ()
-       ? cu->builder->get_global_symbols ()
+    = ((cu->list_in_scope
+ == cu->get_builder ()->get_file_symbols ())
+       ? cu->get_builder ()->get_global_symbols ()
        : cu->list_in_scope);
  }
       else
@@ -21543,8 +21566,9 @@ new_symbol (struct die_info *die, struct type *type, struct dwarf2_cu *cu,
   /* A variable with DW_AT_external is never static, but it
      may be block-scoped.  */
   list_to_add
-    = (cu->list_in_scope == cu->builder->get_file_symbols ()
-       ? cu->builder->get_global_symbols ()
+    = ((cu->list_in_scope
+ == cu->get_builder ()->get_file_symbols ())
+       ? cu->get_builder ()->get_global_symbols ()
        : cu->list_in_scope);
 
   SYMBOL_ACLASS_INDEX (sym) = LOC_UNRESOLVED;
@@ -21566,7 +21590,7 @@ new_symbol (struct die_info *die, struct type *type, struct dwarf2_cu *cu,
        pretend it's a local variable in that case so that the user can
        still see it.  */
     struct context_stack *curr
-      = cu->builder->get_current_context_stack ();
+      = cu->get_builder ()->get_current_context_stack ();
     if (curr != nullptr && curr->name != nullptr)
       SYMBOL_IS_ARGUMENT (sym) = 1;
     attr = dwarf2_attr (die, DW_AT_location, cu);
@@ -21611,10 +21635,11 @@ new_symbol (struct die_info *die, struct type *type, struct dwarf2_cu *cu,
 
     if (!suppress_add)
       {
+ buildsym_compunit *builder = cu->get_builder ();
  list_to_add
-  = (cu->list_in_scope == cu->builder->get_file_symbols ()
+  = (cu->list_in_scope == builder->get_file_symbols ()
      && cu->language == language_cplus
-     ? cu->builder->get_global_symbols ()
+     ? builder->get_global_symbols ()
      : cu->list_in_scope);
 
  /* The semantics of C++ state that "struct foo {
@@ -21655,21 +21680,21 @@ new_symbol (struct die_info *die, struct type *type, struct dwarf2_cu *cu,
        DW_TAG_class_type, etc. block.  */
 
     list_to_add
-      = (cu->list_in_scope == cu->builder->get_file_symbols ()
+      = (cu->list_in_scope == cu->get_builder ()->get_file_symbols ()
  && cu->language == language_cplus
- ? cu->builder->get_global_symbols ()
+ ? cu->get_builder ()->get_global_symbols ()
  : cu->list_in_scope);
   }
   break;
  case DW_TAG_imported_declaration:
  case DW_TAG_namespace:
   SYMBOL_ACLASS_INDEX (sym) = LOC_TYPEDEF;
-  list_to_add = cu->builder->get_global_symbols ();
+  list_to_add = cu->get_builder ()->get_global_symbols ();
   break;
  case DW_TAG_module:
   SYMBOL_ACLASS_INDEX (sym) = LOC_TYPEDEF;
   SYMBOL_DOMAIN (sym) = MODULE_DOMAIN;
-  list_to_add = cu->builder->get_global_symbols ();
+  list_to_add = cu->get_builder ()->get_global_symbols ();
   break;
  case DW_TAG_common_block:
   SYMBOL_ACLASS_INDEX (sym) = LOC_COMMON_BLOCK;
@@ -23001,6 +23026,10 @@ follow_die_offset (sect_offset sect_off, int offset_in_dwz,
 
   *ref_cu = target_cu;
   temp_die.sect_off = sect_off;
+
+  if (target_cu != cu)
+    target_cu->ancestor = cu;
+
   return (struct die_info *) htab_find_with_hash (target_cu->die_hash,
   &temp_die,
   to_underlying (sect_off));
@@ -23335,7 +23364,7 @@ follow_die_sig_1 (struct die_info *src_die, struct signatured_type *sig_type,
   struct dwarf2_cu **ref_cu)
 {
   struct die_info temp_die;
-  struct dwarf2_cu *sig_cu;
+  struct dwarf2_cu *sig_cu, *cu = *ref_cu;
   struct die_info *die;
 
   /* While it might be nice to assert sig_type->type == NULL here,
@@ -23369,6 +23398,9 @@ follow_die_sig_1 (struct die_info *src_die, struct signatured_type *sig_type,
  }
 
       *ref_cu = sig_cu;
+      if (sig_cu != cu)
+ sig_cu->ancestor = cu;
+
       return die;
     }
 
@@ -23945,7 +23977,7 @@ macro_start_file (struct dwarf2_cu *cu,
     {
       /* Note: We don't create a macro table for this compilation unit
  at all until we actually get a filename.  */
-      struct macro_table *macro_table = cu->builder->get_macro_table ();
+      struct macro_table *macro_table = cu->get_builder ()->get_macro_table ();
 
       /* If we have no current file, then this must be the start_file
  directive for the compilation unit's main source file.  */
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index a917a6d04c..15a2fbaf8c 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,8 @@
+YYYY-MM-DD  Keith Seitz  <[hidden email]>
+
+ PR gdb/23773
+ * gdb.dwarf2/inline_subroutine-inheritance.exp: New file.
+
 2018-10-19  Alan Hayward  <[hidden email]>
 
  * gdb.python/py-cmd.exp: Check for gdb_prompt.
diff --git a/gdb/testsuite/gdb.dwarf2/inline_subroutine-inheritance.exp b/gdb/testsuite/gdb.dwarf2/inline_subroutine-inheritance.exp
new file mode 100644
index 0000000000..92c5d0529b
--- /dev/null
+++ b/gdb/testsuite/gdb.dwarf2/inline_subroutine-inheritance.exp
@@ -0,0 +1,213 @@
+# Copyright 2018 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/>.
+
+# This tests a segfault that occurs when reading inline_subroutine DIEs
+# with abstract_origins pointing to DIEs in other CUs.
+#
+# See https://bugzilla.redhat.com/show_bug.cgi?id=1638798 .
+
+load_lib dwarf.exp
+
+# This test can only be run on targets which support DWARF.
+if {![dwarf2_support]} {
+    return 0
+}
+
+standard_testfile main.c .S
+
+# Create the DWARF.  This is derived from the reproducer in the Fedora
+# bugzila mentioned above.  For clarity, some "superfluous" DIES have
+# been left instead of simplifying/pruning the test further.
+set asm_file [standard_output_file $srcfile2]
+Dwarf::assemble $asm_file {
+    declare_labels Db D72f8 D736e
+    declare_labels D266465 D266477 D266483 D266496 D266498 D266ad3 D266ad9 \
+ D266ade D26b227 D26b237
+    declare_labels D26d8b1 D26d8c3 D26d8cf D26d944 D26d946 D26e103 D26e145 \
+ D26e415 D26e48c D26df00 D26df06 D26df0b D272519 D274c1a D274c42
+
+    cu {} {
+ Db: compile_unit {
+    {language @DW_LANG_C99}
+    {name "<artificial>"}
+ } {
+    D72f8: subprogram {
+ {abstract_origin :$D272519}
+ {low_pc 0xb9e20 addr}
+ {high_pc 0x1f5 data4}
+    } {
+ D736e: inlined_subroutine {
+    {abstract_origin :$D26b227}
+    {low_pc 0xb9efc addr}
+    {high_pc 0xc data4}
+ } {
+    formal_parameter {
+ {abstract_origin :$D274c42}
+    }
+ }
+    }
+ }
+    }
+
+    cu {} {
+ D266465: compile_unit {
+    {language @DW_LANG_C99}
+ } {
+    D266477: typedef {
+ {name "size_t"}
+ {type :$D266483}
+    }
+
+    D266483: base_type {
+ {byte_size 8 sdata}
+ {encoding @DW_ATE_unsigned}
+    }
+
+    D266496: pointer_type {
+ {byte_size 8 sdata}
+    }
+
+    D266498: restrict_type {
+ {type :$D266496}
+    }
+
+    D266ad3: pointer_type {
+ {byte_size 8 sdata}
+ {type :$D266ade}
+    }
+
+    D266ad9: restrict_type {
+ {type :$D266ad3}
+    }
+
+    D266ade: const_type {}
+
+    D26b227: subprogram {
+ {external 1 flag}
+ {name "memcpy"}
+ {type :$D266496}
+    } {
+ D26b237: formal_parameter {
+    {name "__dest"}
+    {type :$D266498}
+ }
+ formal_parameter {
+    {name "__src"}
+    {type :$D266ad9}
+ }
+ formal_parameter {
+    {name "__len"}
+    {type :$D266477}
+ }
+    }
+ }
+    }
+
+    cu {} {
+ D26d8b1: compile_unit {
+    {language @DW_LANG_C99}
+ } {
+    D26d8c3: typedef {
+ {name "size_t"}
+ {type :$D26d8cf}
+    }
+
+    D26d8cf: base_type {
+ {byte_size 8 sdata}
+ {encoding @DW_ATE_unsigned}
+ {name "long unsigned int"}
+    }
+
+    D26d944: pointer_type {
+ {byte_size 8 sdata}
+    }
+
+    D26d946: restrict_type {
+ {type :$D26d944}
+    }
+
+    D26e103: structure_type {
+ {name "__object"}
+ {byte_size 12 sdata}
+    } {
+ member {
+    {name "__ob_next"}
+    {type :$D26e145}
+    {data_member_location 0 sdata}
+ }
+    }
+
+    D26e145: pointer_type {
+ {byte_size 8 sdata}
+ {type :$D26e103}
+    }
+
+    D26e415: typedef {
+ {name "PyObject"}
+ {type :$D26e103}
+    }
+
+    D26e48c: pointer_type {
+ {byte_size 8 sdata}
+ {type :$D26e415}
+    }
+
+    D26df00: pointer_type {
+ {byte_size 8 sdata}
+ {type :$D26df0b}
+    }
+
+    D26df06: restrict_type {
+ {type :$D26df00}
+    }
+
+    D26df0b: const_type {}
+
+    D272519: subprogram {
+ {name "bytes_repeat"}
+ {type :$D26e48c}
+    }
+
+    D274c1a: subprogram {
+ {external 1 flag}
+ {name "memcpy"}
+ {type :$D26d944}
+    } {
+ formal_parameter {
+    {name "__dest"}
+    {type :$D26d946}
+ }
+ formal_parameter {
+    {name "__src"}
+    {type :$D26df06}
+ }
+ D274c42: formal_parameter {
+    {name "__len"}
+    {type :$D26d8c3}
+ }
+    }
+ }
+    }
+}
+
+if {[prepare_for_testing "failed to prepare" $testfile \
+ "${asm_file} ${srcfile}" {}]} {
+    return -1
+}
+
+# All we need to do is set a breakpoint, which causes the DWARF
+# info to be read, to demonstrate the problem.
+
+gdb_breakpoint "bytes_repeat" message
--
2.17.2

Reply | Threaded
Open this post in threaded view
|

Re: [RFC PATCH] Introduce dwarf2_cu::get_builder

Sergio Durigan Junior
On Tuesday, October 23 2018, Keith Seitz wrote:

> This patch is an attempt to deal with a variety of bugs reported where
> GDB segfaults attempting to access a dwarf2_cu's builder.  In certain
> circumstances, this builder can be NULL.
>
> The approach taken here is to save the ancestor CU into the dwarf2_cu of
> all CUs with DIEs that are "imported."  This can happen whenever
> follow_die_offset and friends are called.  This essentially introduces a
> chain of CUs that caused the importation of a DIE from a CU.  Whenever
> a builder is requested of a CU that has none, the ancestors are searched
> for the first one with a builder.
>
> The bulk of the patch is relatively mindless text conversion from
> "cu->builder" to "cu->get_builder ()".  I've included one test which
> was derived from one (of the many) bugs reported on the issue in both
> sourceware and Fedora bugzillas.

Thanks for the patch.

As we were discussing on IRC: I think that "builder" can become a
private member of the struct, since, IIUC, it's not supposed to be
accessed directly anymore.

The patch also makes sense to me, but I'm far from an expert in this
area to offer a definitive review.

> I'm submitting this as an RFC rather than an actual patch because of the
> lack of coverage testing for all the places where get_builder() is used.

It's hard to know when to stop.  Complete coverage may not be even
humanly possible, given that we've been receiving bug reports for
scenarios that you didn't think about.  IMHO, if you can create a
reproducer for the bug (which you did) and the patch fixes all the
*known* problems, then it may be OK to go...?  But I'm not a maintainer.

Thanks,

> gdb/ChangeLog:
>
> PR gdb/23773
> * dwarf2read.c (dwarf2_cu) <ancestor>: New field.
> (dwarf2_cu::get_builder): New method.  Change all users of
> `builder' to use this method.
> (follow_die_offset): Record the ancestor CU if it is different
> from the followed DIE's CU.
> (follow_die_sig_1): Likewise.
>
> gdb/testsuite/ChangeLog:
>
> PR gdb/23773
> * gdb.dwarf2/inline_subroutine-inheritance.exp: New file.
> ---
>  gdb/ChangeLog                                 |  10 +
>  gdb/dwarf2read.c                              | 154 ++++++++-----
>  gdb/testsuite/ChangeLog                       |   5 +
>  .../inline_subroutine-inheritance.exp         | 213 ++++++++++++++++++
>  4 files changed, 321 insertions(+), 61 deletions(-)
>  create mode 100644 gdb/testsuite/gdb.dwarf2/inline_subroutine-inheritance.exp
>
> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> index 0e9b0a1a5f..bfc2554688 100644
> --- a/gdb/ChangeLog
> +++ b/gdb/ChangeLog
> @@ -1,3 +1,13 @@
> +YYYY-MM-DD  Keith Seitz  <[hidden email]>
> +
> + PR gdb/23773
> + * dwarf2read.c (dwarf2_cu) <ancestor>: New field.
> + (dwarf2_cu::get_builder): New method.  Change all users of
> + `builder' to use this method.
> + (follow_die_offset): Record the ancestor CU if it is different
> + from the followed DIE's CU.
> + (follow_die_sig_1): Likewise.
> +
>  2018-10-21  Simon Marchi  <[hidden email]>
>  
>   * gdbarch.sh (gdbarch_num_cooked_regs): New.
> diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
> index 2a1b805c9a..8981bbbedf 100644
> --- a/gdb/dwarf2read.c
> +++ b/gdb/dwarf2read.c
> @@ -561,6 +561,24 @@ struct dwarf2_cu
>    unsigned int processing_has_namespace_info : 1;
>  
>    struct partial_die_info *find_partial_die (sect_offset sect_off);
> +
> +  /* If this CU was inherited by another CU (via specification,
> +     abstract_origin, etc), this is the ancestor CU.  */
> +  dwarf2_cu *ancestor;
> +
> +  /* Get the buildsym_compunit for this CU.  */
> +  buildsym_compunit *get_builder ()
> +  {
> +    /* If this CU has a builder associated with it, use that.  */
> +    if (builder != nullptr)
> +      return builder.get ();
> +
> +    /* Otherwise, search ancestors for a valid builder.  */
> +    if (ancestor != nullptr)
> +      return ancestor->get_builder ();
> +
> +    return nullptr;
> +  }
>  };
>  
>  /* A struct that can be used as a hash key for tables based on DW_AT_stmt_list.
> @@ -9822,7 +9840,7 @@ fixup_go_packaging (struct dwarf2_cu *cu)
>    struct pending *list;
>    int i;
>  
> -  for (list = *cu->builder->get_global_symbols ();
> +  for (list = *cu->get_builder ()->get_global_symbols ();
>         list != NULL;
>         list = list->next)
>      {
> @@ -10377,7 +10395,7 @@ process_full_comp_unit (struct dwarf2_per_cu_data *per_cu,
>    get_scope_pc_bounds (cu->dies, &lowpc, &highpc, cu);
>  
>    addr = gdbarch_adjust_dwarf2_addr (gdbarch, highpc + baseaddr);
> -  static_block = cu->builder->end_symtab_get_static_block (addr, 0, 1);
> +  static_block = cu->get_builder ()->end_symtab_get_static_block (addr, 0, 1);
>  
>    /* If the comp unit has DW_AT_ranges, it may have discontiguous ranges.
>       Also, DW_AT_ranges may record ranges not belonging to any child DIEs
> @@ -10386,7 +10404,7 @@ process_full_comp_unit (struct dwarf2_per_cu_data *per_cu,
>       this comp unit.  */
>    dwarf2_record_block_ranges (cu->dies, static_block, baseaddr, cu);
>  
> -  cust = cu->builder->end_symtab_from_static_block (static_block,
> +  cust = cu->get_builder ()->end_symtab_from_static_block (static_block,
>      SECT_OFF_TEXT (objfile),
>      0);
>  
> @@ -10481,7 +10499,8 @@ process_full_type_unit (struct dwarf2_per_cu_data *per_cu,
>       this TU's symbols to the existing symtab.  */
>    if (sig_type->type_unit_group->compunit_symtab == NULL)
>      {
> -      cust = cu->builder->end_expandable_symtab (0, SECT_OFF_TEXT (objfile));
> +      buildsym_compunit *builder = cu->get_builder ();
> +      cust = builder->end_expandable_symtab (0, SECT_OFF_TEXT (objfile));
>        sig_type->type_unit_group->compunit_symtab = cust;
>  
>        if (cust != NULL)
> @@ -10497,7 +10516,7 @@ process_full_type_unit (struct dwarf2_per_cu_data *per_cu,
>      }
>    else
>      {
> -      cu->builder->augment_type_symtab ();
> +      cu->get_builder ()->augment_type_symtab ();
>        cust = sig_type->type_unit_group->compunit_symtab;
>      }
>  
> @@ -11206,10 +11225,11 @@ read_namespace_alias (struct die_info *die, struct dwarf2_cu *cu)
>  static struct using_direct **
>  using_directives (struct dwarf2_cu *cu)
>  {
> -  if (cu->language == language_ada && cu->builder->outermost_context_p ())
> -    return cu->builder->get_global_using_directives ();
> +  if (cu->language == language_ada
> +      && cu->get_builder ()->outermost_context_p ())
> +    return cu->get_builder ()->get_global_using_directives ();
>    else
> -    return cu->builder->get_local_using_directives ();
> +    return cu->get_builder ()->get_local_using_directives ();
>  }
>  
>  /* Read the import statement specified by the given die and record it.  */
> @@ -11681,20 +11701,20 @@ setup_type_unit_groups (struct die_info *die, struct dwarf2_cu *cu)
>    file_entry &fe = cu->line_header->file_names[i];
>  
>    dwarf2_start_subfile (cu, fe.name, fe.include_dir (cu->line_header));
> -
> -  if (cu->builder->get_current_subfile ()->symtab == NULL)
> +  buildsym_compunit *builder = cu->get_builder ();
> +  if (builder->get_current_subfile ()->symtab == NULL)
>      {
>        /* NOTE: start_subfile will recognize when it's been
>   passed a file it has already seen.  So we can't
>   assume there's a simple mapping from
>   cu->line_header->file_names to subfiles, plus
>   cu->line_header->file_names may contain dups.  */
> -      cu->builder->get_current_subfile ()->symtab
> +      builder->get_current_subfile ()->symtab
>   = allocate_symtab (cust,
> -   cu->builder->get_current_subfile ()->name);
> +   builder->get_current_subfile ()->name);
>      }
>  
> -  fe.symtab = cu->builder->get_current_subfile ()->symtab;
> +  fe.symtab = builder->get_current_subfile ()->symtab;
>    tu_group->symtabs[i] = fe.symtab;
>   }
>      }
> @@ -13731,7 +13751,7 @@ read_func_scope (struct die_info *die, struct dwarf2_cu *cu)
>   }
>      }
>  
> -  newobj = cu->builder->push_context (0, lowpc);
> +  newobj = cu->get_builder ()->push_context (0, lowpc);
>    newobj->name = new_symbol (die, read_type_die (die, cu), cu,
>       (struct symbol *) templ_func);
>  
> @@ -13751,7 +13771,7 @@ read_func_scope (struct die_info *die, struct dwarf2_cu *cu)
>        attr_to_dynamic_prop (attr, die, cu, newobj->static_link);
>      }
>  
> -  cu->list_in_scope = cu->builder->get_local_symbols ();
> +  cu->list_in_scope = cu->get_builder ()->get_local_symbols ();
>  
>    if (die->child != NULL)
>      {
> @@ -13799,9 +13819,9 @@ read_func_scope (struct die_info *die, struct dwarf2_cu *cu)
>   }
>      }
>  
> -  struct context_stack cstk = cu->builder->pop_context ();
> +  struct context_stack cstk = cu->get_builder ()->pop_context ();
>    /* Make a block for the local symbols within.  */
> -  block = cu->builder->finish_block (cstk.name, cstk.old_blocks,
> +  block = cu->get_builder ()->finish_block (cstk.name, cstk.old_blocks,
>       cstk.static_link, lowpc, highpc);
>  
>    /* For C++, set the block's scope.  */
> @@ -13843,13 +13863,13 @@ read_func_scope (struct die_info *die, struct dwarf2_cu *cu)
>       a function declares a class that has methods).  This means that
>       when we finish processing a function scope, we may need to go
>       back to building a containing block's symbol lists.  */
> -  *cu->builder->get_local_symbols () = cstk.locals;
> -  cu->builder->set_local_using_directives (cstk.local_using_directives);
> +  *cu->get_builder ()->get_local_symbols () = cstk.locals;
> +  cu->get_builder ()->set_local_using_directives (cstk.local_using_directives);
>  
>    /* If we've finished processing a top-level function, subsequent
>       symbols go in the file symbol list.  */
> -  if (cu->builder->outermost_context_p ())
> -    cu->list_in_scope = cu->builder->get_file_symbols ();
> +  if (cu->get_builder ()->outermost_context_p ())
> +    cu->list_in_scope = cu->get_builder ()->get_file_symbols ();
>  }
>  
>  /* Process all the DIES contained within a lexical block scope.  Start
> @@ -13888,7 +13908,7 @@ read_lexical_block_scope (struct die_info *die, struct dwarf2_cu *cu)
>    lowpc = gdbarch_adjust_dwarf2_addr (gdbarch, lowpc + baseaddr);
>    highpc = gdbarch_adjust_dwarf2_addr (gdbarch, highpc + baseaddr);
>  
> -  cu->builder->push_context (0, lowpc);
> +  cu->get_builder ()->push_context (0, lowpc);
>    if (die->child != NULL)
>      {
>        child_die = die->child;
> @@ -13899,13 +13919,13 @@ read_lexical_block_scope (struct die_info *die, struct dwarf2_cu *cu)
>   }
>      }
>    inherit_abstract_dies (die, cu);
> -  struct context_stack cstk = cu->builder->pop_context ();
> +  struct context_stack cstk = cu->get_builder ()->pop_context ();
>  
> -  if (*cu->builder->get_local_symbols () != NULL
> -      || (*cu->builder->get_local_using_directives ()) != NULL)
> +  if (*cu->get_builder ()->get_local_symbols () != NULL
> +      || (*cu->get_builder ()->get_local_using_directives ()) != NULL)
>      {
>        struct block *block
> -        = cu->builder->finish_block (0, cstk.old_blocks, NULL,
> +        = cu->get_builder ()->finish_block (0, cstk.old_blocks, NULL,
>       cstk.start_addr, highpc);
>  
>        /* Note that recording ranges after traversing children, as we
> @@ -13920,8 +13940,8 @@ read_lexical_block_scope (struct die_info *die, struct dwarf2_cu *cu)
>           to do.  */
>        dwarf2_record_block_ranges (die, block, baseaddr, cu);
>      }
> -  *cu->builder->get_local_symbols () = cstk.locals;
> -  cu->builder->set_local_using_directives (cstk.local_using_directives);
> +  *cu->get_builder ()->get_local_symbols () = cstk.locals;
> +  cu->get_builder ()->set_local_using_directives (cstk.local_using_directives);
>  }
>  
>  /* Read in DW_TAG_call_site and insert it to CU->call_site_htab.  */
> @@ -14844,7 +14864,7 @@ dwarf2_record_block_ranges (struct die_info *die, struct block *block,
>  
>    low = gdbarch_adjust_dwarf2_addr (gdbarch, low + baseaddr);
>    high = gdbarch_adjust_dwarf2_addr (gdbarch, high + baseaddr);
> -  cu->builder->record_block_range (block, low, high - 1);
> +  cu->get_builder ()->record_block_range (block, low, high - 1);
>          }
>      }
>  
> @@ -14869,7 +14889,7 @@ dwarf2_record_block_ranges (struct die_info *die, struct block *block,
>    end += baseaddr;
>    start = gdbarch_adjust_dwarf2_addr (gdbarch, start);
>    end = gdbarch_adjust_dwarf2_addr (gdbarch, end);
> -  cu->builder->record_block_range (block, start, end - 1);
> +  cu->get_builder ()->record_block_range (block, start, end - 1);
>    blockvec.emplace_back (start, end);
>   });
>  
> @@ -20666,7 +20686,7 @@ lnp_state_machine::handle_set_file (file_name_index file)
>      {
>        const char *dir = fe->include_dir (m_line_header);
>  
> -      m_last_subfile = m_cu->builder->get_current_subfile ();
> +      m_last_subfile = m_cu->get_builder ()->get_current_subfile ();
>        m_line_has_non_zero_discriminator = m_discriminator != 0;
>        dwarf2_start_subfile (m_cu, fe->name, dir);
>      }
> @@ -20724,7 +20744,7 @@ dwarf_record_line_p (struct dwarf2_cu *cu,
>       int line_has_non_zero_discriminator,
>       struct subfile *last_subfile)
>  {
> -  if (cu->builder->get_current_subfile () != last_subfile)
> +  if (cu->get_builder ()->get_current_subfile () != last_subfile)
>      return 1;
>    if (line != last_line)
>      return 1;
> @@ -20755,7 +20775,7 @@ dwarf_record_line_1 (struct gdbarch *gdbarch, struct subfile *subfile,
>      }
>  
>    if (cu != nullptr)
> -    cu->builder->record_line (subfile, line, addr);
> +    cu->get_builder ()->record_line (subfile, line, addr);
>  }
>  
>  /* Subroutine of dwarf_decode_lines_1 to simplify it.
> @@ -20806,7 +20826,7 @@ lnp_state_machine::record_line (bool end_sequence)
>        fe->included_p = 1;
>        if (m_record_lines_p && (producer_is_codewarrior (m_cu) || m_is_stmt))
>   {
> -  if (m_last_subfile != m_cu->builder->get_current_subfile ()
> +  if (m_last_subfile != m_cu->get_builder ()->get_current_subfile ()
>        || end_sequence)
>      {
>        dwarf_finish_line (m_gdbarch, m_last_subfile, m_address,
> @@ -20819,12 +20839,13 @@ lnp_state_machine::record_line (bool end_sequence)
>         m_line_has_non_zero_discriminator,
>         m_last_subfile))
>   {
> +  buildsym_compunit *builder = m_cu->get_builder ();
>    dwarf_record_line_1 (m_gdbarch,
> -       m_cu->builder->get_current_subfile (),
> +       builder->get_current_subfile (),
>         m_line, m_address,
>         m_currently_recording_lines ? m_cu : nullptr);
>   }
> -      m_last_subfile = m_cu->builder->get_current_subfile ();
> +      m_last_subfile = m_cu->get_builder ()->get_current_subfile ();
>        m_last_line = m_line;
>      }
>   }
> @@ -21147,7 +21168,8 @@ dwarf_decode_lines (struct line_header *lh, const char *comp_dir,
>        /* Make sure a symtab is created for every file, even files
>   which contain only variables (i.e. no code with associated
>   line numbers).  */
> -      struct compunit_symtab *cust = cu->builder->get_compunit_symtab ();
> +      buildsym_compunit *builder = cu->get_builder ();
> +      struct compunit_symtab *cust = builder->get_compunit_symtab ();
>        int i;
>  
>        for (i = 0; i < lh->file_names.size (); i++)
> @@ -21156,13 +21178,13 @@ dwarf_decode_lines (struct line_header *lh, const char *comp_dir,
>  
>    dwarf2_start_subfile (cu, fe.name, fe.include_dir (lh));
>  
> -  if (cu->builder->get_current_subfile ()->symtab == NULL)
> +  if (builder->get_current_subfile ()->symtab == NULL)
>      {
> -      cu->builder->get_current_subfile ()->symtab
> +      builder->get_current_subfile ()->symtab
>   = allocate_symtab (cust,
> -   cu->builder->get_current_subfile ()->name);
> +   builder->get_current_subfile ()->name);
>      }
> -  fe.symtab = cu->builder->get_current_subfile ()->symtab;
> +  fe.symtab = builder->get_current_subfile ()->symtab;
>   }
>      }
>  }
> @@ -21209,7 +21231,7 @@ dwarf2_start_subfile (struct dwarf2_cu *cu, const char *filename,
>        filename = copy;
>      }
>  
> -  cu->builder->start_subfile (filename);
> +  cu->get_builder ()->start_subfile (filename);
>  
>    if (copy != NULL)
>      xfree (copy);
> @@ -21228,14 +21250,14 @@ dwarf2_start_symtab (struct dwarf2_cu *cu,
>       (cu->per_cu->dwarf2_per_objfile->objfile,
>        name, comp_dir, cu->language, low_pc));
>  
> -  cu->list_in_scope = cu->builder->get_file_symbols ();
> +  cu->list_in_scope = cu->get_builder ()->get_file_symbols ();
>  
> -  cu->builder->record_debugformat ("DWARF 2");
> -  cu->builder->record_producer (cu->producer);
> +  cu->get_builder ()->record_debugformat ("DWARF 2");
> +  cu->get_builder ()->record_producer (cu->producer);
>  
>    cu->processing_has_namespace_info = 0;
>  
> -  return cu->builder->get_compunit_symtab ();
> +  return cu->get_builder ()->get_compunit_symtab ();
>  }
>  
>  static void
> @@ -21421,7 +21443,7 @@ new_symbol (struct die_info *die, struct type *type, struct dwarf2_cu *cu,
>                   access them globally.  For instance, we want to be able
>                   to break on a nested subprogram without having to
>                   specify the context.  */
> -      list_to_add = cu->builder->get_global_symbols ();
> +      list_to_add = cu->get_builder ()->get_global_symbols ();
>      }
>    else
>      {
> @@ -21464,7 +21486,7 @@ new_symbol (struct die_info *die, struct type *type, struct dwarf2_cu *cu,
>        if (!suppress_add)
>   {
>    if (attr2 && (DW_UNSND (attr2) != 0))
> -    list_to_add = cu->builder->get_global_symbols ();
> +    list_to_add = cu->get_builder ()->get_global_symbols ();
>    else
>      list_to_add = cu->list_in_scope;
>   }
> @@ -21510,8 +21532,9 @@ new_symbol (struct die_info *die, struct type *type, struct dwarf2_cu *cu,
>    /* A variable with DW_AT_external is never static,
>       but it may be block-scoped.  */
>    list_to_add
> -    = (cu->list_in_scope == cu->builder->get_file_symbols ()
> -       ? cu->builder->get_global_symbols ()
> +    = ((cu->list_in_scope
> + == cu->get_builder ()->get_file_symbols ())
> +       ? cu->get_builder ()->get_global_symbols ()
>         : cu->list_in_scope);
>   }
>        else
> @@ -21543,8 +21566,9 @@ new_symbol (struct die_info *die, struct type *type, struct dwarf2_cu *cu,
>    /* A variable with DW_AT_external is never static, but it
>       may be block-scoped.  */
>    list_to_add
> -    = (cu->list_in_scope == cu->builder->get_file_symbols ()
> -       ? cu->builder->get_global_symbols ()
> +    = ((cu->list_in_scope
> + == cu->get_builder ()->get_file_symbols ())
> +       ? cu->get_builder ()->get_global_symbols ()
>         : cu->list_in_scope);
>  
>    SYMBOL_ACLASS_INDEX (sym) = LOC_UNRESOLVED;
> @@ -21566,7 +21590,7 @@ new_symbol (struct die_info *die, struct type *type, struct dwarf2_cu *cu,
>         pretend it's a local variable in that case so that the user can
>         still see it.  */
>      struct context_stack *curr
> -      = cu->builder->get_current_context_stack ();
> +      = cu->get_builder ()->get_current_context_stack ();
>      if (curr != nullptr && curr->name != nullptr)
>        SYMBOL_IS_ARGUMENT (sym) = 1;
>      attr = dwarf2_attr (die, DW_AT_location, cu);
> @@ -21611,10 +21635,11 @@ new_symbol (struct die_info *die, struct type *type, struct dwarf2_cu *cu,
>  
>      if (!suppress_add)
>        {
> + buildsym_compunit *builder = cu->get_builder ();
>   list_to_add
> -  = (cu->list_in_scope == cu->builder->get_file_symbols ()
> +  = (cu->list_in_scope == builder->get_file_symbols ()
>       && cu->language == language_cplus
> -     ? cu->builder->get_global_symbols ()
> +     ? builder->get_global_symbols ()
>       : cu->list_in_scope);
>  
>   /* The semantics of C++ state that "struct foo {
> @@ -21655,21 +21680,21 @@ new_symbol (struct die_info *die, struct type *type, struct dwarf2_cu *cu,
>         DW_TAG_class_type, etc. block.  */
>  
>      list_to_add
> -      = (cu->list_in_scope == cu->builder->get_file_symbols ()
> +      = (cu->list_in_scope == cu->get_builder ()->get_file_symbols ()
>   && cu->language == language_cplus
> - ? cu->builder->get_global_symbols ()
> + ? cu->get_builder ()->get_global_symbols ()
>   : cu->list_in_scope);
>    }
>    break;
>   case DW_TAG_imported_declaration:
>   case DW_TAG_namespace:
>    SYMBOL_ACLASS_INDEX (sym) = LOC_TYPEDEF;
> -  list_to_add = cu->builder->get_global_symbols ();
> +  list_to_add = cu->get_builder ()->get_global_symbols ();
>    break;
>   case DW_TAG_module:
>    SYMBOL_ACLASS_INDEX (sym) = LOC_TYPEDEF;
>    SYMBOL_DOMAIN (sym) = MODULE_DOMAIN;
> -  list_to_add = cu->builder->get_global_symbols ();
> +  list_to_add = cu->get_builder ()->get_global_symbols ();
>    break;
>   case DW_TAG_common_block:
>    SYMBOL_ACLASS_INDEX (sym) = LOC_COMMON_BLOCK;
> @@ -23001,6 +23026,10 @@ follow_die_offset (sect_offset sect_off, int offset_in_dwz,
>  
>    *ref_cu = target_cu;
>    temp_die.sect_off = sect_off;
> +
> +  if (target_cu != cu)
> +    target_cu->ancestor = cu;
> +
>    return (struct die_info *) htab_find_with_hash (target_cu->die_hash,
>    &temp_die,
>    to_underlying (sect_off));
> @@ -23335,7 +23364,7 @@ follow_die_sig_1 (struct die_info *src_die, struct signatured_type *sig_type,
>    struct dwarf2_cu **ref_cu)
>  {
>    struct die_info temp_die;
> -  struct dwarf2_cu *sig_cu;
> +  struct dwarf2_cu *sig_cu, *cu = *ref_cu;
>    struct die_info *die;
>  
>    /* While it might be nice to assert sig_type->type == NULL here,
> @@ -23369,6 +23398,9 @@ follow_die_sig_1 (struct die_info *src_die, struct signatured_type *sig_type,
>   }
>  
>        *ref_cu = sig_cu;
> +      if (sig_cu != cu)
> + sig_cu->ancestor = cu;
> +
>        return die;
>      }
>  
> @@ -23945,7 +23977,7 @@ macro_start_file (struct dwarf2_cu *cu,
>      {
>        /* Note: We don't create a macro table for this compilation unit
>   at all until we actually get a filename.  */
> -      struct macro_table *macro_table = cu->builder->get_macro_table ();
> +      struct macro_table *macro_table = cu->get_builder ()->get_macro_table ();
>  
>        /* If we have no current file, then this must be the start_file
>   directive for the compilation unit's main source file.  */
> diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
> index a917a6d04c..15a2fbaf8c 100644
> --- a/gdb/testsuite/ChangeLog
> +++ b/gdb/testsuite/ChangeLog
> @@ -1,3 +1,8 @@
> +YYYY-MM-DD  Keith Seitz  <[hidden email]>
> +
> + PR gdb/23773
> + * gdb.dwarf2/inline_subroutine-inheritance.exp: New file.
> +
>  2018-10-19  Alan Hayward  <[hidden email]>
>  
>   * gdb.python/py-cmd.exp: Check for gdb_prompt.
> diff --git a/gdb/testsuite/gdb.dwarf2/inline_subroutine-inheritance.exp b/gdb/testsuite/gdb.dwarf2/inline_subroutine-inheritance.exp
> new file mode 100644
> index 0000000000..92c5d0529b
> --- /dev/null
> +++ b/gdb/testsuite/gdb.dwarf2/inline_subroutine-inheritance.exp
> @@ -0,0 +1,213 @@
> +# Copyright 2018 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/>.
> +
> +# This tests a segfault that occurs when reading inline_subroutine DIEs
> +# with abstract_origins pointing to DIEs in other CUs.
> +#
> +# See https://bugzilla.redhat.com/show_bug.cgi?id=1638798 .
> +
> +load_lib dwarf.exp
> +
> +# This test can only be run on targets which support DWARF.
> +if {![dwarf2_support]} {
> +    return 0
> +}
> +
> +standard_testfile main.c .S
> +
> +# Create the DWARF.  This is derived from the reproducer in the Fedora
> +# bugzila mentioned above.  For clarity, some "superfluous" DIES have
> +# been left instead of simplifying/pruning the test further.
> +set asm_file [standard_output_file $srcfile2]
> +Dwarf::assemble $asm_file {
> +    declare_labels Db D72f8 D736e
> +    declare_labels D266465 D266477 D266483 D266496 D266498 D266ad3 D266ad9 \
> + D266ade D26b227 D26b237
> +    declare_labels D26d8b1 D26d8c3 D26d8cf D26d944 D26d946 D26e103 D26e145 \
> + D26e415 D26e48c D26df00 D26df06 D26df0b D272519 D274c1a D274c42
> +
> +    cu {} {
> + Db: compile_unit {
> +    {language @DW_LANG_C99}
> +    {name "<artificial>"}
> + } {
> +    D72f8: subprogram {
> + {abstract_origin :$D272519}
> + {low_pc 0xb9e20 addr}
> + {high_pc 0x1f5 data4}
> +    } {
> + D736e: inlined_subroutine {
> +    {abstract_origin :$D26b227}
> +    {low_pc 0xb9efc addr}
> +    {high_pc 0xc data4}
> + } {
> +    formal_parameter {
> + {abstract_origin :$D274c42}
> +    }
> + }
> +    }
> + }
> +    }
> +
> +    cu {} {
> + D266465: compile_unit {
> +    {language @DW_LANG_C99}
> + } {
> +    D266477: typedef {
> + {name "size_t"}
> + {type :$D266483}
> +    }
> +
> +    D266483: base_type {
> + {byte_size 8 sdata}
> + {encoding @DW_ATE_unsigned}
> +    }
> +
> +    D266496: pointer_type {
> + {byte_size 8 sdata}
> +    }
> +
> +    D266498: restrict_type {
> + {type :$D266496}
> +    }
> +
> +    D266ad3: pointer_type {
> + {byte_size 8 sdata}
> + {type :$D266ade}
> +    }
> +
> +    D266ad9: restrict_type {
> + {type :$D266ad3}
> +    }
> +
> +    D266ade: const_type {}
> +
> +    D26b227: subprogram {
> + {external 1 flag}
> + {name "memcpy"}
> + {type :$D266496}
> +    } {
> + D26b237: formal_parameter {
> +    {name "__dest"}
> +    {type :$D266498}
> + }
> + formal_parameter {
> +    {name "__src"}
> +    {type :$D266ad9}
> + }
> + formal_parameter {
> +    {name "__len"}
> +    {type :$D266477}
> + }
> +    }
> + }
> +    }
> +
> +    cu {} {
> + D26d8b1: compile_unit {
> +    {language @DW_LANG_C99}
> + } {
> +    D26d8c3: typedef {
> + {name "size_t"}
> + {type :$D26d8cf}
> +    }
> +
> +    D26d8cf: base_type {
> + {byte_size 8 sdata}
> + {encoding @DW_ATE_unsigned}
> + {name "long unsigned int"}
> +    }
> +
> +    D26d944: pointer_type {
> + {byte_size 8 sdata}
> +    }
> +
> +    D26d946: restrict_type {
> + {type :$D26d944}
> +    }
> +
> +    D26e103: structure_type {
> + {name "__object"}
> + {byte_size 12 sdata}
> +    } {
> + member {
> +    {name "__ob_next"}
> +    {type :$D26e145}
> +    {data_member_location 0 sdata}
> + }
> +    }
> +
> +    D26e145: pointer_type {
> + {byte_size 8 sdata}
> + {type :$D26e103}
> +    }
> +
> +    D26e415: typedef {
> + {name "PyObject"}
> + {type :$D26e103}
> +    }
> +
> +    D26e48c: pointer_type {
> + {byte_size 8 sdata}
> + {type :$D26e415}
> +    }
> +
> +    D26df00: pointer_type {
> + {byte_size 8 sdata}
> + {type :$D26df0b}
> +    }
> +
> +    D26df06: restrict_type {
> + {type :$D26df00}
> +    }
> +
> +    D26df0b: const_type {}
> +
> +    D272519: subprogram {
> + {name "bytes_repeat"}
> + {type :$D26e48c}
> +    }
> +
> +    D274c1a: subprogram {
> + {external 1 flag}
> + {name "memcpy"}
> + {type :$D26d944}
> +    } {
> + formal_parameter {
> +    {name "__dest"}
> +    {type :$D26d946}
> + }
> + formal_parameter {
> +    {name "__src"}
> +    {type :$D26df06}
> + }
> + D274c42: formal_parameter {
> +    {name "__len"}
> +    {type :$D26d8c3}
> + }
> +    }
> + }
> +    }
> +}
> +
> +if {[prepare_for_testing "failed to prepare" $testfile \
> + "${asm_file} ${srcfile}" {}]} {
> +    return -1
> +}
> +
> +# All we need to do is set a breakpoint, which causes the DWARF
> +# info to be read, to demonstrate the problem.
> +
> +gdb_breakpoint "bytes_repeat" message
> --
> 2.17.2

--
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/
Reply | Threaded
Open this post in threaded view
|

Re: [RFC PATCH] Introduce dwarf2_cu::get_builder

Keith Seitz
In reply to this post by Keith Seitz
On 10/23/18 11:57 AM, Keith Seitz wrote:
> I'm submitting this as an RFC rather than an actual patch because of the
> lack of coverage testing for all the places where get_builder() is used.

Would any maintainer like to comment on this?

Keith

> gdb/ChangeLog:
>
> PR gdb/23773
> * dwarf2read.c (dwarf2_cu) <ancestor>: New field.
> (dwarf2_cu::get_builder): New method.  Change all users of
> `builder' to use this method.
> (follow_die_offset): Record the ancestor CU if it is different
> from the followed DIE's CU.
> (follow_die_sig_1): Likewise.
>
> gdb/testsuite/ChangeLog:
>
> PR gdb/23773
> * gdb.dwarf2/inline_subroutine-inheritance.exp: New file.
> ---
>  gdb/ChangeLog                                 |  10 +
>  gdb/dwarf2read.c                              | 154 ++++++++-----
>  gdb/testsuite/ChangeLog                       |   5 +
>  .../inline_subroutine-inheritance.exp         | 213 ++++++++++++++++++
>  4 files changed, 321 insertions(+), 61 deletions(-)
>  create mode 100644 gdb/testsuite/gdb.dwarf2/inline_subroutine-inheritance.exp
>
> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> index 0e9b0a1a5f..bfc2554688 100644
> --- a/gdb/ChangeLog
> +++ b/gdb/ChangeLog
> @@ -1,3 +1,13 @@
> +YYYY-MM-DD  Keith Seitz  <[hidden email]>
> +
> + PR gdb/23773
> + * dwarf2read.c (dwarf2_cu) <ancestor>: New field.
> + (dwarf2_cu::get_builder): New method.  Change all users of
> + `builder' to use this method.
> + (follow_die_offset): Record the ancestor CU if it is different
> + from the followed DIE's CU.
> + (follow_die_sig_1): Likewise.
> +
>  2018-10-21  Simon Marchi  <[hidden email]>
>  
>   * gdbarch.sh (gdbarch_num_cooked_regs): New.
> diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
> index 2a1b805c9a..8981bbbedf 100644
> --- a/gdb/dwarf2read.c
> +++ b/gdb/dwarf2read.c
> @@ -561,6 +561,24 @@ struct dwarf2_cu
>    unsigned int processing_has_namespace_info : 1;
>  
>    struct partial_die_info *find_partial_die (sect_offset sect_off);
> +
> +  /* If this CU was inherited by another CU (via specification,
> +     abstract_origin, etc), this is the ancestor CU.  */
> +  dwarf2_cu *ancestor;
> +
> +  /* Get the buildsym_compunit for this CU.  */
> +  buildsym_compunit *get_builder ()
> +  {
> +    /* If this CU has a builder associated with it, use that.  */
> +    if (builder != nullptr)
> +      return builder.get ();
> +
> +    /* Otherwise, search ancestors for a valid builder.  */
> +    if (ancestor != nullptr)
> +      return ancestor->get_builder ();
> +
> +    return nullptr;
> +  }
>  };
>  
>  /* A struct that can be used as a hash key for tables based on DW_AT_stmt_list.
> @@ -9822,7 +9840,7 @@ fixup_go_packaging (struct dwarf2_cu *cu)
>    struct pending *list;
>    int i;
>  
> -  for (list = *cu->builder->get_global_symbols ();
> +  for (list = *cu->get_builder ()->get_global_symbols ();
>         list != NULL;
>         list = list->next)
>      {
> @@ -10377,7 +10395,7 @@ process_full_comp_unit (struct dwarf2_per_cu_data *per_cu,
>    get_scope_pc_bounds (cu->dies, &lowpc, &highpc, cu);
>  
>    addr = gdbarch_adjust_dwarf2_addr (gdbarch, highpc + baseaddr);
> -  static_block = cu->builder->end_symtab_get_static_block (addr, 0, 1);
> +  static_block = cu->get_builder ()->end_symtab_get_static_block (addr, 0, 1);
>  
>    /* If the comp unit has DW_AT_ranges, it may have discontiguous ranges.
>       Also, DW_AT_ranges may record ranges not belonging to any child DIEs
> @@ -10386,7 +10404,7 @@ process_full_comp_unit (struct dwarf2_per_cu_data *per_cu,
>       this comp unit.  */
>    dwarf2_record_block_ranges (cu->dies, static_block, baseaddr, cu);
>  
> -  cust = cu->builder->end_symtab_from_static_block (static_block,
> +  cust = cu->get_builder ()->end_symtab_from_static_block (static_block,
>      SECT_OFF_TEXT (objfile),
>      0);
>  
> @@ -10481,7 +10499,8 @@ process_full_type_unit (struct dwarf2_per_cu_data *per_cu,
>       this TU's symbols to the existing symtab.  */
>    if (sig_type->type_unit_group->compunit_symtab == NULL)
>      {
> -      cust = cu->builder->end_expandable_symtab (0, SECT_OFF_TEXT (objfile));
> +      buildsym_compunit *builder = cu->get_builder ();
> +      cust = builder->end_expandable_symtab (0, SECT_OFF_TEXT (objfile));
>        sig_type->type_unit_group->compunit_symtab = cust;
>  
>        if (cust != NULL)
> @@ -10497,7 +10516,7 @@ process_full_type_unit (struct dwarf2_per_cu_data *per_cu,
>      }
>    else
>      {
> -      cu->builder->augment_type_symtab ();
> +      cu->get_builder ()->augment_type_symtab ();
>        cust = sig_type->type_unit_group->compunit_symtab;
>      }
>  
> @@ -11206,10 +11225,11 @@ read_namespace_alias (struct die_info *die, struct dwarf2_cu *cu)
>  static struct using_direct **
>  using_directives (struct dwarf2_cu *cu)
>  {
> -  if (cu->language == language_ada && cu->builder->outermost_context_p ())
> -    return cu->builder->get_global_using_directives ();
> +  if (cu->language == language_ada
> +      && cu->get_builder ()->outermost_context_p ())
> +    return cu->get_builder ()->get_global_using_directives ();
>    else
> -    return cu->builder->get_local_using_directives ();
> +    return cu->get_builder ()->get_local_using_directives ();
>  }
>  
>  /* Read the import statement specified by the given die and record it.  */
> @@ -11681,20 +11701,20 @@ setup_type_unit_groups (struct die_info *die, struct dwarf2_cu *cu)
>    file_entry &fe = cu->line_header->file_names[i];
>  
>    dwarf2_start_subfile (cu, fe.name, fe.include_dir (cu->line_header));
> -
> -  if (cu->builder->get_current_subfile ()->symtab == NULL)
> +  buildsym_compunit *builder = cu->get_builder ();
> +  if (builder->get_current_subfile ()->symtab == NULL)
>      {
>        /* NOTE: start_subfile will recognize when it's been
>   passed a file it has already seen.  So we can't
>   assume there's a simple mapping from
>   cu->line_header->file_names to subfiles, plus
>   cu->line_header->file_names may contain dups.  */
> -      cu->builder->get_current_subfile ()->symtab
> +      builder->get_current_subfile ()->symtab
>   = allocate_symtab (cust,
> -   cu->builder->get_current_subfile ()->name);
> +   builder->get_current_subfile ()->name);
>      }
>  
> -  fe.symtab = cu->builder->get_current_subfile ()->symtab;
> +  fe.symtab = builder->get_current_subfile ()->symtab;
>    tu_group->symtabs[i] = fe.symtab;
>   }
>      }
> @@ -13731,7 +13751,7 @@ read_func_scope (struct die_info *die, struct dwarf2_cu *cu)
>   }
>      }
>  
> -  newobj = cu->builder->push_context (0, lowpc);
> +  newobj = cu->get_builder ()->push_context (0, lowpc);
>    newobj->name = new_symbol (die, read_type_die (die, cu), cu,
>       (struct symbol *) templ_func);
>  
> @@ -13751,7 +13771,7 @@ read_func_scope (struct die_info *die, struct dwarf2_cu *cu)
>        attr_to_dynamic_prop (attr, die, cu, newobj->static_link);
>      }
>  
> -  cu->list_in_scope = cu->builder->get_local_symbols ();
> +  cu->list_in_scope = cu->get_builder ()->get_local_symbols ();
>  
>    if (die->child != NULL)
>      {
> @@ -13799,9 +13819,9 @@ read_func_scope (struct die_info *die, struct dwarf2_cu *cu)
>   }
>      }
>  
> -  struct context_stack cstk = cu->builder->pop_context ();
> +  struct context_stack cstk = cu->get_builder ()->pop_context ();
>    /* Make a block for the local symbols within.  */
> -  block = cu->builder->finish_block (cstk.name, cstk.old_blocks,
> +  block = cu->get_builder ()->finish_block (cstk.name, cstk.old_blocks,
>       cstk.static_link, lowpc, highpc);
>  
>    /* For C++, set the block's scope.  */
> @@ -13843,13 +13863,13 @@ read_func_scope (struct die_info *die, struct dwarf2_cu *cu)
>       a function declares a class that has methods).  This means that
>       when we finish processing a function scope, we may need to go
>       back to building a containing block's symbol lists.  */
> -  *cu->builder->get_local_symbols () = cstk.locals;
> -  cu->builder->set_local_using_directives (cstk.local_using_directives);
> +  *cu->get_builder ()->get_local_symbols () = cstk.locals;
> +  cu->get_builder ()->set_local_using_directives (cstk.local_using_directives);
>  
>    /* If we've finished processing a top-level function, subsequent
>       symbols go in the file symbol list.  */
> -  if (cu->builder->outermost_context_p ())
> -    cu->list_in_scope = cu->builder->get_file_symbols ();
> +  if (cu->get_builder ()->outermost_context_p ())
> +    cu->list_in_scope = cu->get_builder ()->get_file_symbols ();
>  }
>  
>  /* Process all the DIES contained within a lexical block scope.  Start
> @@ -13888,7 +13908,7 @@ read_lexical_block_scope (struct die_info *die, struct dwarf2_cu *cu)
>    lowpc = gdbarch_adjust_dwarf2_addr (gdbarch, lowpc + baseaddr);
>    highpc = gdbarch_adjust_dwarf2_addr (gdbarch, highpc + baseaddr);
>  
> -  cu->builder->push_context (0, lowpc);
> +  cu->get_builder ()->push_context (0, lowpc);
>    if (die->child != NULL)
>      {
>        child_die = die->child;
> @@ -13899,13 +13919,13 @@ read_lexical_block_scope (struct die_info *die, struct dwarf2_cu *cu)
>   }
>      }
>    inherit_abstract_dies (die, cu);
> -  struct context_stack cstk = cu->builder->pop_context ();
> +  struct context_stack cstk = cu->get_builder ()->pop_context ();
>  
> -  if (*cu->builder->get_local_symbols () != NULL
> -      || (*cu->builder->get_local_using_directives ()) != NULL)
> +  if (*cu->get_builder ()->get_local_symbols () != NULL
> +      || (*cu->get_builder ()->get_local_using_directives ()) != NULL)
>      {
>        struct block *block
> -        = cu->builder->finish_block (0, cstk.old_blocks, NULL,
> +        = cu->get_builder ()->finish_block (0, cstk.old_blocks, NULL,
>       cstk.start_addr, highpc);
>  
>        /* Note that recording ranges after traversing children, as we
> @@ -13920,8 +13940,8 @@ read_lexical_block_scope (struct die_info *die, struct dwarf2_cu *cu)
>           to do.  */
>        dwarf2_record_block_ranges (die, block, baseaddr, cu);
>      }
> -  *cu->builder->get_local_symbols () = cstk.locals;
> -  cu->builder->set_local_using_directives (cstk.local_using_directives);
> +  *cu->get_builder ()->get_local_symbols () = cstk.locals;
> +  cu->get_builder ()->set_local_using_directives (cstk.local_using_directives);
>  }
>  
>  /* Read in DW_TAG_call_site and insert it to CU->call_site_htab.  */
> @@ -14844,7 +14864,7 @@ dwarf2_record_block_ranges (struct die_info *die, struct block *block,
>  
>    low = gdbarch_adjust_dwarf2_addr (gdbarch, low + baseaddr);
>    high = gdbarch_adjust_dwarf2_addr (gdbarch, high + baseaddr);
> -  cu->builder->record_block_range (block, low, high - 1);
> +  cu->get_builder ()->record_block_range (block, low, high - 1);
>          }
>      }
>  
> @@ -14869,7 +14889,7 @@ dwarf2_record_block_ranges (struct die_info *die, struct block *block,
>    end += baseaddr;
>    start = gdbarch_adjust_dwarf2_addr (gdbarch, start);
>    end = gdbarch_adjust_dwarf2_addr (gdbarch, end);
> -  cu->builder->record_block_range (block, start, end - 1);
> +  cu->get_builder ()->record_block_range (block, start, end - 1);
>    blockvec.emplace_back (start, end);
>   });
>  
> @@ -20666,7 +20686,7 @@ lnp_state_machine::handle_set_file (file_name_index file)
>      {
>        const char *dir = fe->include_dir (m_line_header);
>  
> -      m_last_subfile = m_cu->builder->get_current_subfile ();
> +      m_last_subfile = m_cu->get_builder ()->get_current_subfile ();
>        m_line_has_non_zero_discriminator = m_discriminator != 0;
>        dwarf2_start_subfile (m_cu, fe->name, dir);
>      }
> @@ -20724,7 +20744,7 @@ dwarf_record_line_p (struct dwarf2_cu *cu,
>       int line_has_non_zero_discriminator,
>       struct subfile *last_subfile)
>  {
> -  if (cu->builder->get_current_subfile () != last_subfile)
> +  if (cu->get_builder ()->get_current_subfile () != last_subfile)
>      return 1;
>    if (line != last_line)
>      return 1;
> @@ -20755,7 +20775,7 @@ dwarf_record_line_1 (struct gdbarch *gdbarch, struct subfile *subfile,
>      }
>  
>    if (cu != nullptr)
> -    cu->builder->record_line (subfile, line, addr);
> +    cu->get_builder ()->record_line (subfile, line, addr);
>  }
>  
>  /* Subroutine of dwarf_decode_lines_1 to simplify it.
> @@ -20806,7 +20826,7 @@ lnp_state_machine::record_line (bool end_sequence)
>        fe->included_p = 1;
>        if (m_record_lines_p && (producer_is_codewarrior (m_cu) || m_is_stmt))
>   {
> -  if (m_last_subfile != m_cu->builder->get_current_subfile ()
> +  if (m_last_subfile != m_cu->get_builder ()->get_current_subfile ()
>        || end_sequence)
>      {
>        dwarf_finish_line (m_gdbarch, m_last_subfile, m_address,
> @@ -20819,12 +20839,13 @@ lnp_state_machine::record_line (bool end_sequence)
>         m_line_has_non_zero_discriminator,
>         m_last_subfile))
>   {
> +  buildsym_compunit *builder = m_cu->get_builder ();
>    dwarf_record_line_1 (m_gdbarch,
> -       m_cu->builder->get_current_subfile (),
> +       builder->get_current_subfile (),
>         m_line, m_address,
>         m_currently_recording_lines ? m_cu : nullptr);
>   }
> -      m_last_subfile = m_cu->builder->get_current_subfile ();
> +      m_last_subfile = m_cu->get_builder ()->get_current_subfile ();
>        m_last_line = m_line;
>      }
>   }
> @@ -21147,7 +21168,8 @@ dwarf_decode_lines (struct line_header *lh, const char *comp_dir,
>        /* Make sure a symtab is created for every file, even files
>   which contain only variables (i.e. no code with associated
>   line numbers).  */
> -      struct compunit_symtab *cust = cu->builder->get_compunit_symtab ();
> +      buildsym_compunit *builder = cu->get_builder ();
> +      struct compunit_symtab *cust = builder->get_compunit_symtab ();
>        int i;
>  
>        for (i = 0; i < lh->file_names.size (); i++)
> @@ -21156,13 +21178,13 @@ dwarf_decode_lines (struct line_header *lh, const char *comp_dir,
>  
>    dwarf2_start_subfile (cu, fe.name, fe.include_dir (lh));
>  
> -  if (cu->builder->get_current_subfile ()->symtab == NULL)
> +  if (builder->get_current_subfile ()->symtab == NULL)
>      {
> -      cu->builder->get_current_subfile ()->symtab
> +      builder->get_current_subfile ()->symtab
>   = allocate_symtab (cust,
> -   cu->builder->get_current_subfile ()->name);
> +   builder->get_current_subfile ()->name);
>      }
> -  fe.symtab = cu->builder->get_current_subfile ()->symtab;
> +  fe.symtab = builder->get_current_subfile ()->symtab;
>   }
>      }
>  }
> @@ -21209,7 +21231,7 @@ dwarf2_start_subfile (struct dwarf2_cu *cu, const char *filename,
>        filename = copy;
>      }
>  
> -  cu->builder->start_subfile (filename);
> +  cu->get_builder ()->start_subfile (filename);
>  
>    if (copy != NULL)
>      xfree (copy);
> @@ -21228,14 +21250,14 @@ dwarf2_start_symtab (struct dwarf2_cu *cu,
>       (cu->per_cu->dwarf2_per_objfile->objfile,
>        name, comp_dir, cu->language, low_pc));
>  
> -  cu->list_in_scope = cu->builder->get_file_symbols ();
> +  cu->list_in_scope = cu->get_builder ()->get_file_symbols ();
>  
> -  cu->builder->record_debugformat ("DWARF 2");
> -  cu->builder->record_producer (cu->producer);
> +  cu->get_builder ()->record_debugformat ("DWARF 2");
> +  cu->get_builder ()->record_producer (cu->producer);
>  
>    cu->processing_has_namespace_info = 0;
>  
> -  return cu->builder->get_compunit_symtab ();
> +  return cu->get_builder ()->get_compunit_symtab ();
>  }
>  
>  static void
> @@ -21421,7 +21443,7 @@ new_symbol (struct die_info *die, struct type *type, struct dwarf2_cu *cu,
>                   access them globally.  For instance, we want to be able
>                   to break on a nested subprogram without having to
>                   specify the context.  */
> -      list_to_add = cu->builder->get_global_symbols ();
> +      list_to_add = cu->get_builder ()->get_global_symbols ();
>      }
>    else
>      {
> @@ -21464,7 +21486,7 @@ new_symbol (struct die_info *die, struct type *type, struct dwarf2_cu *cu,
>        if (!suppress_add)
>   {
>    if (attr2 && (DW_UNSND (attr2) != 0))
> -    list_to_add = cu->builder->get_global_symbols ();
> +    list_to_add = cu->get_builder ()->get_global_symbols ();
>    else
>      list_to_add = cu->list_in_scope;
>   }
> @@ -21510,8 +21532,9 @@ new_symbol (struct die_info *die, struct type *type, struct dwarf2_cu *cu,
>    /* A variable with DW_AT_external is never static,
>       but it may be block-scoped.  */
>    list_to_add
> -    = (cu->list_in_scope == cu->builder->get_file_symbols ()
> -       ? cu->builder->get_global_symbols ()
> +    = ((cu->list_in_scope
> + == cu->get_builder ()->get_file_symbols ())
> +       ? cu->get_builder ()->get_global_symbols ()
>         : cu->list_in_scope);
>   }
>        else
> @@ -21543,8 +21566,9 @@ new_symbol (struct die_info *die, struct type *type, struct dwarf2_cu *cu,
>    /* A variable with DW_AT_external is never static, but it
>       may be block-scoped.  */
>    list_to_add
> -    = (cu->list_in_scope == cu->builder->get_file_symbols ()
> -       ? cu->builder->get_global_symbols ()
> +    = ((cu->list_in_scope
> + == cu->get_builder ()->get_file_symbols ())
> +       ? cu->get_builder ()->get_global_symbols ()
>         : cu->list_in_scope);
>  
>    SYMBOL_ACLASS_INDEX (sym) = LOC_UNRESOLVED;
> @@ -21566,7 +21590,7 @@ new_symbol (struct die_info *die, struct type *type, struct dwarf2_cu *cu,
>         pretend it's a local variable in that case so that the user can
>         still see it.  */
>      struct context_stack *curr
> -      = cu->builder->get_current_context_stack ();
> +      = cu->get_builder ()->get_current_context_stack ();
>      if (curr != nullptr && curr->name != nullptr)
>        SYMBOL_IS_ARGUMENT (sym) = 1;
>      attr = dwarf2_attr (die, DW_AT_location, cu);
> @@ -21611,10 +21635,11 @@ new_symbol (struct die_info *die, struct type *type, struct dwarf2_cu *cu,
>  
>      if (!suppress_add)
>        {
> + buildsym_compunit *builder = cu->get_builder ();
>   list_to_add
> -  = (cu->list_in_scope == cu->builder->get_file_symbols ()
> +  = (cu->list_in_scope == builder->get_file_symbols ()
>       && cu->language == language_cplus
> -     ? cu->builder->get_global_symbols ()
> +     ? builder->get_global_symbols ()
>       : cu->list_in_scope);
>  
>   /* The semantics of C++ state that "struct foo {
> @@ -21655,21 +21680,21 @@ new_symbol (struct die_info *die, struct type *type, struct dwarf2_cu *cu,
>         DW_TAG_class_type, etc. block.  */
>  
>      list_to_add
> -      = (cu->list_in_scope == cu->builder->get_file_symbols ()
> +      = (cu->list_in_scope == cu->get_builder ()->get_file_symbols ()
>   && cu->language == language_cplus
> - ? cu->builder->get_global_symbols ()
> + ? cu->get_builder ()->get_global_symbols ()
>   : cu->list_in_scope);
>    }
>    break;
>   case DW_TAG_imported_declaration:
>   case DW_TAG_namespace:
>    SYMBOL_ACLASS_INDEX (sym) = LOC_TYPEDEF;
> -  list_to_add = cu->builder->get_global_symbols ();
> +  list_to_add = cu->get_builder ()->get_global_symbols ();
>    break;
>   case DW_TAG_module:
>    SYMBOL_ACLASS_INDEX (sym) = LOC_TYPEDEF;
>    SYMBOL_DOMAIN (sym) = MODULE_DOMAIN;
> -  list_to_add = cu->builder->get_global_symbols ();
> +  list_to_add = cu->get_builder ()->get_global_symbols ();
>    break;
>   case DW_TAG_common_block:
>    SYMBOL_ACLASS_INDEX (sym) = LOC_COMMON_BLOCK;
> @@ -23001,6 +23026,10 @@ follow_die_offset (sect_offset sect_off, int offset_in_dwz,
>  
>    *ref_cu = target_cu;
>    temp_die.sect_off = sect_off;
> +
> +  if (target_cu != cu)
> +    target_cu->ancestor = cu;
> +
>    return (struct die_info *) htab_find_with_hash (target_cu->die_hash,
>    &temp_die,
>    to_underlying (sect_off));
> @@ -23335,7 +23364,7 @@ follow_die_sig_1 (struct die_info *src_die, struct signatured_type *sig_type,
>    struct dwarf2_cu **ref_cu)
>  {
>    struct die_info temp_die;
> -  struct dwarf2_cu *sig_cu;
> +  struct dwarf2_cu *sig_cu, *cu = *ref_cu;
>    struct die_info *die;
>  
>    /* While it might be nice to assert sig_type->type == NULL here,
> @@ -23369,6 +23398,9 @@ follow_die_sig_1 (struct die_info *src_die, struct signatured_type *sig_type,
>   }
>  
>        *ref_cu = sig_cu;
> +      if (sig_cu != cu)
> + sig_cu->ancestor = cu;
> +
>        return die;
>      }
>  
> @@ -23945,7 +23977,7 @@ macro_start_file (struct dwarf2_cu *cu,
>      {
>        /* Note: We don't create a macro table for this compilation unit
>   at all until we actually get a filename.  */
> -      struct macro_table *macro_table = cu->builder->get_macro_table ();
> +      struct macro_table *macro_table = cu->get_builder ()->get_macro_table ();
>  
>        /* If we have no current file, then this must be the start_file
>   directive for the compilation unit's main source file.  */
> diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
> index a917a6d04c..15a2fbaf8c 100644
> --- a/gdb/testsuite/ChangeLog
> +++ b/gdb/testsuite/ChangeLog
> @@ -1,3 +1,8 @@
> +YYYY-MM-DD  Keith Seitz  <[hidden email]>
> +
> + PR gdb/23773
> + * gdb.dwarf2/inline_subroutine-inheritance.exp: New file.
> +
>  2018-10-19  Alan Hayward  <[hidden email]>
>  
>   * gdb.python/py-cmd.exp: Check for gdb_prompt.
> diff --git a/gdb/testsuite/gdb.dwarf2/inline_subroutine-inheritance.exp b/gdb/testsuite/gdb.dwarf2/inline_subroutine-inheritance.exp
> new file mode 100644
> index 0000000000..92c5d0529b
> --- /dev/null
> +++ b/gdb/testsuite/gdb.dwarf2/inline_subroutine-inheritance.exp
> @@ -0,0 +1,213 @@
> +# Copyright 2018 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/>.
> +
> +# This tests a segfault that occurs when reading inline_subroutine DIEs
> +# with abstract_origins pointing to DIEs in other CUs.
> +#
> +# See https://bugzilla.redhat.com/show_bug.cgi?id=1638798 .
> +
> +load_lib dwarf.exp
> +
> +# This test can only be run on targets which support DWARF.
> +if {![dwarf2_support]} {
> +    return 0
> +}
> +
> +standard_testfile main.c .S
> +
> +# Create the DWARF.  This is derived from the reproducer in the Fedora
> +# bugzila mentioned above.  For clarity, some "superfluous" DIES have
> +# been left instead of simplifying/pruning the test further.
> +set asm_file [standard_output_file $srcfile2]
> +Dwarf::assemble $asm_file {
> +    declare_labels Db D72f8 D736e
> +    declare_labels D266465 D266477 D266483 D266496 D266498 D266ad3 D266ad9 \
> + D266ade D26b227 D26b237
> +    declare_labels D26d8b1 D26d8c3 D26d8cf D26d944 D26d946 D26e103 D26e145 \
> + D26e415 D26e48c D26df00 D26df06 D26df0b D272519 D274c1a D274c42
> +
> +    cu {} {
> + Db: compile_unit {
> +    {language @DW_LANG_C99}
> +    {name "<artificial>"}
> + } {
> +    D72f8: subprogram {
> + {abstract_origin :$D272519}
> + {low_pc 0xb9e20 addr}
> + {high_pc 0x1f5 data4}
> +    } {
> + D736e: inlined_subroutine {
> +    {abstract_origin :$D26b227}
> +    {low_pc 0xb9efc addr}
> +    {high_pc 0xc data4}
> + } {
> +    formal_parameter {
> + {abstract_origin :$D274c42}
> +    }
> + }
> +    }
> + }
> +    }
> +
> +    cu {} {
> + D266465: compile_unit {
> +    {language @DW_LANG_C99}
> + } {
> +    D266477: typedef {
> + {name "size_t"}
> + {type :$D266483}
> +    }
> +
> +    D266483: base_type {
> + {byte_size 8 sdata}
> + {encoding @DW_ATE_unsigned}
> +    }
> +
> +    D266496: pointer_type {
> + {byte_size 8 sdata}
> +    }
> +
> +    D266498: restrict_type {
> + {type :$D266496}
> +    }
> +
> +    D266ad3: pointer_type {
> + {byte_size 8 sdata}
> + {type :$D266ade}
> +    }
> +
> +    D266ad9: restrict_type {
> + {type :$D266ad3}
> +    }
> +
> +    D266ade: const_type {}
> +
> +    D26b227: subprogram {
> + {external 1 flag}
> + {name "memcpy"}
> + {type :$D266496}
> +    } {
> + D26b237: formal_parameter {
> +    {name "__dest"}
> +    {type :$D266498}
> + }
> + formal_parameter {
> +    {name "__src"}
> +    {type :$D266ad9}
> + }
> + formal_parameter {
> +    {name "__len"}
> +    {type :$D266477}
> + }
> +    }
> + }
> +    }
> +
> +    cu {} {
> + D26d8b1: compile_unit {
> +    {language @DW_LANG_C99}
> + } {
> +    D26d8c3: typedef {
> + {name "size_t"}
> + {type :$D26d8cf}
> +    }
> +
> +    D26d8cf: base_type {
> + {byte_size 8 sdata}
> + {encoding @DW_ATE_unsigned}
> + {name "long unsigned int"}
> +    }
> +
> +    D26d944: pointer_type {
> + {byte_size 8 sdata}
> +    }
> +
> +    D26d946: restrict_type {
> + {type :$D26d944}
> +    }
> +
> +    D26e103: structure_type {
> + {name "__object"}
> + {byte_size 12 sdata}
> +    } {
> + member {
> +    {name "__ob_next"}
> +    {type :$D26e145}
> +    {data_member_location 0 sdata}
> + }
> +    }
> +
> +    D26e145: pointer_type {
> + {byte_size 8 sdata}
> + {type :$D26e103}
> +    }
> +
> +    D26e415: typedef {
> + {name "PyObject"}
> + {type :$D26e103}
> +    }
> +
> +    D26e48c: pointer_type {
> + {byte_size 8 sdata}
> + {type :$D26e415}
> +    }
> +
> +    D26df00: pointer_type {
> + {byte_size 8 sdata}
> + {type :$D26df0b}
> +    }
> +
> +    D26df06: restrict_type {
> + {type :$D26df00}
> +    }
> +
> +    D26df0b: const_type {}
> +
> +    D272519: subprogram {
> + {name "bytes_repeat"}
> + {type :$D26e48c}
> +    }
> +
> +    D274c1a: subprogram {
> + {external 1 flag}
> + {name "memcpy"}
> + {type :$D26d944}
> +    } {
> + formal_parameter {
> +    {name "__dest"}
> +    {type :$D26d946}
> + }
> + formal_parameter {
> +    {name "__src"}
> +    {type :$D26df06}
> + }
> + D274c42: formal_parameter {
> +    {name "__len"}
> +    {type :$D26d8c3}
> + }
> +    }
> + }
> +    }
> +}
> +
> +if {[prepare_for_testing "failed to prepare" $testfile \
> + "${asm_file} ${srcfile}" {}]} {
> +    return -1
> +}
> +
> +# All we need to do is set a breakpoint, which causes the DWARF
> +# info to be read, to demonstrate the problem.
> +
> +gdb_breakpoint "bytes_repeat" message
>

Reply | Threaded
Open this post in threaded view
|

Re: [RFC PATCH] Introduce dwarf2_cu::get_builder

Keith Seitz
On 11/6/18 1:33 PM, Keith Seitz wrote:
> On 10/23/18 11:57 AM, Keith Seitz wrote:
>> I'm submitting this as an RFC rather than an actual patch because of the
>> lack of coverage testing for all the places where get_builder() is used.
>
> Would any maintainer like to comment on this?

Ping

>> gdb/ChangeLog:
>>
>> PR gdb/23773
>> * dwarf2read.c (dwarf2_cu) <ancestor>: New field.
>> (dwarf2_cu::get_builder): New method.  Change all users of
>> `builder' to use this method.
>> (follow_die_offset): Record the ancestor CU if it is different
>> from the followed DIE's CU.
>> (follow_die_sig_1): Likewise.
>>
>> gdb/testsuite/ChangeLog:
>>
>> PR gdb/23773
>> * gdb.dwarf2/inline_subroutine-inheritance.exp: New file.
>> ---
>>  gdb/ChangeLog                                 |  10 +
>>  gdb/dwarf2read.c                              | 154 ++++++++-----
>>  gdb/testsuite/ChangeLog                       |   5 +
>>  .../inline_subroutine-inheritance.exp         | 213 ++++++++++++++++++
>>  4 files changed, 321 insertions(+), 61 deletions(-)
>>  create mode 100644 gdb/testsuite/gdb.dwarf2/inline_subroutine-inheritance.exp
>>
>> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
>> index 0e9b0a1a5f..bfc2554688 100644
>> --- a/gdb/ChangeLog
>> +++ b/gdb/ChangeLog
>> @@ -1,3 +1,13 @@
>> +YYYY-MM-DD  Keith Seitz  <[hidden email]>
>> +
>> + PR gdb/23773
>> + * dwarf2read.c (dwarf2_cu) <ancestor>: New field.
>> + (dwarf2_cu::get_builder): New method.  Change all users of
>> + `builder' to use this method.
>> + (follow_die_offset): Record the ancestor CU if it is different
>> + from the followed DIE's CU.
>> + (follow_die_sig_1): Likewise.
>> +
>>  2018-10-21  Simon Marchi  <[hidden email]>
>>  
>>   * gdbarch.sh (gdbarch_num_cooked_regs): New.
>> diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
>> index 2a1b805c9a..8981bbbedf 100644
>> --- a/gdb/dwarf2read.c
>> +++ b/gdb/dwarf2read.c
>> @@ -561,6 +561,24 @@ struct dwarf2_cu
>>    unsigned int processing_has_namespace_info : 1;
>>  
>>    struct partial_die_info *find_partial_die (sect_offset sect_off);
>> +
>> +  /* If this CU was inherited by another CU (via specification,
>> +     abstract_origin, etc), this is the ancestor CU.  */
>> +  dwarf2_cu *ancestor;
>> +
>> +  /* Get the buildsym_compunit for this CU.  */
>> +  buildsym_compunit *get_builder ()
>> +  {
>> +    /* If this CU has a builder associated with it, use that.  */
>> +    if (builder != nullptr)
>> +      return builder.get ();
>> +
>> +    /* Otherwise, search ancestors for a valid builder.  */
>> +    if (ancestor != nullptr)
>> +      return ancestor->get_builder ();
>> +
>> +    return nullptr;
>> +  }
>>  };
>>  
>>  /* A struct that can be used as a hash key for tables based on DW_AT_stmt_list.
>> @@ -9822,7 +9840,7 @@ fixup_go_packaging (struct dwarf2_cu *cu)
>>    struct pending *list;
>>    int i;
>>  
>> -  for (list = *cu->builder->get_global_symbols ();
>> +  for (list = *cu->get_builder ()->get_global_symbols ();
>>         list != NULL;
>>         list = list->next)
>>      {
>> @@ -10377,7 +10395,7 @@ process_full_comp_unit (struct dwarf2_per_cu_data *per_cu,
>>    get_scope_pc_bounds (cu->dies, &lowpc, &highpc, cu);
>>  
>>    addr = gdbarch_adjust_dwarf2_addr (gdbarch, highpc + baseaddr);
>> -  static_block = cu->builder->end_symtab_get_static_block (addr, 0, 1);
>> +  static_block = cu->get_builder ()->end_symtab_get_static_block (addr, 0, 1);
>>  
>>    /* If the comp unit has DW_AT_ranges, it may have discontiguous ranges.
>>       Also, DW_AT_ranges may record ranges not belonging to any child DIEs
>> @@ -10386,7 +10404,7 @@ process_full_comp_unit (struct dwarf2_per_cu_data *per_cu,
>>       this comp unit.  */
>>    dwarf2_record_block_ranges (cu->dies, static_block, baseaddr, cu);
>>  
>> -  cust = cu->builder->end_symtab_from_static_block (static_block,
>> +  cust = cu->get_builder ()->end_symtab_from_static_block (static_block,
>>      SECT_OFF_TEXT (objfile),
>>      0);
>>  
>> @@ -10481,7 +10499,8 @@ process_full_type_unit (struct dwarf2_per_cu_data *per_cu,
>>       this TU's symbols to the existing symtab.  */
>>    if (sig_type->type_unit_group->compunit_symtab == NULL)
>>      {
>> -      cust = cu->builder->end_expandable_symtab (0, SECT_OFF_TEXT (objfile));
>> +      buildsym_compunit *builder = cu->get_builder ();
>> +      cust = builder->end_expandable_symtab (0, SECT_OFF_TEXT (objfile));
>>        sig_type->type_unit_group->compunit_symtab = cust;
>>  
>>        if (cust != NULL)
>> @@ -10497,7 +10516,7 @@ process_full_type_unit (struct dwarf2_per_cu_data *per_cu,
>>      }
>>    else
>>      {
>> -      cu->builder->augment_type_symtab ();
>> +      cu->get_builder ()->augment_type_symtab ();
>>        cust = sig_type->type_unit_group->compunit_symtab;
>>      }
>>  
>> @@ -11206,10 +11225,11 @@ read_namespace_alias (struct die_info *die, struct dwarf2_cu *cu)
>>  static struct using_direct **
>>  using_directives (struct dwarf2_cu *cu)
>>  {
>> -  if (cu->language == language_ada && cu->builder->outermost_context_p ())
>> -    return cu->builder->get_global_using_directives ();
>> +  if (cu->language == language_ada
>> +      && cu->get_builder ()->outermost_context_p ())
>> +    return cu->get_builder ()->get_global_using_directives ();
>>    else
>> -    return cu->builder->get_local_using_directives ();
>> +    return cu->get_builder ()->get_local_using_directives ();
>>  }
>>  
>>  /* Read the import statement specified by the given die and record it.  */
>> @@ -11681,20 +11701,20 @@ setup_type_unit_groups (struct die_info *die, struct dwarf2_cu *cu)
>>    file_entry &fe = cu->line_header->file_names[i];
>>  
>>    dwarf2_start_subfile (cu, fe.name, fe.include_dir (cu->line_header));
>> -
>> -  if (cu->builder->get_current_subfile ()->symtab == NULL)
>> +  buildsym_compunit *builder = cu->get_builder ();
>> +  if (builder->get_current_subfile ()->symtab == NULL)
>>      {
>>        /* NOTE: start_subfile will recognize when it's been
>>   passed a file it has already seen.  So we can't
>>   assume there's a simple mapping from
>>   cu->line_header->file_names to subfiles, plus
>>   cu->line_header->file_names may contain dups.  */
>> -      cu->builder->get_current_subfile ()->symtab
>> +      builder->get_current_subfile ()->symtab
>>   = allocate_symtab (cust,
>> -   cu->builder->get_current_subfile ()->name);
>> +   builder->get_current_subfile ()->name);
>>      }
>>  
>> -  fe.symtab = cu->builder->get_current_subfile ()->symtab;
>> +  fe.symtab = builder->get_current_subfile ()->symtab;
>>    tu_group->symtabs[i] = fe.symtab;
>>   }
>>      }
>> @@ -13731,7 +13751,7 @@ read_func_scope (struct die_info *die, struct dwarf2_cu *cu)
>>   }
>>      }
>>  
>> -  newobj = cu->builder->push_context (0, lowpc);
>> +  newobj = cu->get_builder ()->push_context (0, lowpc);
>>    newobj->name = new_symbol (die, read_type_die (die, cu), cu,
>>       (struct symbol *) templ_func);
>>  
>> @@ -13751,7 +13771,7 @@ read_func_scope (struct die_info *die, struct dwarf2_cu *cu)
>>        attr_to_dynamic_prop (attr, die, cu, newobj->static_link);
>>      }
>>  
>> -  cu->list_in_scope = cu->builder->get_local_symbols ();
>> +  cu->list_in_scope = cu->get_builder ()->get_local_symbols ();
>>  
>>    if (die->child != NULL)
>>      {
>> @@ -13799,9 +13819,9 @@ read_func_scope (struct die_info *die, struct dwarf2_cu *cu)
>>   }
>>      }
>>  
>> -  struct context_stack cstk = cu->builder->pop_context ();
>> +  struct context_stack cstk = cu->get_builder ()->pop_context ();
>>    /* Make a block for the local symbols within.  */
>> -  block = cu->builder->finish_block (cstk.name, cstk.old_blocks,
>> +  block = cu->get_builder ()->finish_block (cstk.name, cstk.old_blocks,
>>       cstk.static_link, lowpc, highpc);
>>  
>>    /* For C++, set the block's scope.  */
>> @@ -13843,13 +13863,13 @@ read_func_scope (struct die_info *die, struct dwarf2_cu *cu)
>>       a function declares a class that has methods).  This means that
>>       when we finish processing a function scope, we may need to go
>>       back to building a containing block's symbol lists.  */
>> -  *cu->builder->get_local_symbols () = cstk.locals;
>> -  cu->builder->set_local_using_directives (cstk.local_using_directives);
>> +  *cu->get_builder ()->get_local_symbols () = cstk.locals;
>> +  cu->get_builder ()->set_local_using_directives (cstk.local_using_directives);
>>  
>>    /* If we've finished processing a top-level function, subsequent
>>       symbols go in the file symbol list.  */
>> -  if (cu->builder->outermost_context_p ())
>> -    cu->list_in_scope = cu->builder->get_file_symbols ();
>> +  if (cu->get_builder ()->outermost_context_p ())
>> +    cu->list_in_scope = cu->get_builder ()->get_file_symbols ();
>>  }
>>  
>>  /* Process all the DIES contained within a lexical block scope.  Start
>> @@ -13888,7 +13908,7 @@ read_lexical_block_scope (struct die_info *die, struct dwarf2_cu *cu)
>>    lowpc = gdbarch_adjust_dwarf2_addr (gdbarch, lowpc + baseaddr);
>>    highpc = gdbarch_adjust_dwarf2_addr (gdbarch, highpc + baseaddr);
>>  
>> -  cu->builder->push_context (0, lowpc);
>> +  cu->get_builder ()->push_context (0, lowpc);
>>    if (die->child != NULL)
>>      {
>>        child_die = die->child;
>> @@ -13899,13 +13919,13 @@ read_lexical_block_scope (struct die_info *die, struct dwarf2_cu *cu)
>>   }
>>      }
>>    inherit_abstract_dies (die, cu);
>> -  struct context_stack cstk = cu->builder->pop_context ();
>> +  struct context_stack cstk = cu->get_builder ()->pop_context ();
>>  
>> -  if (*cu->builder->get_local_symbols () != NULL
>> -      || (*cu->builder->get_local_using_directives ()) != NULL)
>> +  if (*cu->get_builder ()->get_local_symbols () != NULL
>> +      || (*cu->get_builder ()->get_local_using_directives ()) != NULL)
>>      {
>>        struct block *block
>> -        = cu->builder->finish_block (0, cstk.old_blocks, NULL,
>> +        = cu->get_builder ()->finish_block (0, cstk.old_blocks, NULL,
>>       cstk.start_addr, highpc);
>>  
>>        /* Note that recording ranges after traversing children, as we
>> @@ -13920,8 +13940,8 @@ read_lexical_block_scope (struct die_info *die, struct dwarf2_cu *cu)
>>           to do.  */
>>        dwarf2_record_block_ranges (die, block, baseaddr, cu);
>>      }
>> -  *cu->builder->get_local_symbols () = cstk.locals;
>> -  cu->builder->set_local_using_directives (cstk.local_using_directives);
>> +  *cu->get_builder ()->get_local_symbols () = cstk.locals;
>> +  cu->get_builder ()->set_local_using_directives (cstk.local_using_directives);
>>  }
>>  
>>  /* Read in DW_TAG_call_site and insert it to CU->call_site_htab.  */
>> @@ -14844,7 +14864,7 @@ dwarf2_record_block_ranges (struct die_info *die, struct block *block,
>>  
>>    low = gdbarch_adjust_dwarf2_addr (gdbarch, low + baseaddr);
>>    high = gdbarch_adjust_dwarf2_addr (gdbarch, high + baseaddr);
>> -  cu->builder->record_block_range (block, low, high - 1);
>> +  cu->get_builder ()->record_block_range (block, low, high - 1);
>>          }
>>      }
>>  
>> @@ -14869,7 +14889,7 @@ dwarf2_record_block_ranges (struct die_info *die, struct block *block,
>>    end += baseaddr;
>>    start = gdbarch_adjust_dwarf2_addr (gdbarch, start);
>>    end = gdbarch_adjust_dwarf2_addr (gdbarch, end);
>> -  cu->builder->record_block_range (block, start, end - 1);
>> +  cu->get_builder ()->record_block_range (block, start, end - 1);
>>    blockvec.emplace_back (start, end);
>>   });
>>  
>> @@ -20666,7 +20686,7 @@ lnp_state_machine::handle_set_file (file_name_index file)
>>      {
>>        const char *dir = fe->include_dir (m_line_header);
>>  
>> -      m_last_subfile = m_cu->builder->get_current_subfile ();
>> +      m_last_subfile = m_cu->get_builder ()->get_current_subfile ();
>>        m_line_has_non_zero_discriminator = m_discriminator != 0;
>>        dwarf2_start_subfile (m_cu, fe->name, dir);
>>      }
>> @@ -20724,7 +20744,7 @@ dwarf_record_line_p (struct dwarf2_cu *cu,
>>       int line_has_non_zero_discriminator,
>>       struct subfile *last_subfile)
>>  {
>> -  if (cu->builder->get_current_subfile () != last_subfile)
>> +  if (cu->get_builder ()->get_current_subfile () != last_subfile)
>>      return 1;
>>    if (line != last_line)
>>      return 1;
>> @@ -20755,7 +20775,7 @@ dwarf_record_line_1 (struct gdbarch *gdbarch, struct subfile *subfile,
>>      }
>>  
>>    if (cu != nullptr)
>> -    cu->builder->record_line (subfile, line, addr);
>> +    cu->get_builder ()->record_line (subfile, line, addr);
>>  }
>>  
>>  /* Subroutine of dwarf_decode_lines_1 to simplify it.
>> @@ -20806,7 +20826,7 @@ lnp_state_machine::record_line (bool end_sequence)
>>        fe->included_p = 1;
>>        if (m_record_lines_p && (producer_is_codewarrior (m_cu) || m_is_stmt))
>>   {
>> -  if (m_last_subfile != m_cu->builder->get_current_subfile ()
>> +  if (m_last_subfile != m_cu->get_builder ()->get_current_subfile ()
>>        || end_sequence)
>>      {
>>        dwarf_finish_line (m_gdbarch, m_last_subfile, m_address,
>> @@ -20819,12 +20839,13 @@ lnp_state_machine::record_line (bool end_sequence)
>>         m_line_has_non_zero_discriminator,
>>         m_last_subfile))
>>   {
>> +  buildsym_compunit *builder = m_cu->get_builder ();
>>    dwarf_record_line_1 (m_gdbarch,
>> -       m_cu->builder->get_current_subfile (),
>> +       builder->get_current_subfile (),
>>         m_line, m_address,
>>         m_currently_recording_lines ? m_cu : nullptr);
>>   }
>> -      m_last_subfile = m_cu->builder->get_current_subfile ();
>> +      m_last_subfile = m_cu->get_builder ()->get_current_subfile ();
>>        m_last_line = m_line;
>>      }
>>   }
>> @@ -21147,7 +21168,8 @@ dwarf_decode_lines (struct line_header *lh, const char *comp_dir,
>>        /* Make sure a symtab is created for every file, even files
>>   which contain only variables (i.e. no code with associated
>>   line numbers).  */
>> -      struct compunit_symtab *cust = cu->builder->get_compunit_symtab ();
>> +      buildsym_compunit *builder = cu->get_builder ();
>> +      struct compunit_symtab *cust = builder->get_compunit_symtab ();
>>        int i;
>>  
>>        for (i = 0; i < lh->file_names.size (); i++)
>> @@ -21156,13 +21178,13 @@ dwarf_decode_lines (struct line_header *lh, const char *comp_dir,
>>  
>>    dwarf2_start_subfile (cu, fe.name, fe.include_dir (lh));
>>  
>> -  if (cu->builder->get_current_subfile ()->symtab == NULL)
>> +  if (builder->get_current_subfile ()->symtab == NULL)
>>      {
>> -      cu->builder->get_current_subfile ()->symtab
>> +      builder->get_current_subfile ()->symtab
>>   = allocate_symtab (cust,
>> -   cu->builder->get_current_subfile ()->name);
>> +   builder->get_current_subfile ()->name);
>>      }
>> -  fe.symtab = cu->builder->get_current_subfile ()->symtab;
>> +  fe.symtab = builder->get_current_subfile ()->symtab;
>>   }
>>      }
>>  }
>> @@ -21209,7 +21231,7 @@ dwarf2_start_subfile (struct dwarf2_cu *cu, const char *filename,
>>        filename = copy;
>>      }
>>  
>> -  cu->builder->start_subfile (filename);
>> +  cu->get_builder ()->start_subfile (filename);
>>  
>>    if (copy != NULL)
>>      xfree (copy);
>> @@ -21228,14 +21250,14 @@ dwarf2_start_symtab (struct dwarf2_cu *cu,
>>       (cu->per_cu->dwarf2_per_objfile->objfile,
>>        name, comp_dir, cu->language, low_pc));
>>  
>> -  cu->list_in_scope = cu->builder->get_file_symbols ();
>> +  cu->list_in_scope = cu->get_builder ()->get_file_symbols ();
>>  
>> -  cu->builder->record_debugformat ("DWARF 2");
>> -  cu->builder->record_producer (cu->producer);
>> +  cu->get_builder ()->record_debugformat ("DWARF 2");
>> +  cu->get_builder ()->record_producer (cu->producer);
>>  
>>    cu->processing_has_namespace_info = 0;
>>  
>> -  return cu->builder->get_compunit_symtab ();
>> +  return cu->get_builder ()->get_compunit_symtab ();
>>  }
>>  
>>  static void
>> @@ -21421,7 +21443,7 @@ new_symbol (struct die_info *die, struct type *type, struct dwarf2_cu *cu,
>>                   access them globally.  For instance, we want to be able
>>                   to break on a nested subprogram without having to
>>                   specify the context.  */
>> -      list_to_add = cu->builder->get_global_symbols ();
>> +      list_to_add = cu->get_builder ()->get_global_symbols ();
>>      }
>>    else
>>      {
>> @@ -21464,7 +21486,7 @@ new_symbol (struct die_info *die, struct type *type, struct dwarf2_cu *cu,
>>        if (!suppress_add)
>>   {
>>    if (attr2 && (DW_UNSND (attr2) != 0))
>> -    list_to_add = cu->builder->get_global_symbols ();
>> +    list_to_add = cu->get_builder ()->get_global_symbols ();
>>    else
>>      list_to_add = cu->list_in_scope;
>>   }
>> @@ -21510,8 +21532,9 @@ new_symbol (struct die_info *die, struct type *type, struct dwarf2_cu *cu,
>>    /* A variable with DW_AT_external is never static,
>>       but it may be block-scoped.  */
>>    list_to_add
>> -    = (cu->list_in_scope == cu->builder->get_file_symbols ()
>> -       ? cu->builder->get_global_symbols ()
>> +    = ((cu->list_in_scope
>> + == cu->get_builder ()->get_file_symbols ())
>> +       ? cu->get_builder ()->get_global_symbols ()
>>         : cu->list_in_scope);
>>   }
>>        else
>> @@ -21543,8 +21566,9 @@ new_symbol (struct die_info *die, struct type *type, struct dwarf2_cu *cu,
>>    /* A variable with DW_AT_external is never static, but it
>>       may be block-scoped.  */
>>    list_to_add
>> -    = (cu->list_in_scope == cu->builder->get_file_symbols ()
>> -       ? cu->builder->get_global_symbols ()
>> +    = ((cu->list_in_scope
>> + == cu->get_builder ()->get_file_symbols ())
>> +       ? cu->get_builder ()->get_global_symbols ()
>>         : cu->list_in_scope);
>>  
>>    SYMBOL_ACLASS_INDEX (sym) = LOC_UNRESOLVED;
>> @@ -21566,7 +21590,7 @@ new_symbol (struct die_info *die, struct type *type, struct dwarf2_cu *cu,
>>         pretend it's a local variable in that case so that the user can
>>         still see it.  */
>>      struct context_stack *curr
>> -      = cu->builder->get_current_context_stack ();
>> +      = cu->get_builder ()->get_current_context_stack ();
>>      if (curr != nullptr && curr->name != nullptr)
>>        SYMBOL_IS_ARGUMENT (sym) = 1;
>>      attr = dwarf2_attr (die, DW_AT_location, cu);
>> @@ -21611,10 +21635,11 @@ new_symbol (struct die_info *die, struct type *type, struct dwarf2_cu *cu,
>>  
>>      if (!suppress_add)
>>        {
>> + buildsym_compunit *builder = cu->get_builder ();
>>   list_to_add
>> -  = (cu->list_in_scope == cu->builder->get_file_symbols ()
>> +  = (cu->list_in_scope == builder->get_file_symbols ()
>>       && cu->language == language_cplus
>> -     ? cu->builder->get_global_symbols ()
>> +     ? builder->get_global_symbols ()
>>       : cu->list_in_scope);
>>  
>>   /* The semantics of C++ state that "struct foo {
>> @@ -21655,21 +21680,21 @@ new_symbol (struct die_info *die, struct type *type, struct dwarf2_cu *cu,
>>         DW_TAG_class_type, etc. block.  */
>>  
>>      list_to_add
>> -      = (cu->list_in_scope == cu->builder->get_file_symbols ()
>> +      = (cu->list_in_scope == cu->get_builder ()->get_file_symbols ()
>>   && cu->language == language_cplus
>> - ? cu->builder->get_global_symbols ()
>> + ? cu->get_builder ()->get_global_symbols ()
>>   : cu->list_in_scope);
>>    }
>>    break;
>>   case DW_TAG_imported_declaration:
>>   case DW_TAG_namespace:
>>    SYMBOL_ACLASS_INDEX (sym) = LOC_TYPEDEF;
>> -  list_to_add = cu->builder->get_global_symbols ();
>> +  list_to_add = cu->get_builder ()->get_global_symbols ();
>>    break;
>>   case DW_TAG_module:
>>    SYMBOL_ACLASS_INDEX (sym) = LOC_TYPEDEF;
>>    SYMBOL_DOMAIN (sym) = MODULE_DOMAIN;
>> -  list_to_add = cu->builder->get_global_symbols ();
>> +  list_to_add = cu->get_builder ()->get_global_symbols ();
>>    break;
>>   case DW_TAG_common_block:
>>    SYMBOL_ACLASS_INDEX (sym) = LOC_COMMON_BLOCK;
>> @@ -23001,6 +23026,10 @@ follow_die_offset (sect_offset sect_off, int offset_in_dwz,
>>  
>>    *ref_cu = target_cu;
>>    temp_die.sect_off = sect_off;
>> +
>> +  if (target_cu != cu)
>> +    target_cu->ancestor = cu;
>> +
>>    return (struct die_info *) htab_find_with_hash (target_cu->die_hash,
>>    &temp_die,
>>    to_underlying (sect_off));
>> @@ -23335,7 +23364,7 @@ follow_die_sig_1 (struct die_info *src_die, struct signatured_type *sig_type,
>>    struct dwarf2_cu **ref_cu)
>>  {
>>    struct die_info temp_die;
>> -  struct dwarf2_cu *sig_cu;
>> +  struct dwarf2_cu *sig_cu, *cu = *ref_cu;
>>    struct die_info *die;
>>  
>>    /* While it might be nice to assert sig_type->type == NULL here,
>> @@ -23369,6 +23398,9 @@ follow_die_sig_1 (struct die_info *src_die, struct signatured_type *sig_type,
>>   }
>>  
>>        *ref_cu = sig_cu;
>> +      if (sig_cu != cu)
>> + sig_cu->ancestor = cu;
>> +
>>        return die;
>>      }
>>  
>> @@ -23945,7 +23977,7 @@ macro_start_file (struct dwarf2_cu *cu,
>>      {
>>        /* Note: We don't create a macro table for this compilation unit
>>   at all until we actually get a filename.  */
>> -      struct macro_table *macro_table = cu->builder->get_macro_table ();
>> +      struct macro_table *macro_table = cu->get_builder ()->get_macro_table ();
>>  
>>        /* If we have no current file, then this must be the start_file
>>   directive for the compilation unit's main source file.  */
>> diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
>> index a917a6d04c..15a2fbaf8c 100644
>> --- a/gdb/testsuite/ChangeLog
>> +++ b/gdb/testsuite/ChangeLog
>> @@ -1,3 +1,8 @@
>> +YYYY-MM-DD  Keith Seitz  <[hidden email]>
>> +
>> + PR gdb/23773
>> + * gdb.dwarf2/inline_subroutine-inheritance.exp: New file.
>> +
>>  2018-10-19  Alan Hayward  <[hidden email]>
>>  
>>   * gdb.python/py-cmd.exp: Check for gdb_prompt.
>> diff --git a/gdb/testsuite/gdb.dwarf2/inline_subroutine-inheritance.exp b/gdb/testsuite/gdb.dwarf2/inline_subroutine-inheritance.exp
>> new file mode 100644
>> index 0000000000..92c5d0529b
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.dwarf2/inline_subroutine-inheritance.exp
>> @@ -0,0 +1,213 @@
>> +# Copyright 2018 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/>.
>> +
>> +# This tests a segfault that occurs when reading inline_subroutine DIEs
>> +# with abstract_origins pointing to DIEs in other CUs.
>> +#
>> +# See https://bugzilla.redhat.com/show_bug.cgi?id=1638798 .
>> +
>> +load_lib dwarf.exp
>> +
>> +# This test can only be run on targets which support DWARF.
>> +if {![dwarf2_support]} {
>> +    return 0
>> +}
>> +
>> +standard_testfile main.c .S
>> +
>> +# Create the DWARF.  This is derived from the reproducer in the Fedora
>> +# bugzila mentioned above.  For clarity, some "superfluous" DIES have
>> +# been left instead of simplifying/pruning the test further.
>> +set asm_file [standard_output_file $srcfile2]
>> +Dwarf::assemble $asm_file {
>> +    declare_labels Db D72f8 D736e
>> +    declare_labels D266465 D266477 D266483 D266496 D266498 D266ad3 D266ad9 \
>> + D266ade D26b227 D26b237
>> +    declare_labels D26d8b1 D26d8c3 D26d8cf D26d944 D26d946 D26e103 D26e145 \
>> + D26e415 D26e48c D26df00 D26df06 D26df0b D272519 D274c1a D274c42
>> +
>> +    cu {} {
>> + Db: compile_unit {
>> +    {language @DW_LANG_C99}
>> +    {name "<artificial>"}
>> + } {
>> +    D72f8: subprogram {
>> + {abstract_origin :$D272519}
>> + {low_pc 0xb9e20 addr}
>> + {high_pc 0x1f5 data4}
>> +    } {
>> + D736e: inlined_subroutine {
>> +    {abstract_origin :$D26b227}
>> +    {low_pc 0xb9efc addr}
>> +    {high_pc 0xc data4}
>> + } {
>> +    formal_parameter {
>> + {abstract_origin :$D274c42}
>> +    }
>> + }
>> +    }
>> + }
>> +    }
>> +
>> +    cu {} {
>> + D266465: compile_unit {
>> +    {language @DW_LANG_C99}
>> + } {
>> +    D266477: typedef {
>> + {name "size_t"}
>> + {type :$D266483}
>> +    }
>> +
>> +    D266483: base_type {
>> + {byte_size 8 sdata}
>> + {encoding @DW_ATE_unsigned}
>> +    }
>> +
>> +    D266496: pointer_type {
>> + {byte_size 8 sdata}
>> +    }
>> +
>> +    D266498: restrict_type {
>> + {type :$D266496}
>> +    }
>> +
>> +    D266ad3: pointer_type {
>> + {byte_size 8 sdata}
>> + {type :$D266ade}
>> +    }
>> +
>> +    D266ad9: restrict_type {
>> + {type :$D266ad3}
>> +    }
>> +
>> +    D266ade: const_type {}
>> +
>> +    D26b227: subprogram {
>> + {external 1 flag}
>> + {name "memcpy"}
>> + {type :$D266496}
>> +    } {
>> + D26b237: formal_parameter {
>> +    {name "__dest"}
>> +    {type :$D266498}
>> + }
>> + formal_parameter {
>> +    {name "__src"}
>> +    {type :$D266ad9}
>> + }
>> + formal_parameter {
>> +    {name "__len"}
>> +    {type :$D266477}
>> + }
>> +    }
>> + }
>> +    }
>> +
>> +    cu {} {
>> + D26d8b1: compile_unit {
>> +    {language @DW_LANG_C99}
>> + } {
>> +    D26d8c3: typedef {
>> + {name "size_t"}
>> + {type :$D26d8cf}
>> +    }
>> +
>> +    D26d8cf: base_type {
>> + {byte_size 8 sdata}
>> + {encoding @DW_ATE_unsigned}
>> + {name "long unsigned int"}
>> +    }
>> +
>> +    D26d944: pointer_type {
>> + {byte_size 8 sdata}
>> +    }
>> +
>> +    D26d946: restrict_type {
>> + {type :$D26d944}
>> +    }
>> +
>> +    D26e103: structure_type {
>> + {name "__object"}
>> + {byte_size 12 sdata}
>> +    } {
>> + member {
>> +    {name "__ob_next"}
>> +    {type :$D26e145}
>> +    {data_member_location 0 sdata}
>> + }
>> +    }
>> +
>> +    D26e145: pointer_type {
>> + {byte_size 8 sdata}
>> + {type :$D26e103}
>> +    }
>> +
>> +    D26e415: typedef {
>> + {name "PyObject"}
>> + {type :$D26e103}
>> +    }
>> +
>> +    D26e48c: pointer_type {
>> + {byte_size 8 sdata}
>> + {type :$D26e415}
>> +    }
>> +
>> +    D26df00: pointer_type {
>> + {byte_size 8 sdata}
>> + {type :$D26df0b}
>> +    }
>> +
>> +    D26df06: restrict_type {
>> + {type :$D26df00}
>> +    }
>> +
>> +    D26df0b: const_type {}
>> +
>> +    D272519: subprogram {
>> + {name "bytes_repeat"}
>> + {type :$D26e48c}
>> +    }
>> +
>> +    D274c1a: subprogram {
>> + {external 1 flag}
>> + {name "memcpy"}
>> + {type :$D26d944}
>> +    } {
>> + formal_parameter {
>> +    {name "__dest"}
>> +    {type :$D26d946}
>> + }
>> + formal_parameter {
>> +    {name "__src"}
>> +    {type :$D26df06}
>> + }
>> + D274c42: formal_parameter {
>> +    {name "__len"}
>> +    {type :$D26d8c3}
>> + }
>> +    }
>> + }
>> +    }
>> +}
>> +
>> +if {[prepare_for_testing "failed to prepare" $testfile \
>> + "${asm_file} ${srcfile}" {}]} {
>> +    return -1
>> +}
>> +
>> +# All we need to do is set a breakpoint, which causes the DWARF
>> +# info to be read, to demonstrate the problem.
>> +
>> +gdb_breakpoint "bytes_repeat" message
>>
>

Reply | Threaded
Open this post in threaded view
|

Re: [RFC PATCH] Introduce dwarf2_cu::get_builder

Simon Marchi
In reply to this post by Keith Seitz
On 2018-10-23 14:57, Keith Seitz wrote:

> This patch is an attempt to deal with a variety of bugs reported where
> GDB segfaults attempting to access a dwarf2_cu's builder.  In certain
> circumstances, this builder can be NULL.
>
> The approach taken here is to save the ancestor CU into the dwarf2_cu
> of
> all CUs with DIEs that are "imported."  This can happen whenever
> follow_die_offset and friends are called.  This essentially introduces
> a
> chain of CUs that caused the importation of a DIE from a CU.  Whenever
> a builder is requested of a CU that has none, the ancestors are
> searched
> for the first one with a builder.
>
> The bulk of the patch is relatively mindless text conversion from
> "cu->builder" to "cu->get_builder ()".  I've included one test which
> was derived from one (of the many) bugs reported on the issue in both
> sourceware and Fedora bugzillas.
>
> I'm submitting this as an RFC rather than an actual patch because of
> the
> lack of coverage testing for all the places where get_builder() is
> used.

Hi Keith,

I can't give meaningful comments yet, because this deals with cases I'm
not really used to.

With the way you did it, I'm confident that the behavior will not change
for existing cases, since we will end up using the same builder as
before.  Only in those cases where it would have been NULL (and thus
would have caused a segfault) will the behavior change.  And if it gives
some good results for these cases, then it seems right.

I noticed there were still some "naked" access to builder (places that
don't use get_builder).  fixup_go_packaging is one, for example.  Is it
expected or should they be converted too?

Simon
Reply | Threaded
Open this post in threaded view
|

Re: [RFC PATCH] Introduce dwarf2_cu::get_builder

Keith Seitz
On 11/16/18 5:19 AM, Simon Marchi wrote:
> I noticed there were still some "naked" access to builder (places
> that don't use get_builder).  fixup_go_packaging is one, for example.
> Is it expected or should they be converted too?>

I must have missed them in my initial pass. It is my intention to follow
Sergio's recommendation to make "builder" private.

Thank you for your comments.

Keith

Reply | Threaded
Open this post in threaded view
|

Re: [RFC PATCH] Introduce dwarf2_cu::get_builder

Tom Tromey-2
In reply to this post by Keith Seitz
>>>>> "Keith" == Keith Seitz <[hidden email]> writes:

Keith> This patch is an attempt to deal with a variety of bugs reported where
Keith> GDB segfaults attempting to access a dwarf2_cu's builder.  In certain
Keith> circumstances, this builder can be NULL.

I think it would be good to describe one such case in the commit
message.  I suppose there must be some spot that works on an inherited
DIE in the context of its own CU?  I found myself wondering where.

Keith> I'm submitting this as an RFC rather than an actual patch because of the
Keith> lack of coverage testing for all the places where get_builder() is used.

I wouldn't be surprised if some of them aren't really reachable in the
sense of ever being able to take the ancestor path.  I'm not concerned
about needing tests for every single point.

Keith> +# This tests a segfault that occurs when reading inline_subroutine DIEs

inlined_subroutine (missing the "d")

Keith> +# bugzila mentioned above.  For clarity, some "superfluous" DIES have

Missing an "l" in bugzilla.

The rest seems good to me.  Thank you for the patch, and for reducing
and rewriting the test case like this.

Tom
Reply | Threaded
Open this post in threaded view
|

Re: [RFC PATCH] Introduce dwarf2_cu::get_builder

Keith Seitz
On 11/30/18 12:30 PM, Tom Tromey wrote:
>>>>>> "Keith" == Keith Seitz <[hidden email]> writes:
>
> Keith> This patch is an attempt to deal with a variety of bugs reported where
> Keith> GDB segfaults attempting to access a dwarf2_cu's builder.  In certain
> Keith> circumstances, this builder can be NULL.
>
> I think it would be good to describe one such case in the commit
> message.  I suppose there must be some spot that works on an inherited
> DIE in the context of its own CU?  I found myself wondering where.

I've changed the commit log to `hint' at a common pathway that this occurs
with a further suggestion to see either the test or the PR for a concrete
example:

    ... In certain
    circumstances, this builder can be NULL.  This is especially common
    when inheriting DIEs via inlined subroutines in other CUs.  The test
    case demonstrates one such situation reported by users.  See gdb/23773,
    rhbz1638798, and dups for other concrete examples.

That's my "picture is worth 1,000 words" description. Would that be sufficient?

> Keith> I'm submitting this as an RFC rather than an actual patch because of the
> Keith> lack of coverage testing for all the places where get_builder() is used.
>
> I wouldn't be surprised if some of them aren't really reachable in the
> sense of ever being able to take the ancestor path.  I'm not concerned
> about needing tests for every single point.

Great.

> Keith> +# This tests a segfault that occurs when reading inline_subroutine DIEs
>
> inlined_subroutine (missing the "d")

Fixed.
 
> Keith> +# bugzila mentioned above.  For clarity, some "superfluous" DIES have
>
> Missing an "l" in bugzilla.

And fixed.

> The rest seems good to me.  Thank you for the patch, and for reducing
> and rewriting the test case like this.

Thank you for the review!

To save additional messages, I'll reply to the other "official" submission
message (https://sourceware.org/ml/gdb-patches/2018-11/msg00450.html) here:

Keith> +private:
Keith>    /* The symtab builder for this CU.  This is only non-NULL when full
Keith>       symbols are being read.  */
Keith> -  std::unique_ptr<buildsym_compunit> builder;
Keith> +  std::unique_ptr<buildsym_compunit> m_builder;

> Good idea, thanks for doing this.
>
> I think the previous nits still apply; though this email didn't include
> the test so I don't know for certain about those.

The test is unchanged, I simply forgot to include them (for $uknown reasons).

Keith
Reply | Threaded
Open this post in threaded view
|

Re: [RFC PATCH] Introduce dwarf2_cu::get_builder

Tom Tromey-2
>>>>> "Keith" == Keith Seitz <[hidden email]> writes:

[...]
Keith> The test is unchanged, I simply forgot to include them (for
Keith> $uknown reasons).

FAOD this patch is ok.  It came up on irc yesterday that Keith was
waiting for an approval.

Tom
Reply | Threaded
Open this post in threaded view
|

Re: [RFC PATCH] Introduce dwarf2_cu::get_builder

Keith Seitz
On 1/16/19 8:38 AM, Tom Tromey wrote:
>>>>>> "Keith" == Keith Seitz <[hidden email]> writes:
>
> [...]
> Keith> The test is unchanged, I simply forgot to include them (for
> Keith> $uknown reasons).
>
> FAOD this patch is ok.  It came up on irc yesterday that Keith was
> waiting for an approval.

Thanks! Patch is pushed.

Keith