diff mbox

possible fix for PR symtab/23010

Message ID 87po1l2ab0.fsf@tromey.com
State New, archived
Headers

Commit Message

Tom Tromey May 23, 2018, 11:59 p.m. UTC 
  >>>>> "Sergio" == Sergio Durigan Junior <sergiodj@redhat.com> writes:

Sergio> In either case, and speaking without much knowledge of the patch itself,
Sergio> I think it should be included in 8.1.1.  At least I know I will include
Sergio> it in our Fedora GDB (and well, if Tom is willing to backport it, then
Sergio> he'll also save me some time!).

I did the backport, and mentioned it on irc, but neglected to send
email.

I've appended it.

Sergio said he would test it against libwebkitgtk, which is a relief,
since that usually causes problems for my machine.

Tom

commit b5aa3df7a69ed6ecf6ac046dca12b39d9533ed29
Author: Tom Tromey <tom@tromey.com>
Date:   Thu Apr 12 08:24:41 2018 -0600

    Fix for dwz-related crash
    
    PR symtab/23010 reports a crash that occurs when using -readnow
    on a dwz-generated debuginfo file.
    
    The crash occurs because the DWARF has a partial CU with no language
    set, and then a full CU that references this partial CU using
    DW_AT_abstract_origin.
    
    In this case, the partial CU is read by dw2_expand_all_symtabs using
    language_minimal; but then this conflicts with the creation of the
    block's symbol table in the C++ CU.
    
    This patch fixes the problem by arranging for partial CUs not to be
    read by -readnow.  I tend to think that it doesn't make sense to read
    a partial CU in isolation -- they should only be read when imported
    into some other CU.
    
    In conjunction with some other patches I am going to post, this also
    fixes the Rust -readnow crash that Jan reported.
    
    There are two problems with this patch:
    
    1. It is difficult to reason about.  There are many cases where I've
       patched the code to call init_cutu_and_read_dies with the flag set
       to "please do read partial units" -- but I find it difficult to be
       sure that this is always correct.
    
    2. It is still missing a standalone test case.  This seemed hard.
    
    2018-05-17  Tom Tromey  <tom@tromey.com>
    
            PR symtab/23010:
            * dwarf2read.c (load_cu, dw2_do_instantiate_symtab)
            (dw2_instantiate_symtab): Add skip_partial parameter.
            (dw2_find_last_source_symtab, dw2_map_expand_apply)
            (dw2_lookup_symbol, dw2_expand_symtabs_for_function)
            (dw2_expand_all_symtabs, dw2_expand_symtabs_with_fullname)
            (dw2_expand_symtabs_matching_one)
            (dw2_find_pc_sect_compunit_symtab)
            (dw2_debug_names_lookup_symbol)
            (dw2_debug_names_expand_symtabs_for_function): Update.
            (init_cutu_and_read_dies): Add skip_partial parameter.
            (process_psymtab_comp_unit, build_type_psymtabs_1)
            (process_skeletonless_type_unit, load_partial_comp_unit)
            (psymtab_to_symtab_1): Update.
            (load_full_comp_unit): Add skip_partial parameter.
            (process_imported_unit_die, dwarf2_read_addr_index)
            (follow_die_offset, dwarf2_fetch_die_loc_sect_off)
            (dwarf2_fetch_constant_bytes, dwarf2_fetch_die_type_sect_off)
            (read_signatured_type): Update.

Comments

Sergio Durigan Junior May 24, 2018, 12:41 a.m. UTC | #1 
On Wednesday, May 23 2018, Tom Tromey wrote:

>>>>>> "Sergio" == Sergio Durigan Junior <sergiodj@redhat.com> writes:
>
> Sergio> In either case, and speaking without much knowledge of the patch itself,
> Sergio> I think it should be included in 8.1.1.  At least I know I will include
> Sergio> it in our Fedora GDB (and well, if Tom is willing to backport it, then
> Sergio> he'll also save me some time!).
>
> I did the backport, and mentioned it on irc, but neglected to send
> email.
>
> I've appended it.
>
> Sergio said he would test it against libwebkitgtk, which is a relief,
> since that usually causes problems for my machine.

Thanks, Tom.

Keith has performed a few tests today, and it seems that the patch
doesn't fix the real issues reported by Fedora GDB users after all.
I'm still deciding if it makes sense to ship this on Fedora GDB or
not...  Sorry for requesting the backport, I hope it's still useful for
GDB 8.1.1.
Tom Tromey May 24, 2018, 4:08 a.m. UTC | #2 
>>>>> "Sergio" == Sergio Durigan Junior <sergiodj@redhat.com> writes:

Sergio> Keith has performed a few tests today, and it seems that the patch
Sergio> doesn't fix the real issues reported by Fedora GDB users after all.
Sergio> I'm still deciding if it makes sense to ship this on Fedora GDB or
Sergio> not...  Sorry for requesting the backport, I hope it's still useful for
Sergio> GDB 8.1.1.

Maybe the backport is flawed some way.  Or did Keith try git master?
I'm a bit curious to know what fails.

Also, after this patch I noticed that there are now two mechanisms for
skipping partial CUs -- one for psymtabs and one more generic one that
was added by the patch.  I have a patch to remove the psymtab one which
I can submit soon.  (The current setup doesn't hurt anything, it's just
a little redundant.)

Tom
Keith Seitz May 24, 2018, 2:48 p.m. UTC | #3 
On 05/23/2018 09:08 PM, Tom Tromey wrote:
>>>>>> "Sergio" == Sergio Durigan Junior <sergiodj@redhat.com> writes:
> 
> Sergio> Keith has performed a few tests today, and it seems that the patch
> Sergio> doesn't fix the real issues reported by Fedora GDB users after all.
> Sergio> I'm still deciding if it makes sense to ship this on Fedora GDB or
> Sergio> not...  Sorry for requesting the backport, I hope it's still useful for
> Sergio> GDB 8.1.1.
> 
> Maybe the backport is flawed some way.  Or did Keith try git master?
> I'm a bit curious to know what fails.

I'm working on git master with the 23010 patch installed. The patch doesn't help with rhbz 1574015 (which I am investigating). Fortunately, this bug has a reproducer.

Keith
diff mbox

Patch

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index d8c2ef427a..9913c081d6 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,25 @@ 
+2018-05-17  Tom Tromey  <tom@tromey.com>
+
+	PR symtab/23010:
+	* dwarf2read.c (load_cu, dw2_do_instantiate_symtab)
+	(dw2_instantiate_symtab): Add skip_partial parameter.
+	(dw2_find_last_source_symtab, dw2_map_expand_apply)
+	(dw2_lookup_symbol, dw2_expand_symtabs_for_function)
+	(dw2_expand_all_symtabs, dw2_expand_symtabs_with_fullname)
+	(dw2_expand_symtabs_matching_one)
+	(dw2_find_pc_sect_compunit_symtab)
+	(dw2_debug_names_lookup_symbol)
+	(dw2_debug_names_expand_symtabs_for_function): Update.
+	(init_cutu_and_read_dies): Add skip_partial parameter.
+	(process_psymtab_comp_unit, build_type_psymtabs_1)
+	(process_skeletonless_type_unit, load_partial_comp_unit)
+	(psymtab_to_symtab_1): Update.
+	(load_full_comp_unit): Add skip_partial parameter.
+	(process_imported_unit_die, dwarf2_read_addr_index)
+	(follow_die_offset, dwarf2_fetch_die_loc_sect_off)
+	(dwarf2_fetch_constant_bytes, dwarf2_fetch_die_type_sect_off)
+	(read_signatured_type): Update.
+
 2018-05-10  Joel Brobecker  <brobecker@adacore.com>
 
 	PR server/23158:
diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index bd2bc7d278..d4380f8335 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -2140,7 +2140,7 @@  static void create_all_comp_units (struct objfile *);
 
 static int create_all_type_units (struct objfile *);
 
-static void load_full_comp_unit (struct dwarf2_per_cu_data *,
+static void load_full_comp_unit (struct dwarf2_per_cu_data *, bool,
 				 enum language);
 
 static void process_full_comp_unit (struct dwarf2_per_cu_data *,
@@ -2204,7 +2204,7 @@  static const gdb_byte *read_and_check_comp_unit_head
 
 static void init_cutu_and_read_dies
   (struct dwarf2_per_cu_data *this_cu, struct abbrev_table *abbrev_table,
-   int use_existing_cu, int keep,
+   int use_existing_cu, int keep, bool skip_partial,
    die_reader_func_ftype *die_reader_func, void *data);
 
 static void init_cutu_and_read_dies_simple
@@ -3055,12 +3055,12 @@  create_quick_file_names_table (unsigned int nr_initial_entries)
    processing PER_CU->CU.  dw2_setup must have been already called.  */
 
 static void
-load_cu (struct dwarf2_per_cu_data *per_cu)
+load_cu (struct dwarf2_per_cu_data *per_cu, bool skip_partial)
 {
   if (per_cu->is_debug_types)
     load_full_type_unit (per_cu);
   else
-    load_full_comp_unit (per_cu, language_minimal);
+    load_full_comp_unit (per_cu, skip_partial, language_minimal);
 
   if (per_cu->cu == NULL)
     return;  /* Dummy CU.  */
@@ -3071,7 +3071,7 @@  load_cu (struct dwarf2_per_cu_data *per_cu)
 /* Read in the symbols for PER_CU.  */
 
 static void
-dw2_do_instantiate_symtab (struct dwarf2_per_cu_data *per_cu)
+dw2_do_instantiate_symtab (struct dwarf2_per_cu_data *per_cu, bool skip_partial)
 {
   struct cleanup *back_to;
 
@@ -3087,7 +3087,7 @@  dw2_do_instantiate_symtab (struct dwarf2_per_cu_data *per_cu)
       : (per_cu->v.psymtab == NULL || !per_cu->v.psymtab->readin))
     {
       queue_comp_unit (per_cu, language_minimal);
-      load_cu (per_cu);
+      load_cu (per_cu, skip_partial);
 
       /* If we just loaded a CU from a DWO, and we're working with an index
 	 that may badly handle TUs, load all the TUs in that DWO as well.
@@ -3116,14 +3116,14 @@  dw2_do_instantiate_symtab (struct dwarf2_per_cu_data *per_cu)
    table.  */
 
 static struct compunit_symtab *
-dw2_instantiate_symtab (struct dwarf2_per_cu_data *per_cu)
+dw2_instantiate_symtab (struct dwarf2_per_cu_data *per_cu, bool skip_partial)
 {
   gdb_assert (dwarf2_per_objfile->using_index);
   if (!per_cu->v.quick->compunit_symtab)
     {
       struct cleanup *back_to = make_cleanup (free_cached_comp_units, NULL);
       scoped_restore decrementer = increment_reading_symtab ();
-      dw2_do_instantiate_symtab (per_cu);
+      dw2_do_instantiate_symtab (per_cu, skip_partial);
       process_cu_includes ();
       do_cleanups (back_to);
     }
@@ -4002,7 +4002,7 @@  dw2_find_last_source_symtab (struct objfile *objfile)
 
   dw2_setup (objfile);
   index = dwarf2_per_objfile->n_comp_units - 1;
-  cust = dw2_instantiate_symtab (dw2_get_cutu (index));
+  cust = dw2_instantiate_symtab (dw2_get_cutu (index), false);
   if (cust == NULL)
     return NULL;
   return compunit_primary_filetab (cust);
@@ -4055,7 +4055,7 @@  dw2_map_expand_apply (struct objfile *objfile,
 
   /* This may expand more than one symtab, and we want to iterate over
      all of them.  */
-  dw2_instantiate_symtab (per_cu);
+  dw2_instantiate_symtab (per_cu, false);
 
   return iterate_over_some_symtabs (name, real_path, objfile->compunit_symtabs,
 				    last_made, callback);
@@ -4302,7 +4302,8 @@  dw2_lookup_symbol (struct objfile *objfile, int block_index,
       while ((per_cu = dw2_symtab_iter_next (&iter)) != NULL)
 	{
 	  struct symbol *sym, *with_opaque = NULL;
-	  struct compunit_symtab *stab = dw2_instantiate_symtab (per_cu);
+	  struct compunit_symtab *stab = dw2_instantiate_symtab (per_cu,
+								 false);
 	  const struct blockvector *bv = COMPUNIT_BLOCKVECTOR (stab);
 	  struct block *block = BLOCKVECTOR_BLOCK (bv, block_index);
 
@@ -4397,7 +4398,7 @@  dw2_expand_symtabs_for_function (struct objfile *objfile,
 			    func_name);
 
       while ((per_cu = dw2_symtab_iter_next (&iter)) != NULL)
-	dw2_instantiate_symtab (per_cu);
+	dw2_instantiate_symtab (per_cu, false);
     }
 }
 
@@ -4413,7 +4414,12 @@  dw2_expand_all_symtabs (struct objfile *objfile)
     {
       struct dwarf2_per_cu_data *per_cu = dw2_get_cutu (i);
 
-      dw2_instantiate_symtab (per_cu);
+      /* We don't want to directly expand a partial CU, because if we
+	 read it with the wrong language, then assertion failures can
+	 be triggered later on.  See PR symtab/23010.  So, tell
+	 dw2_instantiate_symtab to skip partial CUs -- any important
+	 partial CU will be read via DW_TAG_imported_unit anyway.  */
+      dw2_instantiate_symtab (per_cu, true);
     }
 }
 
@@ -4450,7 +4456,7 @@  dw2_expand_symtabs_with_fullname (struct objfile *objfile,
 
 	  if (filename_cmp (this_fullname, fullname) == 0)
 	    {
-	      dw2_instantiate_symtab (per_cu);
+	      dw2_instantiate_symtab (per_cu, false);
 	      break;
 	    }
 	}
@@ -5274,7 +5280,7 @@  dw2_expand_symtabs_matching_one
       bool symtab_was_null
 	= (per_cu->v.quick->compunit_symtab == NULL);
 
-      dw2_instantiate_symtab (per_cu);
+      dw2_instantiate_symtab (per_cu, false);
 
       if (expansion_notify != NULL
 	  && symtab_was_null
@@ -5529,7 +5535,8 @@  dw2_find_pc_sect_compunit_symtab (struct objfile *objfile,
 	     paddress (get_objfile_arch (objfile), pc));
 
   result
-    = recursively_find_pc_sect_compunit_symtab (dw2_instantiate_symtab (data),
+    = recursively_find_pc_sect_compunit_symtab (dw2_instantiate_symtab (data,
+									false),
 						pc);
   gdb_assert (result != NULL);
   return result;
@@ -6345,7 +6352,7 @@  dw2_debug_names_lookup_symbol (struct objfile *objfile, int block_index_int,
   while ((per_cu = iter.next ()) != NULL)
     {
       struct symbol *sym, *with_opaque = NULL;
-      struct compunit_symtab *stab = dw2_instantiate_symtab (per_cu);
+      struct compunit_symtab *stab = dw2_instantiate_symtab (per_cu, false);
       const struct blockvector *bv = COMPUNIT_BLOCKVECTOR (stab);
       struct block *block = BLOCKVECTOR_BLOCK (bv, block_index);
 
@@ -6404,7 +6411,7 @@  dw2_debug_names_expand_symtabs_for_function (struct objfile *objfile,
 
       struct dwarf2_per_cu_data *per_cu;
       while ((per_cu = iter.next ()) != NULL)
-	dw2_instantiate_symtab (per_cu);
+	dw2_instantiate_symtab (per_cu, false);
     }
 }
 
@@ -7725,6 +7732,7 @@  static void
 init_cutu_and_read_dies (struct dwarf2_per_cu_data *this_cu,
 			 struct abbrev_table *abbrev_table,
 			 int use_existing_cu, int keep,
+			 bool skip_partial,
 			 die_reader_func_ftype *die_reader_func,
 			 void *data)
 {
@@ -7875,6 +7883,9 @@  init_cutu_and_read_dies (struct dwarf2_per_cu_data *this_cu,
   init_cu_die_reader (&reader, cu, section, NULL);
   info_ptr = read_full_die (&reader, &comp_unit_die, info_ptr, &has_children);
 
+  if (skip_partial && comp_unit_die->tag == DW_TAG_partial_unit)
+    return;
+
   /* If we are in a DWO stub, process it and then read in the "real" CU/TU
      from the DWO file.
      Note that if USE_EXISTING_OK != 0, and THIS_CU->cu already contains a
@@ -8379,14 +8390,14 @@  process_psymtab_comp_unit (struct dwarf2_per_cu_data *this_cu,
     free_one_cached_comp_unit (this_cu);
 
   if (this_cu->is_debug_types)
-    init_cutu_and_read_dies (this_cu, NULL, 0, 0, build_type_psymtabs_reader,
-			     NULL);
+    init_cutu_and_read_dies (this_cu, NULL, 0, 0, false,
+			     build_type_psymtabs_reader, NULL);
   else
     {
       process_psymtab_comp_unit_data info;
       info.want_partial_unit = want_partial_unit;
       info.pretend_language = pretend_language;
-      init_cutu_and_read_dies (this_cu, NULL, 0, 0,
+      init_cutu_and_read_dies (this_cu, NULL, 0, 0, false,
 			       process_psymtab_comp_unit_reader, &info);
     }
 
@@ -8562,7 +8573,7 @@  build_type_psymtabs_1 (void)
 	}
 
       init_cutu_and_read_dies (&tu->sig_type->per_cu, abbrev_table, 0, 0,
-			       build_type_psymtabs_reader, NULL);
+			       false, build_type_psymtabs_reader, NULL);
     }
 
   do_cleanups (cleanups);
@@ -8668,7 +8679,7 @@  process_skeletonless_type_unit (void **slot, void *info)
   *slot = entry;
 
   /* This does the job that build_type_psymtabs_1 would have done.  */
-  init_cutu_and_read_dies (&entry->per_cu, NULL, 0, 0,
+  init_cutu_and_read_dies (&entry->per_cu, NULL, 0, 0, false,
 			   build_type_psymtabs_reader, NULL);
 
   return 1;
@@ -8827,7 +8838,7 @@  load_partial_comp_unit_reader (const struct die_reader_specs *reader,
 static void
 load_partial_comp_unit (struct dwarf2_per_cu_data *this_cu)
 {
-  init_cutu_and_read_dies (this_cu, NULL, 1, 1,
+  init_cutu_and_read_dies (this_cu, NULL, 1, 1, false,
 			   load_partial_comp_unit_reader, NULL);
 }
 
@@ -9974,7 +9985,7 @@  psymtab_to_symtab_1 (struct partial_symtab *pst)
       return;
     }
 
-  dw2_do_instantiate_symtab (per_cu);
+  dw2_do_instantiate_symtab (per_cu, false);
 }
 
 /* Trivial hash function for die_info: the hash value of a DIE
@@ -10043,11 +10054,12 @@  load_full_comp_unit_reader (const struct die_reader_specs *reader,
 
 static void
 load_full_comp_unit (struct dwarf2_per_cu_data *this_cu,
+		     bool skip_partial,
 		     enum language pretend_language)
 {
   gdb_assert (! this_cu->is_debug_types);
 
-  init_cutu_and_read_dies (this_cu, NULL, 1, 1,
+  init_cutu_and_read_dies (this_cu, NULL, 1, 1, skip_partial,
 			   load_full_comp_unit_reader, &pretend_language);
 }
 
@@ -10568,7 +10580,7 @@  process_imported_unit_die (struct die_info *die, struct dwarf2_cu *cu)
 
       /* If necessary, add it to the queue and load its DIEs.  */
       if (maybe_queue_comp_unit (cu, per_cu, cu->language))
-	load_full_comp_unit (per_cu, cu->language);
+	load_full_comp_unit (per_cu, false, cu->language);
 
       VEC_safe_push (dwarf2_per_cu_ptr, cu->per_cu->imported_symtabs,
 		     per_cu);
@@ -19654,7 +19666,7 @@  dwarf2_read_addr_index (struct dwarf2_per_cu_data *per_cu,
 
       /* Note: We can't use init_cutu_and_read_dies_simple here,
 	 we need addr_base.  */
-      init_cutu_and_read_dies (per_cu, NULL, 0, 0,
+      init_cutu_and_read_dies (per_cu, NULL, 0, 0, false,
 			       dwarf2_read_addr_index_reader, &aidata);
       addr_base = aidata.addr_base;
       addr_size = aidata.addr_size;
@@ -22852,7 +22864,7 @@  follow_die_offset (sect_offset sect_off, int offset_in_dwz,
 
       /* If necessary, add it to the queue and load its DIEs.  */
       if (maybe_queue_comp_unit (cu, per_cu, cu->language))
-	load_full_comp_unit (per_cu, cu->language);
+	load_full_comp_unit (per_cu, false, cu->language);
 
       target_cu = per_cu->cu;
     }
@@ -22860,7 +22872,7 @@  follow_die_offset (sect_offset sect_off, int offset_in_dwz,
     {
       /* We're loading full DIEs during partial symbol reading.  */
       gdb_assert (dwarf2_per_objfile->reading_partial_symbols);
-      load_full_comp_unit (cu->per_cu, language_minimal);
+      load_full_comp_unit (cu->per_cu, false, language_minimal);
     }
 
   *ref_cu = target_cu;
@@ -22913,7 +22925,7 @@  dwarf2_fetch_die_loc_sect_off (sect_offset sect_off,
   dw2_setup (per_cu->objfile);
 
   if (per_cu->cu == NULL)
-    load_cu (per_cu);
+    load_cu (per_cu, false);
   cu = per_cu->cu;
   if (cu == NULL)
     {
@@ -23021,7 +23033,7 @@  dwarf2_fetch_constant_bytes (sect_offset sect_off,
   dw2_setup (per_cu->objfile);
 
   if (per_cu->cu == NULL)
-    load_cu (per_cu);
+    load_cu (per_cu, false);
   cu = per_cu->cu;
   if (cu == NULL)
     {
@@ -23146,7 +23158,7 @@  dwarf2_fetch_die_type_sect_off (sect_offset sect_off,
   dw2_setup (per_cu->objfile);
 
   if (per_cu->cu == NULL)
-    load_cu (per_cu);
+    load_cu (per_cu, false);
   cu = per_cu->cu;
   if (!cu)
     return NULL;
@@ -23421,7 +23433,7 @@  read_signatured_type (struct signatured_type *sig_type)
   gdb_assert (per_cu->is_debug_types);
   gdb_assert (per_cu->cu == NULL);
 
-  init_cutu_and_read_dies (per_cu, NULL, 0, 1,
+  init_cutu_and_read_dies (per_cu, NULL, 0, 1, false,
 			   read_signatured_type_reader, NULL);
   sig_type->per_cu.tu_read = 1;
 }