[PATCH] gdb: Reinitialize objfile::section_offsets during objfile reload

[PATCH] gdb: Reinitialize objfile::section_offsets during objfile reload

[Andrew Burgess]

When building and testing with '-D_GLIBCXX_DEBUG=1' I noticed that the
test gdb.base/reload.exp was failing.  This turns out to be because
the objfile::section_offsets vector is not reinitialilzed during the
objfile reload process, and in this particular test, GDB ends up
indexing outside the bounds of the vector.

In order to make this issue show up even if GDB is not compiled with
'-D_GLIBCXX_DEBUG=1' I added an assert into elfread.c, then I made
some changes in symfile.c:reread_symbols to reinitialilze the vector,
after which the problem is resolved.

While looking at the code in reread_symbols I noticed a difference
between reread_symbols and syms_from_objfile_1 with how we handle the
case where objfile->sf is NULL.  In the later function we handle this
case, while in the former we just assume that objfile->sf is not
NULL.  I can't see why the NULL case couldn't occur in reread_symbols,
however, this would only happen if the user is using srec, ihex, or
texhex format BFDs.  As such I didn't fancy trying to update
reread_symbols to handle this case, so instead I just added an error
if this case should arise.  This should be better than the current
undefined behaviour (crash), and should let us know what has gone
wrong if a user ever reports this as an issue.

One thing I did wonder about while looking at this fix is whether it
would be possible to combine at least parts of syms_from_objfile_1
with the core of reread_symbols.  I did have a go at doing this but
gave up in the end due to the subtle differences between the two.
Still, I think that with some more effort this might be possible, and
this could be a nice clean up.

gdb/ChangeLog:

	* elfread.c (record_minimal_symbol): Add an assertion.
	* symfile.c (reread_symbols): Clear the section_offsets vector,
	and reinitialize it during reload.  Add an error if objfile->sf is
	NULL.

Change-Id: I9d62292641416483a8a7415c7504095bf439fc29
---
 gdb/ChangeLog |  7 +++++++
 gdb/elfread.c |  4 ++++
 gdb/symfile.c | 19 +++++++++++++++++++
 3 files changed, 30 insertions(+)

