[PATCH][gdb/symtab] Fix partial unit psymtabs

[PATCH][gdb/symtab] Fix partial unit psymtabs

[Tom de Vries]

Hi,

Consider test-case gdb.dwarf2/imported-unit.exp.

It contains a CU with type int:
...
 <0><129>: Abbrev Number: 2 (DW_TAG_compile_unit)
    <12a>   DW_AT_language    : 4       (C++)
    <12b>   DW_AT_name        : imported_unit.c
 <1><13b>: Abbrev Number: 3 (DW_TAG_base_type)
    <13c>   DW_AT_byte_size   : 4
    <13d>   DW_AT_encoding    : 5       (signed)
    <13e>   DW_AT_name        : int
...
which is imported in another CU:
...
 <0><d2>: Abbrev Number: 2 (DW_TAG_compile_unit)
    <d3>   DW_AT_language    : 4        (C++)
    <d4>   DW_AT_name        : <artificial>
 <1><e1>: Abbrev Number: 3 (DW_TAG_imported_unit)
    <e2>   DW_AT_import      : <0x129>  [Abbrev Number: 2]
...

However, if we print the partial symbols:
...
$ gdb -batch imported-unit  -ex "maint print psymbols"
...
we see type int both in the importing CU:
...
Partial symtab for source file <artificial>@0xc7 (object 0x29f9b80)
  ...
  Depends on 1 other partial symtabs.
    0 0x2a24240 imported_unit.c
  Global partial symbols:
    `main', function, 0x4004b2
  Static partial symbols:
    `int', type, 0x0
