possible fix for PR symtab/23010

possible fix for PR symtab/23010

[Tom Tromey]

I was talking with Keith & Pedro about PR symtab/23010 on irc this week,
because it was the bug at the base of the Rust -readnow problem that Jan
reported.

I came up with this patch yesterday.  It fixes the problem, but I didn't
write a test and also I'm not sure if it is the best way to go.

So, I thought I'd send it for commentary.

Tom

commit bc5411ff15de7e207bfb80a42790adb5d6f80852
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-04-12  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.

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index f2bb3d0af3..46e3c9d6cd 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,27 @@
 2018-04-12  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-04-12  Tom Tromey  <tom@tromey.com>
+
 	* dwarf2read.c (read_structure_type): Set TYPE_NAME for rust
 	unions.
 
diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index e3f544a6a4..406aa0d52e 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -1832,7 +1832,7 @@ static void create_all_comp_units (struct dwarf2_per_objfile *dwarf2_per_objfile
 
 static int create_all_type_units (struct dwarf2_per_objfile *dwarf2_per_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 *,
@@ -1932,7 +1932,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
@@ -2835,12 +2835,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.  */
@@ -2851,7 +2851,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 dwarf2_per_objfile *dwarf2_per_objfile = per_cu->dwarf2_per_objfile;
 
@@ -2870,7 +2870,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, true);
 
       /* 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.
@@ -2897,7 +2897,7 @@ 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)
 {
   struct dwarf2_per_objfile *dwarf2_per_objfile = per_cu->dwarf2_per_objfile;
 
@@ -2906,7 +2906,7 @@ dw2_instantiate_symtab (struct dwarf2_per_cu_data *per_cu)
     {
       free_cached_comp_units freer (dwarf2_per_objfile);
       scoped_restore decrementer = increment_reading_symtab ();
-      dw2_do_instantiate_symtab (per_cu);
+      dw2_do_instantiate_symtab (per_cu, skip_partial);
       process_cu_includes (dwarf2_per_objfile);
     }
 
@@ -3741,7 +3741,7 @@ dw2_find_last_source_symtab (struct objfile *objfile)
   struct dwarf2_per_objfile *dwarf2_per_objfile
     = get_dwarf2_per_objfile (objfile);
   dwarf2_per_cu_data *dwarf_cu = dwarf2_per_objfile->all_comp_units.back ();
-  compunit_symtab *cust = dw2_instantiate_symtab (dwarf_cu);
+  compunit_symtab *cust = dw2_instantiate_symtab (dwarf_cu, false);
 
   if (cust == NULL)
     return NULL;
@@ -3797,7 +3797,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);
@@ -4037,7 +4037,7 @@ 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);
 
@@ -4128,7 +4128,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);
 
 }
 
@@ -4144,7 +4144,7 @@ dw2_expand_all_symtabs (struct objfile *objfile)
     {
       dwarf2_per_cu_data *per_cu = dwarf2_per_objfile->get_cutu (i);
 
-      dw2_instantiate_symtab (per_cu);
+      dw2_instantiate_symtab (per_cu, true);
     }
 }
 
@@ -4176,7 +4176,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;
 	    }
 	}
@@ -5000,7 +5000,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
@@ -5250,7 +5250,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;
@@ -6049,7 +6050,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);
 
@@ -6111,7 +6112,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);
     }
 }
 
@@ -7410,6 +7411,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)
 {
@@ -7548,6 +7550,9 @@ init_cutu_and_read_dies (struct dwarf2_per_cu_data *this_cu,
   init_cu_die_reader (&reader, cu, section, NULL, abbrev_table);
   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.  read_cutu_die_from_dwo will allocate the abbreviation
      table from the DWO file and pass the ownership over to us.  It will be
@@ -8042,14 +8047,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);
     }
 
@@ -8209,7 +8214,7 @@ build_type_psymtabs_1 (struct dwarf2_per_objfile *dwarf2_per_objfile)
 	}
 
       init_cutu_and_read_dies (&tu.sig_type->per_cu, abbrev_table.get (),
-			       0, 0, build_type_psymtabs_reader, NULL);
+			       0, 0, false, build_type_psymtabs_reader, NULL);
     }
 }
 
@@ -8316,7 +8321,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;
@@ -8464,7 +8469,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);
 }
 
@@ -9558,7 +9563,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
@@ -9627,11 +9632,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);
 }
 
@@ -10458,7 +10464,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);
@@ -19569,7 +19575,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;
@@ -22776,7 +22782,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;
     }
@@ -22784,7 +22790,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;
@@ -22838,7 +22844,7 @@ dwarf2_fetch_die_loc_sect_off (sect_offset sect_off,
   struct objfile *objfile = dwarf2_per_objfile->objfile;
 
   if (per_cu->cu == NULL)
-    load_cu (per_cu);
+    load_cu (per_cu, false);
   cu = per_cu->cu;
   if (cu == NULL)
     {
@@ -22945,7 +22951,7 @@ dwarf2_fetch_constant_bytes (sect_offset sect_off,
   struct objfile *objfile = per_cu->dwarf2_per_objfile->objfile;
 
   if (per_cu->cu == NULL)
-    load_cu (per_cu);
+    load_cu (per_cu, false);
   cu = per_cu->cu;
   if (cu == NULL)
     {
@@ -23067,7 +23073,7 @@ dwarf2_fetch_die_type_sect_off (sect_offset sect_off,
   struct die_info *die;
 
   if (per_cu->cu == NULL)
-    load_cu (per_cu);
+    load_cu (per_cu, false);
   cu = per_cu->cu;
   if (!cu)
     return NULL;
@@ -23348,7 +23354,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;
 }

Re: possible fix for PR symtab/23010

[Keith Seitz]

On 04/12/2018 12:06 PM, Tom Tromey wrote:
> I was talking with Keith & Pedro about PR symtab/23010 on irc this week,
> because it was the bug at the base of the Rust -readnow problem that Jan
> reported.
> 
> I came up with this patch yesterday.  It fixes the problem, but I didn't
> write a test and also I'm not sure if it is the best way to go.
> 
> So, I thought I'd send it for commentary.

I've looked into this quite a bit, too. In my case, I was looking specifically at the assertion caused by passing "-readnow" with libwebkit.so.debug on Fedora (Jan's reproducer).

I tried for darn near a week to come up with an isolated reproducer to no avail. :-(

Based on what I was seeing, I came up with a different approach, but I don't care for it at all. I find the proposed solution a whole lot less risky. [My approach was to have language_minimal CUs "inherit" the importing CU's language in inherit_abstract_dies. I can dream up a few instances where this might be problematic. I don't know if they're really legitimate use cases, but nothing in the standard specifically discredits my imaginings.]

>     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.

It is. :-)

>     2018-04-12  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.

Aside: didn't we decide that "update all callers" was sufficient? [I'm only curious -- not asking for changes to a silly ChangeLog.]

This patch looks reasonable to me, but I would ask you to consider mentioning why partial_units are skipped where they are (even if to just reference the problem or bug?). That's these two hunks, I think:

> diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
> index e3f544a6a4..406aa0d52e 100644
> --- a/gdb/dwarf2read.c
> +++ b/gdb/dwarf2read.c
> @@ -2870,7 +2870,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, true);
>  
>        /* 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.

and

> @@ -4144,7 +4144,7 @@ dw2_expand_all_symtabs (struct objfile *objfile)
>      {
>        dwarf2_per_cu_data *per_cu = dwarf2_per_objfile->get_cutu (i);
>  
> -      dw2_instantiate_symtab (per_cu);
> +      dw2_instantiate_symtab (per_cu, true);
>      }
>  }
>  

I've been manually testing this patch with everything that I can think of on libwebkit.so, and I cannot trigger anything ill-behaved at all.

I recommend a maintainer approve this, even if it is more a band-aid than a "proper" fix. It fixes a fairly big (and maybe even common) problem with minimal impact/risk.

Keith

Re: possible fix for PR symtab/23010

[Tom Tromey]

>>>>> "Keith" == Keith Seitz <keiths@redhat.com> writes:

Keith> I tried for darn near a week to come up with an isolated
Keith> reproducer to no avail. :-(

If you have a test case, could you send it, even it if doesn't work?
I could try to poke at it a little.

Keith> Aside: didn't we decide that "update all callers" was sufficient? [I'm
Keith> only curious -- not asking for changes to a silly ChangeLog.]

Yeah, old habits I guess.

Keith> This patch looks reasonable to me, but I would ask you to consider
Keith> mentioning why partial_units are skipped where they are (even if to
Keith> just reference the problem or bug?). That's these two hunks, I think:

Good idea, will do.

Tom

Re: possible fix for PR symtab/23010

[Joel Brobecker]

Hello,


> > I was talking with Keith & Pedro about PR symtab/23010 on irc this week,
> > because it was the bug at the base of the Rust -readnow problem that Jan
> > reported.
> > 
> > I came up with this patch yesterday.  It fixes the problem, but I didn't
> > write a test and also I'm not sure if it is the best way to go.
> > 
> > So, I thought I'd send it for commentary.

I am not sure either, but I can't think of anything else.

> >     2018-04-12  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.
[...]
> This patch looks reasonable to me, but I would ask you to consider
> mentioning why partial_units are skipped where they are (even if to
> just reference the problem or bug?). That's these two hunks, I think:

+1.

> I've been manually testing this patch with everything that I can think
> of on libwebkit.so, and I cannot trigger anything ill-behaved at all.
> 
> I recommend a maintainer approve this, even if it is more a band-aid
> than a "proper" fix. It fixes a fairly big (and maybe even common)
> problem with minimal impact/risk.

I reviewed the patch as best as I could, but as Tom says, it's hard
to reason. But at the same time, it was conservative, as the new
param is false by default except in a couple of cases.

I'd love for another maintainer to take a look, especially if we are
going to consider this patch for inclusion in 8.1.1. But I can't
think of someone who was actively involved in this area.

Considering the fact that this has had two reviews, and also that
it comes from you, whom I trust quite a bit for changes in this area,
let's give others a week to provide comments. Failing that, let's
push it, to see how well it fares.

We may decide to skip this bug for 8.1.1, though. Although, thinking
aloud, if there was any regression caused by it, it would be with units
which haven't been expanded yet, right? A workaround would be to trigger
the unit's expansion, which seems easy enough. So, small risk vs
no-crash reward.... Hmmm...

-- 
Joel

Re: possible fix for PR symtab/23010

[Joel Brobecker]

Hello Global Maintainers,

I was wondering if anyone had any thoughts regarding Tom patch.
https://sourceware.org/ml/gdb-patches/2018-04/msg00234.html

Below are my comments on it, and also my interrogations on whether
we might want this patch in 8.1.1 or not.

Additional thoughts:
  - This is a regression
  - This is an internal error, so it can be fairly problematic
  - It only happens with -readnow, it seems, which I assume
    is not widely used considering the performance and memory
    cost of this feature.

I might tip in favor of putting it in, considering the fact that
I don't think there is much of a workaround, but I would not make
that call just on my own, because the patch is far from obvious.


> > >     2018-04-12  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.
> [...]
> > This patch looks reasonable to me, but I would ask you to consider
> > mentioning why partial_units are skipped where they are (even if to
> > just reference the problem or bug?). That's these two hunks, I think:
> 
> +1.
> 
> > I've been manually testing this patch with everything that I can think
> > of on libwebkit.so, and I cannot trigger anything ill-behaved at all.
> > 
> > I recommend a maintainer approve this, even if it is more a band-aid
> > than a "proper" fix. It fixes a fairly big (and maybe even common)
> > problem with minimal impact/risk.
> 
> I reviewed the patch as best as I could, but as Tom says, it's hard
> to reason. But at the same time, it was conservative, as the new
> param is false by default except in a couple of cases.
> 
> I'd love for another maintainer to take a look, especially if we are
> going to consider this patch for inclusion in 8.1.1. But I can't
> think of someone who was actively involved in this area.
> 
> Considering the fact that this has had two reviews, and also that
> it comes from you, whom I trust quite a bit for changes in this area,
> let's give others a week to provide comments. Failing that, let's
> push it, to see how well it fares.
> 
> We may decide to skip this bug for 8.1.1, though. Although, thinking
> aloud, if there was any regression caused by it, it would be with units
> which haven't been expanded yet, right? A workaround would be to trigger
> the unit's expansion, which seems easy enough. So, small risk vs
> no-crash reward.... Hmmm...
> 
> -- 
> Joel

-- 
Joel

Re: possible fix for PR symtab/23010

[Joel Brobecker]

Hi Tom,

On Mon, May 07, 2018 at 10:13:09AM -0700, Joel Brobecker wrote:
> Hello Global Maintainers,
> 
> I was wondering if anyone had any thoughts regarding Tom patch.
> https://sourceware.org/ml/gdb-patches/2018-04/msg00234.html
> 
> Below are my comments on it, and also my interrogations on whether
> we might want this patch in 8.1.1 or not.
> 
> Additional thoughts:
>   - This is a regression
>   - This is an internal error, so it can be fairly problematic
>   - It only happens with -readnow, it seems, which I assume
>     is not widely used considering the performance and memory
>     cost of this feature.
> 
> I might tip in favor of putting it in, considering the fact that
> I don't think there is much of a workaround, but I would not make
> that call just on my own, because the patch is far from obvious.

Considering that the patch has been under review for 1 month, and
has been available for additional comments for a couple of weeks
since my review, and that this patch is a potential for inclusion
in 8.1.1, I propose we rebase, make whatever tiny adjustments that
were discussed, and start by pushing it to master.

As mentioned in my weekly update of the 8.1.1 release, we can then
wait a week or two before deciding whether we want it in 8.1.1 or not.

WDYT?

Thanks!
-- 
Joel

Re: possible fix for PR symtab/23010

[Tom Tromey]

>>>>> "Keith" == Keith Seitz <keiths@redhat.com> writes:

Keith> This patch looks reasonable to me, but I would ask you to consider
Keith> mentioning why partial_units are skipped where they are (even if to
Keith> just reference the problem or bug?). That's these two hunks, I think:

>> diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
>> index e3f544a6a4..406aa0d52e 100644
>> --- a/gdb/dwarf2read.c
>> +++ b/gdb/dwarf2read.c
>> @@ -2870,7 +2870,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, true);
>> 
>> /* 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.

This use of "true" actually seems weird to me on re-reading.
dw2_do_instantiate_symtab doesn't actually use the skip_partial
parameter I added.

So, I changed this to pass skip_partial to load_cu.
I verified that the -readnow test on libwebkit2gtk still works.

>> @@ -4144,7 +4144,7 @@ dw2_expand_all_symtabs (struct objfile *objfile)
>> {
>> dwarf2_per_cu_data *per_cu = dwarf2_per_objfile->get_cutu (i);
>> 
>> -      dw2_instantiate_symtab (per_cu);
>> +      dw2_instantiate_symtab (per_cu, true);
>> }
>> }
>> 

Here I added this comment:

      /* 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.  */


I looked at your test case a bit but I also couldn't make it fail.

I'm pushing this patch in now.

Tom

Re: possible fix for PR symtab/23010

[Sergio Durigan Junior]

On Monday, May 07 2018, Joel Brobecker wrote:

> I was wondering if anyone had any thoughts regarding Tom patch.
> https://sourceware.org/ml/gdb-patches/2018-04/msg00234.html
>
> Below are my comments on it, and also my interrogations on whether
> we might want this patch in 8.1.1 or not.
>
> Additional thoughts:
>   - This is a regression
>   - This is an internal error, so it can be fairly problematic
>   - It only happens with -readnow, it seems, which I assume
>     is not widely used considering the performance and memory
>     cost of this feature.
>
> I might tip in favor of putting it in, considering the fact that
> I don't think there is much of a workaround, but I would not make
> that call just on my own, because the patch is far from obvious.

[ CCing Pedro.  ]

Hey Joel,

Just my two cents here.  This patch apparently fixes a bunch of bugs
filed against Fedora GDB, which may indicate that either (a) it's not
related only to -readnow, or (b) more people use -readnow than we know
of ;-).

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

