[PATCH] gdb: reset/recompute objfile section offsets in reread_symbols

[PATCH] gdb: reset/recompute objfile section offsets in reread_symbols

[Simon Marchi]

This patch started as an investigation of this bug, where the program is
re-compiled between two "start" runs:

    $ ./gdb -nx --data-directory=data-directory -q a.out
    Reading symbols from a.out...
    (gdb) start
    Temporary breakpoint 1 at 0x1131: file test.c, line 1.
    Starting program: /home/smarchi/build/wt/test/gdb/a.out

    Temporary breakpoint 1, main () at test.c:1
    1       int main() { return 0; }

    *** re-compile a.out ***

    (gdb) start
    The program being debugged has been started already.
    Start it from the beginning? (y or n) y
    `/home/smarchi/build/wt/test/gdb/a.out' has changed; re-reading symbols.
    Temporary breakpoint 2 at 0x555555555129: file test.c, line 1.
    Starting program: /home/smarchi/build/wt/test/gdb/a.out
    warning: Probes-based dynamic linker interface failed.
    Reverting to original interface.

    Temporary breakpoint 2, main () at test.c:1
    1       int main() { return 0; }
    (gdb)

To reproduce the bug, a.out needs to be a position-independent
executable (PIE).

Here's what happens:

1) We first read the symbols of a.out.  The section offsets in the
   objfile are all 0, so the symbols are created unrelocated.
2) The breakpoint on main is created, as you can see the breakpoint
   address (derived from the `main` symbol with value 0x1129) is still
   unrelocated (0x1131).  Since the program is not yet started, we don't
   know at which base address the executable is going to end at.
   Everything good so far.
3) The execution starts, GDB finds out the executable's base address,
   fills the objfile's section_offsets vector with a bunch of offsets,
   and relocates the symbols with those offsets.  The latter modifies
   the symbol values (the `main` symbol is changed from 0x1129 to
   0x555555555129).
4) We `start` again, we detect that `a.out` has changed, the
   `reread_symbols` function kicks in.  It tries to reset everything
   in the `struct objfile` corresponding to `a.out`, except that it
   leaves the `section_offsets` vector there.
5) `reread_symbols` reads the debug info (calls `read_symbols`).  As the
   DWARF info is read, symbols are created using the old offsets still
   in `section_offsets`.  For example, the `main` symbol is created with
   the value 0x555555555129.  Even though at this point there is no
   process, so that address is bogus.  There's probably more that
   depends on section_offsets that is not done correctly.
6) Something in the SVR4 solib handling goes wrong, probably because
   of something that went wrong in (5).  I can't quite explain it (if
   somebody would like to provide a more complete analysis, please go
   ahead).  But this is where it takes a wrong turn:

    #0  elf_locate_base () at /home/smarchi/src/wt/test/gdb/solib-svr4.c:799
    #1  0x000055f0a5bee6d5 in locate_base (info=<optimized out>) at /home/smarchi/src/wt/test/gdb/solib-svr4.c:848
    #2  0x000055f0a5bf1771 in svr4_handle_solib_event () at /home/smarchi/src/wt/test/gdb/solib-svr4.c:1955
    #3  0x000055f0a5c0ff92 in handle_solib_event () at /home/smarchi/src/wt/test/gdb/solib.c:1258

   In the non-working case (without this patch), elf_locate_base returns
   0, whereas in the working case (with this patch) it returns a valid
   address, as we should expect.

This patch fixes this by making reread_symbols clear the
`section_offsets` vector, and re-create it by calling `sym_offsets`.
This is analogous to what syms_from_objfile_1 does.  I didn't seem
absolutely necessary, but I also made it clear the various
`sect_index_*` fields, since their values no longer make sense (they
describe the old executable, and are indices in the now cleared
sections/section_offsets arrays).

I don't really like the approach taken by reread_symbols, trying to
reset everything manually on the objfile object, instead of, for
example, creating a new one from scratch.  But I don't know enough yet
to propose a better solution.

One more reason I think this patch is needed is that the number of
sections of the new executable could be different from the number of
sections of the old executable.  So if we don't re-create the
section_offsets array, not only we'll have wrong offsets, but we could
make accesses past the array.

Something else that silently fails (but doesn't seem to have
consequences) is the prologue analysis when we try to create the
breakpoint on `main`.  Since the `main` symbol has the wrong value
0x555555555129, we try to access memory in that area, which fails.  This
can be observed by debugging gdb and using `catch throw`.  Before the
process is started, we need to access the memory at its unrelocated
address, 0x1129, which will read memory from the ELF file.  This is now
what happens, with this patch applied.

It silently fails, probably because commit 46a62268b, "Catch exceptions
thrown from gdbarch_skip_prologue", papered over the problem and added
an empty catch clause.  I'm quite sure that the root cause then was the
one fixed by this patch.

This fixes tests gdb.ada/exec_changed.exp and gdb.base/reread.exp for
me.

gdb/ChangeLog:

	* symfile.c (reread_symbols): Clear objfile's section_offsets
	vector and section indices, re-compute them by calling
	sym_offsets.
---
 gdb/symfile.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/gdb/symfile.c b/gdb/symfile.c
index dd8192a67fbb..b02a9235663b 100644
--- a/gdb/symfile.c
+++ b/gdb/symfile.c
@@ -2543,6 +2543,11 @@ reread_symbols (void)
 	     will need to be called (see discussion below).  */
 	  obstack_free (&objfile->objfile_obstack, 0);
 	  objfile->sections = NULL;
+	  objfile->section_offsets.clear ();
+	  objfile->sect_index_bss = -1;
+	  objfile->sect_index_data = -1;
+	  objfile->sect_index_rodata = -1;
+	  objfile->sect_index_text = -1;
 	  objfile->compunit_symtabs = NULL;
 	  objfile->template_symbols = NULL;
 	  objfile->static_links.reset (nullptr);
@@ -2597,6 +2602,9 @@ reread_symbols (void)
 
 	  objfiles_changed ();
 
+	  /* Recompute section offsets and section indices.  */
+	  objfile->sf->sym_offsets (objfile, {});
+
 	  read_symbols (objfile, 0);
 
 	  if (!objfile_has_symbols (objfile))
-- 
2.26.2

Re: [PATCH] gdb: reset/recompute objfile section offsets in reread_symbols

[Christian Biesinger]

On Wed, May 20, 2020 at 2:22 PM Simon Marchi via Gdb-patches
<gdb-patches@sourceware.org> wrote:
> diff --git a/gdb/symfile.c b/gdb/symfile.c
> index dd8192a67fbb..b02a9235663b 100644
> --- a/gdb/symfile.c
> +++ b/gdb/symfile.c
> @@ -2543,6 +2543,11 @@ reread_symbols (void)
>              will need to be called (see discussion below).  */
>           obstack_free (&objfile->objfile_obstack, 0);
>           objfile->sections = NULL;
> +         objfile->section_offsets.clear ();
> +         objfile->sect_index_bss = -1;
> +         objfile->sect_index_data = -1;
> +         objfile->sect_index_rodata = -1;
> +         objfile->sect_index_text = -1;

Would it make sense to have a reset() function on objfile instead,
that handles all necesary clearing of members?

Christian

Re: [PATCH] gdb: reset/recompute objfile section offsets in reread_symbols

[Simon Marchi]

On 2020-05-20 3:24 p.m., Christian Biesinger via Gdb-patches wrote:
> On Wed, May 20, 2020 at 2:22 PM Simon Marchi via Gdb-patches
> <gdb-patches@sourceware.org> wrote:
>> diff --git a/gdb/symfile.c b/gdb/symfile.c
>> index dd8192a67fbb..b02a9235663b 100644
>> --- a/gdb/symfile.c
>> +++ b/gdb/symfile.c
>> @@ -2543,6 +2543,11 @@ reread_symbols (void)
>>              will need to be called (see discussion below).  */
>>           obstack_free (&objfile->objfile_obstack, 0);
>>           objfile->sections = NULL;
>> +         objfile->section_offsets.clear ();
>> +         objfile->sect_index_bss = -1;
>> +         objfile->sect_index_data = -1;
>> +         objfile->sect_index_rodata = -1;
>> +         objfile->sect_index_text = -1;
> 
> Would it make sense to have a reset() function on objfile instead,
> that handles all necesary clearing of members?
> 
> Christian
> 

Probably not ideal, but probably better than what we have now.  At least
the knowledge of how to reset an objfile to an original state would be in
the objfile class itself.

I think it would be nicer to just free the objfile and create a new one from
scratch, although I don't know what other problems come with that.

Simon

Re: [PATCH] gdb: reset/recompute objfile section offsets in reread_symbols

[Tom Tromey]

>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:

Simon> This patch fixes this by making reread_symbols clear the
Simon> `section_offsets` vector, and re-create it by calling `sym_offsets`.
Simon> This is analogous to what syms_from_objfile_1 does.  I didn't seem
Simon> absolutely necessary, but I also made it clear the various
Simon> `sect_index_*` fields, since their values no longer make sense (they
Simon> describe the old executable, and are indices in the now cleared
Simon> sections/section_offsets arrays).

This makes sense to me.

Simon> I don't really like the approach taken by reread_symbols, trying to
Simon> reset everything manually on the objfile object, instead of, for
Simon> example, creating a new one from scratch.

A long time ago, Jan had a patch to do exactly this.
I couldn't easily find it; there was some opposition to it at the time,
but I still think it's a better approach.

thanks,
Tom

Re: [PATCH] gdb: reset/recompute objfile section offsets in reread_symbols

[Simon Marchi]

On 2020-05-20 3:36 p.m., Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> Simon> This patch fixes this by making reread_symbols clear the
> Simon> `section_offsets` vector, and re-create it by calling `sym_offsets`.
> Simon> This is analogous to what syms_from_objfile_1 does.  I didn't seem
> Simon> absolutely necessary, but I also made it clear the various
> Simon> `sect_index_*` fields, since their values no longer make sense (they
> Simon> describe the old executable, and are indices in the now cleared
> Simon> sections/section_offsets arrays).
> 
> This makes sense to me.

Thanks, I'll push the patch then.

> Simon> I don't really like the approach taken by reread_symbols, trying to
> Simon> reset everything manually on the objfile object, instead of, for
> Simon> example, creating a new one from scratch.
> 
> A long time ago, Jan had a patch to do exactly this.
> I couldn't easily find it; there was some opposition to it at the time,
> but I still think it's a better approach.

Here's the thread:

https://sourceware.org/legacy-ml/gdb-patches/2009-06/msg00679.html

Simon