...
and in the imported CU:
...
Partial symtab for source file imported_unit.c (object 0x2a24240)
  ...
  Depends on 0 other partial symtabs.
  Shared partial symtab with user 0x29f9b80
  Static partial symbols:
    `int', type, 0x0
...

This is an artefact resulting from the fact that all CUs in an objfile
share the same storage array for static partial symbols (and another array for
global partial symbols), using a range to describe their symbols.

Then when scanning the partial symbols of a CU and encountering an import, either:
- the referred CU has not been parsed yet, and will be parsed, and the range of
  static partial symbols of the referred CU will be a subrange of the range of
  static partial symbols of this CU, or
- the referred CU has already been parsed, and the range of static partial
  symbols of the referred CU will not be a subrange of the range of static
  partial symbols of this CU.

This is inconsistent handling, and confuses the notion of a symbol belonging to
a single symtab.

Furthermore, it might slow down searches, given that the symbol needs to be
skipped twice.

Finally, the same issue holds for global partial symbols, where the range of a
CU is sorted after parsing is finished.  Obviously sorting the range of a CU
may invalidate subranges, effectively moving symbols in and out of imported
CUs.

Fix this for both static and global partial symbols, by gathering partial
symbols in a per-CU vector, and adding those symbols to the per-objfile
storage only once complete.

Tested on x86_64-linux, with native and board cc-with-dwz and cc-with-dwz-m.

OK for trunk?

Thanks,
- Tom

[gdb/symtab] Fix partial unit psymtabs

gdb/ChangeLog:

2020-03-10  Tom de Vries  <tdevries@suse.de>

	PR symtab/25646
	* psymtab.c (partial_symtab::partial_symtab): Don't set
	globals_offset and statics_offset.  Push element onto
	current_global_psymbols and current_static_psymbols stacks.
	(concat): New function.
	(end_psymtab_common): Set globals_offset and statics_offset.  Pop
	element from current_global_psymbols and current_static_psymbols
	stacks.  Concat popped elements to global_psymbols and
	static_symbols.
	(add_psymbol_to_list): Use current_global_psymbols and
	current_static_psymbols stacks.
	* psymtab.h (class psymtab_storage): Add current_global_psymbols and
	current_static_psymbols fields.

gdb/testsuite/ChangeLog:

2020-03-10  Tom de Vries  <tdevries@suse.de>

	PR symtab/25646
        * gdb.dwarf2/imported-unit.exp: Add test.

---
 gdb/psymtab.c                              | 44 ++++++++++++++++++++++++------
 gdb/psymtab.h                              |  6 ++++
 gdb/testsuite/gdb.dwarf2/imported-unit.exp | 13 +++++++++
 3 files changed, 55 insertions(+), 8 deletions(-)

diff --git a/gdb/psymtab.c b/gdb/psymtab.c
index 93b7c77d1b..f77f6d5108 100644
--- a/gdb/psymtab.c
+++ b/gdb/psymtab.c
@@ -1490,8 +1490,20 @@ partial_symtab::partial_symtab (const char *filename,
 {
   set_text_low (textlow);
   set_text_high (raw_text_low ()); /* default */
-  globals_offset = objfile->partial_symtabs->global_psymbols.size ();
-  statics_offset = objfile->partial_symtabs->static_psymbols.size ();
+
+  auto *v1 = new std::vector<partial_symbol *>;
+  objfile->partial_symtabs->current_global_psymbols.push_back (v1);
+  auto *v2 = new std::vector<partial_symbol *>;
+  objfile->partial_symtabs->current_static_psymbols.push_back (v2);
+}
+
+/* Concat vectors V1 and V2.  */
+
+static void
+concat (std::vector<partial_symbol *> *v1, std::vector<partial_symbol *> *v2)
+{
+  v1->insert (v1->end (), v2->begin (), v2->end ());
+  v2->clear ();
 }
 
 /* Perform "finishing up" operations of a partial symtab.  */
@@ -1499,10 +1511,26 @@ partial_symtab::partial_symtab (const char *filename,
 void
 end_psymtab_common (struct objfile *objfile, struct partial_symtab *pst)
 {
-  pst->n_global_syms = (objfile->partial_symtabs->global_psymbols.size ()
-			- pst->globals_offset);
-  pst->n_static_syms = (objfile->partial_symtabs->static_psymbols.size ()
-			- pst->statics_offset);
+  pst->globals_offset = objfile->partial_symtabs->global_psymbols.size ();
+  pst->statics_offset = objfile->partial_symtabs->static_psymbols.size ();
+
+  auto *current_global_psymbols
+    = objfile->partial_symtabs->current_global_psymbols.back ();
+  auto *current_static_psymbols
+    = objfile->partial_symtabs->current_static_psymbols.back ();
+  objfile->partial_symtabs->current_global_psymbols.pop_back ();
+  objfile->partial_symtabs->current_static_psymbols.pop_back ();
+
+  pst->n_global_syms
+    = current_global_psymbols->size ();
+  pst->n_static_syms
+    = current_static_psymbols->size ();
+
+  concat (&objfile->partial_symtabs->global_psymbols, current_global_psymbols);
+  concat (&objfile->partial_symtabs->static_psymbols, current_static_psymbols);
+
+  delete current_global_psymbols;
+  delete current_static_psymbols;
 
   sort_pst_symbols (objfile, pst);
 }
@@ -1621,8 +1649,8 @@ add_psymbol_to_list (gdb::string_view name, bool copy_name,
   /* Save pointer to partial symbol in psymtab, growing symtab if needed.  */
   std::vector<partial_symbol *> *list
     = (where == psymbol_placement::STATIC
-       ? &objfile->partial_symtabs->static_psymbols
-       : &objfile->partial_symtabs->global_psymbols);
+       ? objfile->partial_symtabs->current_static_psymbols.back ()
+       : objfile->partial_symtabs->current_global_psymbols.back ());
   append_psymbol_to_list (list, psym, objfile);
 }
 
diff --git a/gdb/psymtab.h b/gdb/psymtab.h
index 040b973927..e8bafbe433 100644
--- a/gdb/psymtab.h
+++ b/gdb/psymtab.h
@@ -129,6 +129,12 @@ class psymtab_storage
   std::vector<partial_symbol *> global_psymbols;
   std::vector<partial_symbol *> static_psymbols;
 
+  /* Stack of vectors of partial symbols, using during psymtab
+     initialization.  */
+
+  std::vector<std::vector<partial_symbol *>*> current_global_psymbols;
+  std::vector<std::vector<partial_symbol *>*> current_static_psymbols;
+
 private:
 
   /* The obstack where allocations are made.  This is lazily allocated
diff --git a/gdb/testsuite/gdb.dwarf2/imported-unit.exp b/gdb/testsuite/gdb.dwarf2/imported-unit.exp
index 2b50173bc6..80d6628357 100644
--- a/gdb/testsuite/gdb.dwarf2/imported-unit.exp
+++ b/gdb/testsuite/gdb.dwarf2/imported-unit.exp
@@ -149,6 +149,19 @@ if { [prepare_for_testing "failed to prepare" ${testfile} \
 
 gdb_test_no_output "set language c++"
 
+# Verify that the partial symtab for the unit importing the partial unit does
+# not contain the static partial symbol int, which is defined in the partial
+# unit.  Test-case for PR25646.
+gdb_test "main print psymbols" \
+    [multi_line \
+	 "  Depends on 1 other partial symtabs\." \
+	 "\[^\r\n\]*" \
+	 "  Global partial symbols:" \
+	 "    `main', function, $hex" \
+	 "" \
+	 ".*"] \
+    "no static partial symbols in importing unit"
+
 # Sanity check
 gdb_test "ptype main" "= int \\(void\\)"
 

Re: [PATCH][gdb/symtab] Fix partial unit psymtabs

[Tom Tromey]

>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:

Tom> This is an artefact resulting from the fact that all CUs in an objfile
Tom> share the same storage array for static partial symbols (and another array for
Tom> global partial symbols), using a range to describe their symbols.

Wow, nice catch.  This bug has been there since imported CUs were
implemented.

Tom> +  /* Stack of vectors of partial symbols, using during psymtab
Tom> +     initialization.  */
Tom> +
Tom> +  std::vector<std::vector<partial_symbol *>*> current_global_psymbols;
Tom> +  std::vector<std::vector<partial_symbol *>*> current_static_psymbols;

I mildly don't like putting this here.  Ideally it would be handled
purely in the DWARF reader.  However, I see this as a side effect of the
way that psymtabs are currently managed ("poorly").  I have some patches
that change this, and I suppose this could be modified on that branch.

Meanwhile, this patch is ok.  Thank you for find this and for this fix.

Tom