Cheers,

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

Re: possible fix for PR symtab/23010

[Tom Tromey]

>>>>> "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.

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;
 }

Re: possible fix for PR symtab/23010

[Tom Tromey]

>>>>> "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

Re: possible fix for PR symtab/23010

[Sergio Durigan Junior]

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.

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

Re: possible fix for PR symtab/23010

[Keith Seitz]

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

Re: possible fix for PR symtab/23010

[Keith Seitz]

[WARNING: Very long explanation enclosed. Skip to end if interested in conclusions.]

On 05/24/2018 07:48 AM, Keith Seitz wrote:
> 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.

Okay, I have a patch, but I still don't have a test case for it (yet?). The problem is that most of the time, the bug is latent. It is non-trivial to tickle the bug. [But WebKit does it *very well*.]

As a reminder, the symptom manifested here is the assertion that DICT_LANGUAGE == SYMBOL_LANGUAGE. The webkit reproducer in 1574015 shows that we are attempting to add a language_minimal symbol to a language_cplus dictionary.

The root cause of this, of course, is that SYMBOL_LANGUAGE shouldn't be language_minimal (duh). That's just the default "pretend" language that is set for the containing CU. So the question, is *why* isn't the CU's language set?

I changed add_symbol_to_list to print out a message when the symbol's language was language_minimal. WebKit shows many hundreds (thousands?) of these messages, so it is a prevalent problem -- not all of which actually cause any problems (that we know of).

