[PATCH 0/2] more progress on objfile splitting

[PATCH 0/2] more progress on objfile splitting

[Tom Tromey]

This series makes a bit more headway on the objfile splitting project.

    http://sourceware.org/gdb/wiki/ObjfileSplitting

This introduces a generic way to solve the problem of data being
unshareable due to relocations, and it makes the demangled name hash
shareable.

This series conflicts with my patch to move gdbarch into the per-BFD,
but only in a trivial way.

Unfortunately for me, I'm running out of easy tasks to do for this
project.  Next up may be changing type allocation to prepare the way
for later changes.  Probably the next thing after that is switching
over minsyms, definitely non-trivial, but easier than partial or full
symbols due to the lack of backlinks.

Tom

[PATCH 2/2] move the demangled_names_hash into the per-BFD

[Tom Tromey]

This moves the demangled_names_hash from the objfile into the per-BFD
object.  This is part of the objfile splitting project.

The demangled names hash is independent of the program space.  And, it
is needed by the symbol tables.  Both of these things indicate that it
must be pushed into the per-BFD object, which this patch does.

Built and regtested on x86-64 Fedora 18.

	* objfiles.c (free_objfile_per_bfd_storage): Delete the
	demangled_names_hash.
	(free_objfile): Don't delete the demangled_names_hash.
	* objfiles.h (struct objfile_per_bfd_storage)
	<demangled_names_hash>: New field.
	(struct objfile) <demangled_names_hash>: Move to
	objfile_per_bfd_storage.
	* symfile.c (reread_symbols): Don't delete the
	demangled_names_hash.
	* symtab.c (create_demangled_names_hash): Update.
	(symbol_set_names): Update.
---
 gdb/objfiles.c |  4 ++--
 gdb/objfiles.h | 14 +++++++-------
 gdb/symfile.c  |  5 -----
 gdb/symtab.c   | 21 +++++++++++----------
 4 files changed, 20 insertions(+), 24 deletions(-)

diff --git a/gdb/objfiles.c b/gdb/objfiles.c
index 8711a13..7868367 100644
--- a/gdb/objfiles.c
+++ b/gdb/objfiles.c
@@ -166,6 +166,8 @@ free_objfile_per_bfd_storage (struct objfile_per_bfd_storage *storage)
 {
   bcache_xfree (storage->filename_cache);
   bcache_xfree (storage->macro_cache);
+  if (storage->demangled_names_hash)
+    htab_delete (storage->demangled_names_hash);
   obstack_free (&storage->storage_obstack, 0);
 }
 
@@ -653,8 +655,6 @@ free_objfile (struct objfile *objfile)
     xfree (objfile->static_psymbols.list);
   /* Free the obstacks for non-reusable objfiles.  */
   psymbol_bcache_free (objfile->psymbol_cache);
-  if (objfile->demangled_names_hash)
-    htab_delete (objfile->demangled_names_hash);
   obstack_free (&objfile->objfile_obstack, 0);
 
   /* Rebuild section map next time we need it.  */
diff --git a/gdb/objfiles.h b/gdb/objfiles.h
index 84167e0..31cc0c0 100644
--- a/gdb/objfiles.h
+++ b/gdb/objfiles.h
@@ -178,6 +178,13 @@ struct objfile_per_bfd_storage
 
   /* Byte cache for macros.  */
   struct bcache *macro_cache;
+
+  /* Hash table for mapping symbol names to demangled names.  Each
+     entry in the hash table is actually two consecutive strings,
+     both null-terminated; the first one is a mangled or linkage
+     name, and the second is the demangled name or just a zero byte
+     if the name doesn't demangle.  */
+  struct htab *demangled_names_hash;
 };
 
 /* Master structure for keeping track of each file from which
@@ -270,13 +277,6 @@ struct objfile
 
     struct psymbol_bcache *psymbol_cache; /* Byte cache for partial syms.  */
 
-    /* Hash table for mapping symbol names to demangled names.  Each
-       entry in the hash table is actually two consecutive strings,
-       both null-terminated; the first one is a mangled or linkage
-       name, and the second is the demangled name or just a zero byte
-       if the name doesn't demangle.  */
-    struct htab *demangled_names_hash;
-
     /* Vectors of all partial symbols read in from file.  The actual data
        is stored in the objfile_obstack.  */
 
diff --git a/gdb/symfile.c b/gdb/symfile.c
index 3dd5509..54973ba 100644
--- a/gdb/symfile.c
+++ b/gdb/symfile.c
@@ -2479,11 +2479,6 @@ reread_symbols (void)
 	  /* Free the obstacks for non-reusable objfiles.  */
 	  psymbol_bcache_free (objfile->psymbol_cache);
 	  objfile->psymbol_cache = psymbol_bcache_init ();
-	  if (objfile->demangled_names_hash != NULL)
-	    {
-	      htab_delete (objfile->demangled_names_hash);
-	      objfile->demangled_names_hash = NULL;
-	    }
 	  obstack_free (&objfile->objfile_obstack, 0);
 	  objfile->sections = NULL;
 	  objfile->symtabs = NULL;
diff --git a/gdb/symtab.c b/gdb/symtab.c
index 3bcec23..cbac42b 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -561,7 +561,7 @@ create_demangled_names_hash (struct objfile *objfile)
      Choosing a much larger table size wastes memory, and saves only about
      1% in symbol reading.  */
 
