gdb: reset/recompute objfile section offsets in reread_symbols

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.

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index bef88d0df6a32fa4c3009e9bfbc2ff5a7c156bab..38d18976110982a67319404d25763b03968c38e0 100644 (file)
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,9 @@
+2020-05-20  Simon Marchi  <simon.marchi@efficios.com>
+
+       * symfile.c (reread_symbols): Clear objfile's section_offsets
+       vector and section indices, re-compute them by calling
+       sym_offsets.
+
 2020-05-20  Tom Tromey  <tromey@adacore.com>
 
        * ada-lang.c (bound_name, MAX_ADA_DIMENS): Remove.

diff --git a/gdb/symfile.c b/gdb/symfile.c
index dd8192a67fbba4f40d2b68a3f8e9b0ad86318d50..b02a9235663b00823d687c002630de566ef7e07b 100644 (file)
--- 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))