After a lot of investigation, here is why this is happening.

The backtrace command (thread apply all bt full) that we're running is looking for the compunit containing the PC of the thread. That calls get_prev_frame several times. This function calls (eventually) dwarf2_frame_prev_register. That eventually ends up calling find_pc_compunit_symtab.

In this function (find_pc_sect_compunit_symtab actually), we loop over all compunits, calling the "quick" function dw2_find_pc_sect_compunit_symtab. That function calls dw2_instantiate_symtab to read in all the CU's symbols. Now the fun begins.

dw2_do_instantiate_symtab queues the per_cu for reading, using a default "pretend" language of language_minimal with the expectation that this will be set later.

The DIEs of this (only queued) CU are then processed.

The first DIE is DW_TAG_compile_unit. That's handled by read_file_scope.

(Nearly) The first thing read_file_scope does is:

  get_scope_pc_bounds (die, &lowpc, &highpc, cu);

This function loops over the children of the current DIE (a compile_unit), looking for bounds. The first such child is a subprogram, and we attempt to get its bounds. We use dwarf2_attr to get at DW_AT_high_pc.

This subprogram has DW_AT_specification set, so dwarf_attr (via follow_die_ref/follow_die_offset) will follow that, but follow_die_offset *also* attempts to load the containing CU for the spec DIE. That spec DIE lives inside a CU that is a partial_unit and has no language attribute. So it simply inherits the language from the CU that elicited the read. [That all happens in follow_die_offset.]