-  objfile->demangled_names_hash = htab_create_alloc
+  objfile->per_bfd->demangled_names_hash = htab_create_alloc
     (256, hash_demangled_name_entry, eq_demangled_name_entry,
      NULL, xcalloc, xfree);
 }
@@ -656,7 +656,7 @@ symbol_find_demangled_name (struct general_symbol_info *gsymbol,
    objfile), and it will not be copied.
 
    The hash table corresponding to OBJFILE is used, and the memory
-   comes from that objfile's objfile_obstack.  LINKAGE_NAME is copied,
+   comes from the per-BFD storage_obstack.  LINKAGE_NAME is copied,
    so the pointer can be discarded after calling this function.  */
 
 /* We have to be careful when dealing with Java names: when we run
@@ -692,6 +692,7 @@ symbol_set_names (struct general_symbol_info *gsymbol,
   /* The length of lookup_name.  */
   int lookup_len;
   struct demangled_name_entry entry;
+  struct objfile_per_bfd_storage *per_bfd = objfile->per_bfd;
 
   if (gsymbol->language == language_ada)
     {
@@ -707,18 +708,18 @@ symbol_set_names (struct general_symbol_info *gsymbol,
 	gsymbol->name = linkage_name;
       else
 	{
-	  char *name = obstack_alloc (&objfile->objfile_obstack, len + 1);
+	  char *name = obstack_alloc (&per_bfd->storage_obstack, len + 1);
 
 	  memcpy (name, linkage_name, len);
 	  name[len] = '\0';
 	  gsymbol->name = name;
 	}
-      symbol_set_demangled_name (gsymbol, NULL, &objfile->objfile_obstack);
+      symbol_set_demangled_name (gsymbol, NULL, &per_bfd->storage_obstack);
 
       return;
     }
 
-  if (objfile->demangled_names_hash == NULL)
+  if (per_bfd->demangled_names_hash == NULL)
     create_demangled_names_hash (objfile);
 
   /* The stabs reader generally provides names that are not
@@ -758,7 +759,7 @@ symbol_set_names (struct general_symbol_info *gsymbol,
 
   entry.mangled = lookup_name;
   slot = ((struct demangled_name_entry **)
-	  htab_find_slot (objfile->demangled_names_hash,
+	  htab_find_slot (per_bfd->demangled_names_hash,
 			  &entry, INSERT));
 
   /* If this name is not in the hash table, add it.  */
@@ -783,7 +784,7 @@ symbol_set_names (struct general_symbol_info *gsymbol,
 	 us better bcache hit rates for partial symbols.  */
       if (!copy_name && lookup_name == linkage_name)
 	{
-	  *slot = obstack_alloc (&objfile->objfile_obstack,
+	  *slot = obstack_alloc (&per_bfd->storage_obstack,
 				 offsetof (struct demangled_name_entry,
 					   demangled)
 				 + demangled_len + 1);
@@ -796,7 +797,7 @@ symbol_set_names (struct general_symbol_info *gsymbol,
 	  /* If we must copy the mangled name, put it directly after
 	     the demangled name so we can have a single
 	     allocation.  */
-	  *slot = obstack_alloc (&objfile->objfile_obstack,
+	  *slot = obstack_alloc (&per_bfd->storage_obstack,
 				 offsetof (struct demangled_name_entry,
 					   demangled)
 				 + lookup_len + demangled_len + 2);
@@ -817,9 +818,9 @@ symbol_set_names (struct general_symbol_info *gsymbol,
   gsymbol->name = (*slot)->mangled + lookup_len - len;
   if ((*slot)->demangled[0] != '\0')
     symbol_set_demangled_name (gsymbol, (*slot)->demangled,
-			       &objfile->objfile_obstack);
+			       &per_bfd->storage_obstack);
   else
-    symbol_set_demangled_name (gsymbol, NULL, &objfile->objfile_obstack);
+    symbol_set_demangled_name (gsymbol, NULL, &per_bfd->storage_obstack);
 }
 
 /* Return the source code name of a symbol.  In languages where
-- 
1.8.1.4

Re: [PATCH 2/2] move the demangled_names_hash into the per-BFD

[Pedro Alves]

On 08/14/2013 09:36 PM, Tom Tromey wrote:

> 
> 	* objfiles.c (free_objfile_per_bfd_storage): Delete the
> 	demangled_names_hash.
> 	(free_objfile): Don't delete the demangled_names_hash.
> 	* objfiles.h (struct objfile_per_bfd_storage)
> 	<demangled_names_hash>: New field.
> 	(struct objfile) <demangled_names_hash>: Move to
> 	objfile_per_bfd_storage.
> 	* symfile.c (reread_symbols): Don't delete the
> 	demangled_names_hash.
> 	* symtab.c (create_demangled_names_hash): Update.
> 	(symbol_set_names): Update.

Looks fine to me.

Thanks,
-- 
Pedro Alves

Re: [PATCH 2/2] move the demangled_names_hash into the per-BFD

[Tom Tromey]

>> * objfiles.c (free_objfile_per_bfd_storage): Delete the
>> demangled_names_hash.
>> (free_objfile): Don't delete the demangled_names_hash.
>> * objfiles.h (struct objfile_per_bfd_storage)
>> <demangled_names_hash>: New field.
>> (struct objfile) <demangled_names_hash>: Move to
>> objfile_per_bfd_storage.
>> * symfile.c (reread_symbols): Don't delete the
>> demangled_names_hash.
>> * symtab.c (create_demangled_names_hash): Update.
>> (symbol_set_names): Update.

Pedro> Looks fine to me.

I'm checking this in now.

Tom

ASAN crash regression [Re: [PATCH 2/2] move the demangled_names_hash into the per-BFD]

[Jan Kratochvil]

On Mon, 07 Oct 2013 21:27:04 +0200, Tom Tromey wrote:
> >> * objfiles.c (free_objfile_per_bfd_storage): Delete the
> >> demangled_names_hash.
> >> (free_objfile): Don't delete the demangled_names_hash.
> >> * objfiles.h (struct objfile_per_bfd_storage)
> >> <demangled_names_hash>: New field.
> >> (struct objfile) <demangled_names_hash>: Move to
> >> objfile_per_bfd_storage.
> >> * symfile.c (reread_symbols): Don't delete the
> >> demangled_names_hash.
> >> * symtab.c (create_demangled_names_hash): Update.
> >> (symbol_set_names): Update.
> 
> Pedro> Looks fine to me.
> 
> I'm checking this in now.

84a1243b15122dfe6414a4f9bdd82096b37bc625 is the first bad commit
commit 84a1243b15122dfe6414a4f9bdd82096b37bc625
Author: Tom Tromey <tromey@redhat.com>
Date:   Mon Oct 7 19:40:38 2013 +0000

    move the demangled_names_hash into the per-BFD

./configure ... -fsanitize=address
echo 'void f(){}main(){}'|gcc -x c++ - -g;ASAN_OPTIONS=symbolize=1 ASAN_SYMBOLIZER_PATH=/usr/bin/llvm-symbolizer ./gdb -batch a.out -ex 'file a.out'

==2077== ERROR: AddressSanitizer: heap-use-after-free on address 0x606200145eb0 at pc 0x7f4e28c4e947 bp 0x7fffb2e2c920 sp 0x7fffb2e2c8e0
READ of size 4 at 0x606200145eb0 thread T0
    #0 0x7f4e28c4e946 in __interceptor_strcmp /usr/src/debug/gcc-4.8.2-20131212/obj-x86_64-redhat-linux/x86_64-redhat-linux/libsanitizer/asan/../../../../libsanitizer/asan/asan_interceptors.cc:399
    #1 0x9c274e in eq_demangled_name_entry /home/jkratoch/redhat/gdb-clean-f20/gdb/symtab.c:612
    #2 0x1f6e4dc in htab_find_slot_with_hash /home/jkratoch/redhat/gdb-clean-f20/libiberty/./hashtab.c:660
    #3 0x1f6e81d in htab_find_slot /home/jkratoch/redhat/gdb-clean-f20/libiberty/./hashtab.c:704
    #4 0x9c3992 in symbol_set_names /home/jkratoch/redhat/gdb-clean-f20/gdb/symtab.c:860
    #5 0xb82fc9 in new_symbol_full /home/jkratoch/redhat/gdb-clean-f20/gdb/dwarf2read.c:17696
    #6 0xb5a90c in read_func_scope /home/jkratoch/redhat/gdb-clean-f20/gdb/dwarf2read.c:11193
    #7 0xb49918 in process_die /home/jkratoch/redhat/gdb-clean-f20/gdb/dwarf2read.c:8165
    #8 0xb4d245 in read_file_scope /home/jkratoch/redhat/gdb-clean-f20/gdb/dwarf2read.c:9042
    #9 0xb498e8 in process_die /home/jkratoch/redhat/gdb-clean-f20/gdb/dwarf2read.c:8158
    #10 0xb48577 in process_full_comp_unit /home/jkratoch/redhat/gdb-clean-f20/gdb/dwarf2read.c:7941
    #11 0xb459aa in process_queue /home/jkratoch/redhat/gdb-clean-f20/gdb/dwarf2read.c:7481
    #12 0xb2a790 in dw2_do_instantiate_symtab /home/jkratoch/redhat/gdb-clean-f20/gdb/dwarf2read.c:2641
    #13 0xb46055 in psymtab_to_symtab_1 /home/jkratoch/redhat/gdb-clean-f20/gdb/dwarf2read.c:7570
    #14 0xb451ad in dwarf2_read_symtab /home/jkratoch/redhat/gdb-clean-f20/gdb/dwarf2read.c:7350
    #15 0x9dc416 in psymtab_to_symtab /home/jkratoch/redhat/gdb-clean-f20/gdb/psymtab.c:779
    #16 0x9dafea in lookup_symbol_aux_psymtabs /home/jkratoch/redhat/gdb-clean-f20/gdb/psymtab.c:513
    #17 0x9c759e in lookup_symbol_aux_quick /home/jkratoch/redhat/gdb-clean-f20/gdb/symtab.c:1767
    #18 0x9c78d0 in lookup_symbol_global_iterator_cb /home/jkratoch/redhat/gdb-clean-f20/gdb/symtab.c:1873
    #19 0xaf308f in default_iterate_over_objfiles_in_search_order /home/jkratoch/redhat/gdb-clean-f20/gdb/objfiles.c:1491
    #20 0xa8834f in gdbarch_iterate_over_objfiles_in_search_order /home/jkratoch/redhat/gdb-clean-f20/gdb/gdbarch.c:4307
    #21 0x9c7b12 in lookup_symbol_global /home/jkratoch/redhat/gdb-clean-f20/gdb/symtab.c:1904
    #22 0xd00050 in lookup_symbol_file /home/jkratoch/redhat/gdb-clean-f20/gdb/cp-namespace.c:636
    #23 0xcfeca0 in cp_lookup_symbol_in_namespace /home/jkratoch/redhat/gdb-clean-f20/gdb/cp-namespace.c:255
    #24 0xcfffad in lookup_namespace_scope /home/jkratoch/redhat/gdb-clean-f20/gdb/cp-namespace.c:601
    #25 0xcfebfb in cp_lookup_symbol_nonlocal /home/jkratoch/redhat/gdb-clean-f20/gdb/cp-namespace.c:234
    #26 0x9c6a69 in lookup_symbol_aux /home/jkratoch/redhat/gdb-clean-f20/gdb/symtab.c:1488
    #27 0x9c5ccc in lookup_symbol_in_language /home/jkratoch/redhat/gdb-clean-f20/gdb/symtab.c:1334
    #28 0x9c5d82 in lookup_symbol /home/jkratoch/redhat/gdb-clean-f20/gdb/symtab.c:1349
    #29 0x9ea09a in set_initial_language /home/jkratoch/redhat/gdb-clean-f20/gdb/symfile.c:1702
    #30 0x9e86d3 in symbol_file_add_main_1 /home/jkratoch/redhat/gdb-clean-f20/gdb/symfile.c:1329
    #31 0x9e9fc3 in symbol_file_command /home/jkratoch/redhat/gdb-clean-f20/gdb/symfile.c:1669
    #32 0xae4323 in file_command /home/jkratoch/redhat/gdb-clean-f20/gdb/exec.c:329
    #33 0x7f131e in do_cfunc /home/jkratoch/redhat/gdb-clean-f20/gdb/./cli/cli-decode.c:107
    #34 0x7f8d3e in cmd_func /home/jkratoch/redhat/gdb-clean-f20/gdb/./cli/cli-decode.c:1886
    #35 0xcc7cc6 in execute_command /home/jkratoch/redhat/gdb-clean-f20/gdb/top.c:461
    #36 0xa4e258 in catch_command_errors /home/jkratoch/redhat/gdb-clean-f20/gdb/exceptions.c:551
    #37 0xa5a2f3 in captured_main /home/jkratoch/redhat/gdb-clean-f20/gdb/main.c:1073
    #38 0xa4e03d in catch_errors /home/jkratoch/redhat/gdb-clean-f20/gdb/exceptions.c:524
    #39 0xa5a3a6 in gdb_main /home/jkratoch/redhat/gdb-clean-f20/gdb/main.c:1105
    #40 0x49307e in main /home/jkratoch/redhat/gdb-clean-f20/gdb/gdb.c:33
    #41 0x3721e21d64 in __libc_start_main (/lib64/libc.so.6+0x3721e21d64)
    #42 0x492e48 in _start (/home/jkratoch/redhat/gdb-clean-f20/gdb/gdb+0x492e48)
0x606200145eb0 is located 2992 bytes inside of 4064-byte region [0x606200145300,0x6062001462e0)
freed by thread T0 here:
    #0 0x7f4e28c550f9 in __interceptor_free /usr/src/debug/gcc-4.8.2-20131212/obj-x86_64-redhat-linux/x86_64-redhat-linux/libsanitizer/asan/../../../../libsanitizer/asan/asan_malloc_linux.cc:61
    #1 0xd53840 in xfree /home/jkratoch/redhat/gdb-clean-f20/gdb/./common/common-utils.c:108
    #2 0x3721e84857 in obstack_free (/lib64/libc.so.6+0x3721e84857)
previously allocated by thread T0 here:
    #0 0x7f4e28c55219 in __interceptor_malloc /usr/src/debug/gcc-4.8.2-20131212/obj-x86_64-redhat-linux/x86_64-redhat-linux/libsanitizer/asan/../../../../libsanitizer/asan/asan_malloc_linux.cc:71
    #1 0xd536f7 in xmalloc /home/jkratoch/redhat/gdb-clean-f20/gdb/./common/common-utils.c:51
    #2 0x3721e8477d in __GI__obstack_newchunk (/lib64/libc.so.6+0x3721e8477d)
SUMMARY: AddressSanitizer: heap-use-after-free /usr/src/debug/gcc-4.8.2-20131212/obj-x86_64-redhat-linux/x86_64-redhat-linux/libsanitizer/asan/../../../../libsanitizer/asan/asan_interceptors.cc:399 __interceptor_strcmp
Shadow bytes around the buggy address:
  0x0c0cc0020b80: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c0cc0020b90: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c0cc0020ba0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c0cc0020bb0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c0cc0020bc0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
=>0x0c0cc0020bd0: fd fd fd fd fd fd[fd]fd fd fd fd fd fd fd fd fd
  0x0c0cc0020be0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c0cc0020bf0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c0cc0020c00: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c0cc0020c10: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c0cc0020c20: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:     fa
  Heap righ redzone:     fb
  Freed Heap region:     fd
  Stack left redzone:    f1
  Stack mid redzone:     f2
  Stack right redzone:   f3
  Stack partial redzone: f4
  Stack after return:    f5
  Stack use after scope: f8
  Global redzone:        f9
  Global init order:     f6
  Poisoned by user:      f7
  ASan internal:         fe
==2077== ABORTING

Re: ASAN crash regression [Re: [PATCH 2/2] move the demangled_names_hash into the per-BFD]

[Tom Tromey]

Jan> ./configure ... -fsanitize=address
Jan> echo 'void f(){}main(){}'|gcc -x c++ - -g;ASAN_OPTIONS=symbolize=1 ASAN_SYMBOLIZER_PATH=/usr/bin/llvm-symbolizer ./gdb -batch a.out -ex 'file a.out'

Readily seen with valgrind as well.

Here's my proposed fix.

Tom

commit 3a93a67ad0ea3495f67c9708673345b73de2d806
Author: Tom Tromey <tromey@redhat.com>
Date:   Mon Jun 16 03:17:19 2014 -0600

    fix memory errors with demangled name hash
    
    This fixes a regression that Jan pointed out.
    
    The bug is that some names were allocated by dwarf2read on the objfile
    obstack, but then passed to SYMBOL_SET_NAMES with copy_name=0.  This
    violates the invariant that the names must have a lifetime tied to the
    lifetime of the BFD.
    
    The fix is to allocate names on the per-BFD obstack.
    
    I looked at all callers, direct or indirect, of SYMBOL_SET_NAMES that
    pass copy_name=0.  Note that only the ELF and DWARF readers do this;
    other symbol readers were never updated (and perhaps cannot be,
    depending on the details of the formats).  This is why the patch is
    relatively small.
    
    Built and regtested on x86-64 Fedora 20.
    
    2014-06-16  Tom Tromey  <tromey@redhat.com>
    
    	* dwarf2read.c (fixup_go_packaging, dwarf2_compute_name)
    	(dwarf2_physname, read_partial_die)
    	(guess_partial_die_structure_name, fixup_partial_die)
    	(guess_full_die_structure_name, anonymous_struct_prefix)
    	(dwarf2_name): Use per-BFD obstack.

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index a999390..c773e7b 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,13 @@
 2014-06-16  Tom Tromey  <tromey@redhat.com>
 
+	* dwarf2read.c (fixup_go_packaging, dwarf2_compute_name)
+	(dwarf2_physname, read_partial_die)
+	(guess_partial_die_structure_name, fixup_partial_die)
+	(guess_full_die_structure_name, anonymous_struct_prefix)
+	(dwarf2_name): Use per-BFD obstack.
+
+2014-06-16  Tom Tromey  <tromey@redhat.com>
+
 	* minsyms.h (prim_record_minimal_symbol)
 	(prim_record_minimal_symbol_and_info): Update comments.
 
diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index 3e441dc..bd6e547 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -7745,9 +7745,10 @@ fixup_go_packaging (struct dwarf2_cu *cu)
   if (package_name != NULL)
     {
       struct objfile *objfile = cu->objfile;
-      const char *saved_package_name = obstack_copy0 (&objfile->objfile_obstack,
-						      package_name,
-						      strlen (package_name));
+      const char *saved_package_name
+	= obstack_copy0 (&objfile->per_bfd->storage_obstack,
+			 package_name,
+			 strlen (package_name));
       struct type *type = init_type (TYPE_CODE_MODULE, 0, 0,
 				     saved_package_name, objfile);
       struct symbol *sym;
@@ -8365,6 +8366,8 @@ dwarf2_compute_name (const char *name,
 	  long length;
 	  const char *prefix;
 	  struct ui_file *buf;
+	  char *intermediate_name;
+	  const char *canonical_name = NULL;
 
 	  prefix = determine_prefix (die, cu);
 	  buf = mem_fileopen ();
@@ -8541,19 +8544,25 @@ dwarf2_compute_name (const char *name,
 		}
 	    }
 
-	  name = ui_file_obsavestring (buf, &objfile->objfile_obstack,
-				       &length);
+	  intermediate_name = ui_file_xstrdup (buf, &length);
 	  ui_file_delete (buf);
 
 	  if (cu->language == language_cplus)
-	    {
-	      const char *cname
-		= dwarf2_canonicalize_name (name, cu,
-					    &objfile->objfile_obstack);
+	    canonical_name
+	      = dwarf2_canonicalize_name (intermediate_name, cu,
+					  &objfile->per_bfd->storage_obstack);
+
+	  /* If we only computed INTERMEDIATE_NAME, or if
+	     INTERMEDIATE_NAME is already canonical, then we need to
+	     copy it to the appropriate obstack.  */
+	  if (canonical_name == NULL || canonical_name == intermediate_name)
+	    name = obstack_copy0 (&objfile->per_bfd->storage_obstack,
+				  intermediate_name,
+				  strlen (intermediate_name));
+	  else
+	    name = canonical_name;
 
-	      if (cname != NULL)
-		name = cname;
-	    }
+	  xfree (intermediate_name);
 	}
     }
 
@@ -8562,7 +8571,7 @@ dwarf2_compute_name (const char *name,
 
 /* Return the fully qualified name of DIE, based on its DW_AT_name.
    If scope qualifiers are appropriate they will be added.  The result
-   will be allocated on the objfile_obstack, or NULL if the DIE does
+   will be allocated on the storage_obstack, or NULL if the DIE does
    not have a name.  NAME may either be from a previous call to
    dwarf2_name or NULL.
 
@@ -8677,7 +8686,8 @@ dwarf2_physname (const char *name, struct die_info *die, struct dwarf2_cu *cu)
     retval = canon;
 
   if (need_copy)
-    retval = obstack_copy0 (&objfile->objfile_obstack, retval, strlen (retval));
+    retval = obstack_copy0 (&objfile->per_bfd->storage_obstack,
+			    retval, strlen (retval));
 
   do_cleanups (back_to);
   return retval;
@@ -15508,7 +15518,7 @@ read_partial_die (const struct die_reader_specs *reader,
 	    default:
 	      part_die->name
 		= dwarf2_canonicalize_name (DW_STRING (&attr), cu,
-					    &objfile->objfile_obstack);
+					    &objfile->per_bfd->storage_obstack);
 	      break;
 	    }
 	  break;
@@ -15793,7 +15803,7 @@ guess_partial_die_structure_name (struct partial_die_info *struct_pdi,
 	  if (actual_class_name != NULL)
 	    {
 	      struct_pdi->name
-		= obstack_copy0 (&cu->objfile->objfile_obstack,
+		= obstack_copy0 (&cu->objfile->per_bfd->storage_obstack,
 				 actual_class_name,
 				 strlen (actual_class_name));
 	      xfree (actual_class_name);
@@ -15879,8 +15889,9 @@ fixup_partial_die (struct partial_die_info *part_die,
 	  else
 	    base = demangled;
 
-	  part_die->name = obstack_copy0 (&cu->objfile->objfile_obstack,
-					  base, strlen (base));
+	  part_die->name
+	    = obstack_copy0 (&cu->objfile->per_bfd->storage_obstack,
+			     base, strlen (base));
 	  xfree (demangled);
 	}
     }
@@ -18557,7 +18568,7 @@ guess_full_die_structure_name (struct die_info *die, struct dwarf2_cu *cu)
 			  && actual_name[actual_name_len
 					 - die_name_len - 1] == ':')
 			name =
-			  obstack_copy0 (&cu->objfile->objfile_obstack,
+			  obstack_copy0 (&cu->objfile->per_bfd->storage_obstack,
 					 actual_name,
 					 actual_name_len - die_name_len - 2);
 		    }
@@ -18603,7 +18614,7 @@ anonymous_struct_prefix (struct die_info *die, struct dwarf2_cu *cu)
   if (base == NULL || base == DW_STRING (attr) || base[-1] != ':')
     return "";
 
-  return obstack_copy0 (&cu->objfile->objfile_obstack,
+  return obstack_copy0 (&cu->objfile->per_bfd->storage_obstack,
 			DW_STRING (attr), &base[-1] - DW_STRING (attr));
 }
 
@@ -18943,8 +18954,9 @@ dwarf2_name (struct die_info *die, struct dwarf2_cu *cu)
 	      char *base;
 
 	      /* FIXME: we already did this for the partial symbol... */
-	      DW_STRING (attr) = obstack_copy0 (&cu->objfile->objfile_obstack,
-						demangled, strlen (demangled));
+	      DW_STRING (attr)
+		= obstack_copy0 (&cu->objfile->per_bfd->storage_obstack,
+				 demangled, strlen (demangled));
 	      DW_STRING_IS_CANONICAL (attr) = 1;
 	      xfree (demangled);
 
@@ -18967,7 +18979,7 @@ dwarf2_name (struct die_info *die, struct dwarf2_cu *cu)
     {
       DW_STRING (attr)
 	= dwarf2_canonicalize_name (DW_STRING (attr), cu,
-				    &cu->objfile->objfile_obstack);
+				    &cu->objfile->per_bfd->storage_obstack);
       DW_STRING_IS_CANONICAL (attr) = 1;
     }
   return DW_STRING (attr);

Re: ASAN crash regression [Re: [PATCH 2/2] move the demangled_names_hash into the per-BFD]

[Tom Tromey]

>>>>> "Tom" == Tom Tromey <tromey@redhat.com> writes:

Jan> ./configure ... -fsanitize=address
Jan> echo 'void f(){}main(){}'|gcc -x c++ -
Jan> -g;ASAN_OPTIONS=symbolize=1
Jan> ASAN_SYMBOLIZER_PATH=/usr/bin/llvm-symbolizer ./gdb -batch a.out
Jan> -ex 'file a.out'

Tom> Readily seen with valgrind as well.
Tom> Here's my proposed fix.

I'm checking this in on the trunk and the 7.8 branch.

It turns out that this was PR 16902, so I've updated the ChangeLog.

Tom

[PATCH 1/2] don't share per-BFD data if relocations are needed

[Tom Tromey]

Right now we always share per-BFD data across objfiles, if there is a
BFD.  This works fine.  However, we're going to start sharing more
data, and sometimes this data will come directly from sections of the
BFD.  If such a section has SEC_RELOC set, then the data coming from
that section will not be truly sharable -- the section will be
program-space-dependent, and re-read by gdb for each objfile.

This patch disallows per-BFD sharing in this case.  This is a bit
"heavy" in that we could in theory examine each bit of shared data for
suitability.  However, that is more complicated, and SEC_RELOC is rare
enough that I think we needn't bother.

Note that the "no sharing" case is equivalent to "gdb works as it
historically did".  That is, the sharing is a new(-ish) optimization.

Built and regtested on x86-64 Fedora 18.

	* gdb_bfd.c (struct gdb_bfd_data) <relocation_computed,
	needs_relocations>: New fields.
	(gdb_bfd_requires_relocations): New function.
	* gdb_bfd.h (gdb_bfd_requires_relocations): Declare.
	* objfiles.c (get_objfile_bfd_data): Disallow sharing if
	the BFD needs relocations applied.
---
 gdb/gdb_bfd.c  | 31 +++++++++++++++++++++++++++++++
 gdb/gdb_bfd.h  |  5 +++++
 gdb/objfiles.c |  5 ++++-
 3 files changed, 40 insertions(+), 1 deletion(-)

diff --git a/gdb/gdb_bfd.c b/gdb/gdb_bfd.c
index 7ba120e..25cee5c 100644
--- a/gdb/gdb_bfd.c
+++ b/gdb/gdb_bfd.c
@@ -82,6 +82,13 @@ struct gdb_bfd_data
   /* The mtime of the BFD at the point the cache entry was made.  */
   time_t mtime;
 
+  /* This is true if we have determined whether this BFD has any
+     sections requiring relocation.  */
+  unsigned int relocation_computed : 1;
+
+  /* This is true if any section needs relocation.  */
+  unsigned int needs_relocations : 1;
+
   /* This is true if we have successfully computed the file's CRC.  */
   unsigned int crc_computed : 1;
 
@@ -634,6 +641,30 @@ gdb_bfd_count_sections (bfd *abfd)
   return bfd_count_sections (abfd) + 4;
 }
 
+/* See gdb_bfd.h.  */
+
+int
+gdb_bfd_requires_relocations (bfd *abfd)
+{
+  struct gdb_bfd_data *gdata = bfd_usrdata (abfd);
+
+  if (gdata->relocation_computed == 0)
+    {
+      asection *sect;
+
+      for (sect = abfd->sections; sect != NULL; sect = sect->next)
+	if ((sect->flags & SEC_RELOC) != 0)
+	  {
+	    gdata->needs_relocations = 1;
+	    break;
+	  }
+
+      gdata->relocation_computed = 1;
+    }
+
+  return gdata->needs_relocations;
+}
+
 \f
 
 /* A callback for htab_traverse that prints a single BFD.  */
diff --git a/gdb/gdb_bfd.h b/gdb/gdb_bfd.h
index ca2eddc..d28b29e 100644
--- a/gdb/gdb_bfd.h
+++ b/gdb/gdb_bfd.h
@@ -135,4 +135,9 @@ int gdb_bfd_section_index (bfd *abfd, asection *section);
 
 int gdb_bfd_count_sections (bfd *abfd);
 
+/* Return true if any section requires relocations, false
+   otherwise.  */
+
+int gdb_bfd_requires_relocations (bfd *abfd);
+
 #endif /* GDB_BFD_H */
diff --git a/gdb/objfiles.c b/gdb/objfiles.c
index 759159c..8711a13 100644
--- a/gdb/objfiles.c
+++ b/gdb/objfiles.c
@@ -139,7 +139,10 @@ get_objfile_bfd_data (struct objfile *objfile, struct bfd *abfd)
 
   if (storage == NULL)
     {
-      if (abfd != NULL)
+      /* If the object requires gdb to do relocations, we simply fall
+	 back to not sharing data across users.  These cases are rare
+	 enough that this seems reasonable.  */
+      if (abfd != NULL && !gdb_bfd_requires_relocations (abfd))
 	{
 	  storage = bfd_zalloc (abfd, sizeof (struct objfile_per_bfd_storage));
 	  set_bfd_data (abfd, objfiles_bfd_data, storage);
-- 
1.8.1.4

Re: [PATCH 1/2] don't share per-BFD data if relocations are needed

[Pedro Alves]

On 08/14/2013 09:36 PM, Tom Tromey wrote:
> Right now we always share per-BFD data across objfiles, if there is a
> BFD.  This works fine.  However, we're going to start sharing more
> data, and sometimes this data will come directly from sections of the
> BFD.  If such a section has SEC_RELOC set, then the data coming from
> that section will not be truly sharable -- the section will be
> program-space-dependent, and re-read by gdb for each objfile.
> 
> This patch disallows per-BFD sharing in this case.  This is a bit
> "heavy" in that we could in theory examine each bit of shared data for
> suitability.  However, that is more complicated, and SEC_RELOC is rare
> enough that I think we needn't bother.

Yeah, some targets use relocatable objects as main binary format,
but I agree we don't need to bother.

> @@ -139,7 +139,10 @@ get_objfile_bfd_data (struct objfile *objfile, struct bfd *abfd)
>  
>    if (storage == NULL)
>      {
> -      if (abfd != NULL)
> +      /* If the object requires gdb to do relocations, we simply fall
> +	 back to not sharing data across users.  These cases are rare
> +	 enough that this seems reasonable.  */
> +      if (abfd != NULL && !gdb_bfd_requires_relocations (abfd))
>  	{
>  	  storage = bfd_zalloc (abfd, sizeof (struct objfile_per_bfd_storage));
>  	  set_bfd_data (abfd, objfiles_bfd_data, storage);
> 

Shouldn't we still set storage->gdbarch to the bfd's gdbarch?

Otherwise looks fine to me.

-- 
Pedro Alves

Re: [PATCH 1/2] don't share per-BFD data if relocations are needed

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

>> @@ -139,7 +139,10 @@ get_objfile_bfd_data (struct objfile *objfile, struct bfd *abfd)
>> 
>> if (storage == NULL)
>> {
>> -      if (abfd != NULL)
>> +      /* If the object requires gdb to do relocations, we simply fall
>> +	 back to not sharing data across users.  These cases are rare
>> +	 enough that this seems reasonable.  */
>> +      if (abfd != NULL && !gdb_bfd_requires_relocations (abfd))
>> {
>> storage = bfd_zalloc (abfd, sizeof (struct objfile_per_bfd_storage));
>> set_bfd_data (abfd, objfiles_bfd_data, storage);
>> 

Pedro> Shouldn't we still set storage->gdbarch to the bfd's gdbarch?

Pedro> Otherwise looks fine to me.

Yes, good catch.

I'm checking in the appended updated patch instead.

Tom

commit 1e907c9922ff15d72d301cc3aa3586f85dbfe6b0
Author: Tom Tromey <tromey@redhat.com>
Date:   Wed Aug 14 14:06:53 2013 -0600

    don't share per-BFD data if relocations are needed
    
    Right now we always share per-BFD data across objfiles, if there is a
    BFD.  This works fine.  However, we're going to start sharing more
    data, and sometimes this data will come directly from sections of the
    BFD.  If such a section has SEC_RELOC set, then the data coming from
    that section will not be truly sharable -- the section will be
    program-space-dependent, and re-read by gdb for each objfile.
    
    This patch disallows per-BFD sharing in this case.  This is a bit
    "heavy" in that we could in theory examine each bit of shared data for
    suitability.  However, that is more complicated, and SEC_RELOC is rare
    enough that I think we needn't bother.
    
    Note that the "no sharing" case is equivalent to "gdb works as it
    historically did".  That is, the sharing is a new(-ish) optimization.
    
    Built and regtested on x86-64 Fedora 18.
    
    	* gdb_bfd.c (struct gdb_bfd_data) <relocation_computed,
    	needs_relocations>: New fields.
    	(gdb_bfd_requires_relocations): New function.
    	* gdb_bfd.h (gdb_bfd_requires_relocations): Declare.
    	* objfiles.c (get_objfile_bfd_data): Disallow sharing if
    	the BFD needs relocations applied.

diff --git a/gdb/gdb_bfd.c b/gdb/gdb_bfd.c
index 7ba120e..25cee5c 100644
--- a/gdb/gdb_bfd.c
+++ b/gdb/gdb_bfd.c
@@ -82,6 +82,13 @@ struct gdb_bfd_data
   /* The mtime of the BFD at the point the cache entry was made.  */
   time_t mtime;
 
+  /* This is true if we have determined whether this BFD has any
+     sections requiring relocation.  */
+  unsigned int relocation_computed : 1;
+
+  /* This is true if any section needs relocation.  */
+  unsigned int needs_relocations : 1;
+
   /* This is true if we have successfully computed the file's CRC.  */
   unsigned int crc_computed : 1;
 
@@ -634,6 +641,30 @@ gdb_bfd_count_sections (bfd *abfd)
   return bfd_count_sections (abfd) + 4;
 }
 
+/* See gdb_bfd.h.  */
+
+int
+gdb_bfd_requires_relocations (bfd *abfd)
+{
+  struct gdb_bfd_data *gdata = bfd_usrdata (abfd);
+
+  if (gdata->relocation_computed == 0)
+    {
+      asection *sect;
+
+      for (sect = abfd->sections; sect != NULL; sect = sect->next)
+	if ((sect->flags & SEC_RELOC) != 0)
+	  {
+	    gdata->needs_relocations = 1;
+	    break;
+	  }
+
+      gdata->relocation_computed = 1;
+    }
+
+  return gdata->needs_relocations;
+}
+
 \f
 
 /* A callback for htab_traverse that prints a single BFD.  */
diff --git a/gdb/gdb_bfd.h b/gdb/gdb_bfd.h
index ca2eddc..d28b29e 100644
--- a/gdb/gdb_bfd.h
+++ b/gdb/gdb_bfd.h
@@ -135,4 +135,9 @@ int gdb_bfd_section_index (bfd *abfd, asection *section);
 
 int gdb_bfd_count_sections (bfd *abfd);
 
+/* Return true if any section requires relocations, false
+   otherwise.  */
+
+int gdb_bfd_requires_relocations (bfd *abfd);
+
 #endif /* GDB_BFD_H */
diff --git a/gdb/objfiles.c b/gdb/objfiles.c
index b10f803..b9bcfd7 100644
--- a/gdb/objfiles.c
+++ b/gdb/objfiles.c
@@ -137,18 +137,22 @@ get_objfile_bfd_data (struct objfile *objfile, struct bfd *abfd)
 
   if (storage == NULL)
     {
-      if (abfd != NULL)
+      /* If the object requires gdb to do relocations, we simply fall
+	 back to not sharing data across users.  These cases are rare
+	 enough that this seems reasonable.  */
+      if (abfd != NULL && !gdb_bfd_requires_relocations (abfd))
 	{
 	  storage = bfd_zalloc (abfd, sizeof (struct objfile_per_bfd_storage));
 	  set_bfd_data (abfd, objfiles_bfd_data, storage);
-
-	  /* Look up the gdbarch associated with the BFD.  */
-	  storage->gdbarch = gdbarch_from_bfd (abfd);
 	}
       else
 	storage = OBSTACK_ZALLOC (&objfile->objfile_obstack,
 				  struct objfile_per_bfd_storage);
 
+      /* Look up the gdbarch associated with the BFD.  */
+      if (abfd != NULL)
+	storage->gdbarch = gdbarch_from_bfd (abfd);
+
       obstack_init (&storage->storage_obstack);
       storage->filename_cache = bcache_xmalloc (NULL, NULL);
       storage->macro_cache = bcache_xmalloc (NULL, NULL);