diff --git a/gdb/elfread.c b/gdb/elfread.c
index 453bca527e9..5d0acede078 100644
--- a/gdb/elfread.c
+++ b/gdb/elfread.c
@@ -218,6 +218,10 @@ record_minimal_symbol (minimal_symbol_reader &reader,
   if ((bfd_section_flags (bfd_section) & SEC_ALLOC) == SEC_ALLOC)
     section_index = gdb_bfd_section_index (objfile->obfd, bfd_section);
 
+  /* The section_offsets vector should have been initialised by now, and
+     there should be one entry for each section in objfile.  */
+  gdb_assert (section_index < objfile->section_offsets.size ());
+
   struct minimal_symbol *result
     = reader.record_full (name, copy_name, address, ms_type, section_index);
   if ((objfile->flags & OBJF_MAINLINE) == 0
diff --git a/gdb/symfile.c b/gdb/symfile.c
index f7bada75f35..0da879fd751 100644
--- a/gdb/symfile.c
+++ b/gdb/symfile.c
@@ -2553,6 +2553,7 @@ reread_symbols (void)
 	  objfile->compunit_symtabs = NULL;
 	  objfile->template_symbols = NULL;
 	  objfile->static_links.reset (nullptr);
+	  objfile->section_offsets.clear ();
 
 	  /* obstack_init also initializes the obstack so it is
 	     empty.  We could use obstack_specify_allocation but
@@ -2573,6 +2574,16 @@ reread_symbols (void)
 	     start over.  PR symtab/15885  */
 	  objfile_set_sym_fns (objfile, find_sym_fns (objfile->obfd));
 
+	  /* In syms_from_objfile_1 after calling objfile_set_sym_fns we
+	     handle the possibility that objfile->sf might be NULL, which
+	     can happen for some obscure objfile formats.  We've never
+	     handled the NULL case here before, but */
+	  if (objfile->sf == nullptr)
+	    error (_("unable to reload object file with format `%s'"),
+		   bfd_get_target (objfile->obfd));
+
+	  gdb_assert (objfile->sf != nullptr);
+
 	  build_objfile_section_table (objfile);
 
 	  /* What the hell is sym_new_init for, anyway?  The concept of
@@ -2604,6 +2615,14 @@ reread_symbols (void)
 
 	  objfiles_changed ();
 
+	  /* Setup the section offsets structure for this objfile.  We use
+	     zero section address information here, though it's not clear
+	     this will always be correct.  If the user originally loaded
+	     this objfile with non-zero address information then we're
+	     going to loose that here.  */
+	  section_addr_info local_addr;
+	  (*objfile->sf->sym_offsets) (objfile, local_addr);
+
 	  read_symbols (objfile, 0);
 
 	  if (!objfile_has_symbols (objfile))
-- 
2.14.5

Re: [PATCH] gdb: Reinitialize objfile::section_offsets during objfile reload

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:

Andrew> When building and testing with '-D_GLIBCXX_DEBUG=1' I noticed that the
Andrew> test gdb.base/reload.exp was failing.  This turns out to be because
Andrew> the objfile::section_offsets vector is not reinitialilzed during the
Andrew> objfile reload process, and in this particular test, GDB ends up
Andrew> indexing outside the bounds of the vector.

Thanks for catching this.

Andrew> One thing I did wonder about while looking at this fix is whether it
Andrew> would be possible to combine at least parts of syms_from_objfile_1
Andrew> with the core of reread_symbols.  I did have a go at doing this but
Andrew> gave up in the end due to the subtle differences between the two.
Andrew> Still, I think that with some more effort this might be possible, and
Andrew> this could be a nice clean up.

A long time ago, Jan had a patch along these lines.
I believe what his did was just throw away the logic in reread_symbols
in favor of simply creating a new objfile.  I wonder if it's too late to
do this now, since objfiles are exposed to Python.

Anyway, IMO, if there are subtle differences, they are probably bugs of
some sort; and unifying these code paths seems like clearly the right
thing to do.

Andrew> +	  /* In syms_from_objfile_1 after calling objfile_set_sym_fns we
Andrew> +	     handle the possibility that objfile->sf might be NULL, which
Andrew> +	     can happen for some obscure objfile formats.  We've never
Andrew> +	     handled the NULL case here before, but */

This looks like it got cut off.

Andrew> +	  /* Setup the section offsets structure for this objfile.  We use
Andrew> +	     zero section address information here, though it's not clear
Andrew> +	     this will always be correct.  If the user originally loaded
Andrew> +	     this objfile with non-zero address information then we're
Andrew> +	     going to loose that here.  */

s/loose/lose/

I don't know what else would make sense in this case.
Warn the user?

The patch seems otherwise reasonable to me.

Tom

Re: [PATCH] gdb: Reinitialize objfile::section_offsets during objfile reload

[Tom Tromey]

Andrew> When building and testing with '-D_GLIBCXX_DEBUG=1' I noticed that the
Andrew> test gdb.base/reload.exp was failing.  This turns out to be because
Andrew> the objfile::section_offsets vector is not reinitialilzed during the
Andrew> objfile reload process, and in this particular test, GDB ends up
Andrew> indexing outside the bounds of the vector.

Tom> Thanks for catching this.

I wonder if this is a regression due to 

commit 6a053cb1ff643cec3349d7f2f47ae5573f82d613
Author: Tom Tromey <tromey@adacore.com>
Date:   Mon Jan 6 14:34:52 2020 -0700

    Change section_offsets to a std::vector

See appended.

I think at the time I thought removing this code would simply preserve
the offsets.  But maybe we instead should std::move the offsets out of
the objfile and then move them back in?

This change would preserve the old status quo.

Tom

@@ -2479,9 +2468,6 @@ reread_symbols (void)
       new_modtime = new_statbuf.st_mtime;
       if (new_modtime != objfile->mtime)
 	{
-	  struct section_offsets *offsets;
-	  int num_offsets;
-
 	  printf_filtered (_("`%s' has changed; re-reading symbols.\n"),
 			   objfile_name (objfile));
 
@@ -2556,14 +2542,6 @@ reread_symbols (void)
 	    error (_("Can't read symbols from %s: %s."), objfile_name (objfile),
 		   bfd_errmsg (bfd_get_error ()));
 
-	  /* Save the offsets, we will nuke them with the rest of the
-	     objfile_obstack.  */
-	  num_offsets = objfile->num_sections;
-	  offsets = ((struct section_offsets *)
-		     alloca (SIZEOF_N_SECTION_OFFSETS (num_offsets)));
-	  memcpy (offsets, objfile->section_offsets,
-		  SIZEOF_N_SECTION_OFFSETS (num_offsets));
-
 	  objfile->reset_psymtabs ();
 
 	  /* NB: after this call to obstack_free, objfiles_changed
@@ -2595,15 +2573,6 @@ reread_symbols (void)
 
 	  build_objfile_section_table (objfile);
 
-	  /* We use the same section offsets as from last time.  I'm not
-	     sure whether that is always correct for shared libraries.  */
-	  objfile->section_offsets = (struct section_offsets *)
-	    obstack_alloc (&objfile->objfile_obstack,
-			   SIZEOF_N_SECTION_OFFSETS (num_offsets));
-	  memcpy (objfile->section_offsets, offsets,
-		  SIZEOF_N_SECTION_OFFSETS (num_offsets));
-	  objfile->num_sections = num_offsets;
-
 	  /* What the hell is sym_new_init for, anyway?  The concept of
 	     distinguishing between the main file and additional files
 	     in this way seems rather dubious.  */

Re: [PATCH] gdb: Reinitialize objfile::section_offsets during objfile reload

[Pedro Alves]

On 1/26/20 4:24 PM, Tom Tromey wrote:
> Andrew> When building and testing with '-D_GLIBCXX_DEBUG=1' I noticed that the
> Andrew> test gdb.base/reload.exp was failing.  This turns out to be because
> Andrew> the objfile::section_offsets vector is not reinitialilzed during the
> Andrew> objfile reload process, and in this particular test, GDB ends up
> Andrew> indexing outside the bounds of the vector.
> 
> Tom> Thanks for catching this.
> 
> I wonder if this is a regression due to 
> 
> commit 6a053cb1ff643cec3349d7f2f47ae5573f82d613
> Author: Tom Tromey <tromey@adacore.com>
> Date:   Mon Jan 6 14:34:52 2020 -0700
> 
>     Change section_offsets to a std::vector
> 
> See appended.
> 
> I think at the time I thought removing this code would simply preserve
> the offsets.  But maybe we instead should std::move the offsets out of
> the objfile and then move them back in?

+1.  Losing the user-specified offsets sounds nasty.

Thanks,
Pedro Alves

Re: [PATCH] gdb: Reinitialize objfile::section_offsets during objfile reload

[Pedro Alves]

On 1/26/20 4:15 PM, Tom Tromey wrote:

> Andrew> One thing I did wonder about while looking at this fix is whether it
> Andrew> would be possible to combine at least parts of syms_from_objfile_1
> Andrew> with the core of reread_symbols.  I did have a go at doing this but
> Andrew> gave up in the end due to the subtle differences between the two.
> Andrew> Still, I think that with some more effort this might be possible, and
> Andrew> this could be a nice clean up.
> 
> A long time ago, Jan had a patch along these lines.
> I believe what his did was just throw away the logic in reread_symbols
> in favor of simply creating a new objfile.  I wonder if it's too late to
> do this now, since objfiles are exposed to Python.

IIRC, that patch walked all the places that store objfile pointers,
to recreate the objfiles.  IMO, an approach that avoids that would be
better.

> 
> Anyway, IMO, if there are subtle differences, they are probably bugs of
> some sort; and unifying these code paths seems like clearly the right
> thing to do.

Agreed.

Thanks,
Pedro Alves