The original CU's language is still language_minimal -- we haven't gotten to the line in read_file_scope that actually sets the language yet!

And that is the cause of these problems. The call to prepare_one_comp_unit needs to be the *first* thing that is done when reading a CU so that the CU's language can be recorded (and inherited by any referenced partial_units).

So, alas, this is the near trivial patch to fix this dictionary/symbol assertion in insert_symbol_hashed:

diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index 49ce83ff20..0145c83b30 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -11470,6 +11470,8 @@ read_file_scope (struct die_info *die, struct dwarf2_cu *cu)
   struct die_info *child_die;
   CORE_ADDR baseaddr;
 
+  prepare_one_comp_unit (cu, die, cu->language);
+
   baseaddr = ANOFFSET (objfile->section_offsets, SECT_OFF_TEXT (objfile));
 
   get_scope_pc_bounds (die, &lowpc, &highpc, cu);
@@ -11482,8 +11484,6 @@ read_file_scope (struct die_info *die, struct dwarf2_cu *cu)
 
   file_and_directory fnd = find_file_and_directory (die, cu);
 
-  prepare_one_comp_unit (cu, die, cu->language);
-
   /* The XLCL doesn't generate DW_LANG_OpenCL because this attribute is not
      standardised yet.  As a workaround for the language detection we fall
      back to the DW_AT_producer string.  */

This fixes 1574015 and all the -readnow problems reported against WebKit (such as rhbz1560010/symtab/23010).
[I have also verified that all my language_minimal printfs in add_symbol_to_list are now gone.]

But I still haven't come up with a reduced reproducer, and I'm exhausted, and not just from writing this longer-than-needed message. :-P

Keith

Re: possible fix for PR symtab/23010

[Tom Tromey]

>>>>> "Keith" == Keith Seitz <keiths@redhat.com> writes:

Keith> And that is the cause of these problems. The call to
Keith> prepare_one_comp_unit needs to be the *first* thing that is done
Keith> when reading a CU so that the CU's language can be recorded (and
Keith> inherited by any referenced partial_units).

Nice work debugging this.

Tom

Re: possible fix for PR symtab/23010

[Joel Brobecker]

> [WARNING: Very long explanation enclosed. Skip to end if interested in
> conclusions.]

This is extremely useful in understanding the source of the problem,
and therefore the solution. A bit thank you for writing it up.

> And that is the cause of these problems. The call to
> prepare_one_comp_unit needs to be the *first* thing that is done when
> reading a CU so that the CU's language can be recorded (and inherited
> by any referenced partial_units).
> 
> So, alas, this is the near trivial patch to fix this dictionary/symbol
> assertion in insert_symbol_hashed:

I agree with Tom; very nice investigative work, and the near trivial
patch feels like gravy.

> 
> diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
> index 49ce83ff20..0145c83b30 100644
> --- a/gdb/dwarf2read.c
> +++ b/gdb/dwarf2read.c
> @@ -11470,6 +11470,8 @@ read_file_scope (struct die_info *die, struct dwarf2_cu *cu)
>    struct die_info *child_die;
>    CORE_ADDR baseaddr;
>  
> +  prepare_one_comp_unit (cu, die, cu->language);
> +
>    baseaddr = ANOFFSET (objfile->section_offsets, SECT_OFF_TEXT (objfile));
>  
>    get_scope_pc_bounds (die, &lowpc, &highpc, cu);
> @@ -11482,8 +11484,6 @@ read_file_scope (struct die_info *die, struct dwarf2_cu *cu)
>  
>    file_and_directory fnd = find_file_and_directory (die, cu);
>  
> -  prepare_one_comp_unit (cu, die, cu->language);
> -
>    /* The XLCL doesn't generate DW_LANG_OpenCL because this attribute is not
>       standardised yet.  As a workaround for the language detection we fall
>       back to the DW_AT_producer string.  */
> 

I looked at the patch, and in particular, checked to see if there
is anything we were doing ahead of the call to prepare_one_comp_unit
that prepare_one_comp_unit would also need. But this function
does very little, basically reading the DW_AT_language and
the DW_AT_producer attributes. So I don't see how this could have
some negative side-effect, or any hints of why it might have been
placed slight later in read_file_scope's body.

A testcase in this particular case, where the order in which we do
things is important, would be very useful in avoiding a regression.
However, considering how specific the conditions need to be in order
to trigger the bug, and considering the current timing, I (personally)
think that we should not let best be the enemy of good, and allow
this patch in if Keith fails to create a testcase after a reasonable
amount of effort.

-- 
Joel