Re: [PATCH] Fix PR 21337 v2: segfault when re-reading symbols with remote debugging.

[Doug Gilmore <Doug dot Gilmore at imgtec dot com>]

On 04/29/2017 10:14 PM, Doug Gilmore wrote:
>
> >>> ...
> >> I'll need to take a look.  Last time I tried I it was more difficult
> >> to expose the problem on the native build of GDB.
> >
> > reread_symbols is called when using the run (run_command_1), attach (attach_post_wait which then calls setup_inferior) and load (load_command) commands.  So maybe something like this would reproduce it?
> >
> > - compile test program
> > - launch gdb with test program
> > - touch test program
> > - run
> >
> > Simon
> >
> Hi Simon,
>
> What I meant was that when I previously did tests with the native
> build, for some reason the freed data was not being overwritten, or
> possible written with the same data, at the time read_symbols was
> called in reread_symbols.  Thus problem wasn't exposed before the
> objfiles_changed is eventually called in reread_symbols.
>
> I just did a test with a native build of gdb (7.9.1) and the problem
> was exposed, so chances are it will be also be exposed with a ToT
> build.
>
> Doug
>
It tooks some experimentation, but I found a quick, and it appears
reliable, way to reproduce the problem natively.

I have attached a new patch with commit with a detailed explanation
for the commit log entry.

OK to apply?

Thanks,

Doug

Fix PR 21337: segfault when re-reading symbols.

Fix issue exposed by commit 3e29f34.

The basic issue is that section data referenced through an objfile
pointer can also be referenced via the program-space data pointer,
although via a separate mapping mechanism, which is set up by
update_section_map.  Thus once section data attached to an objfile
pointer is released, the section map associated with the program-space
data pointer must be marked dirty to ensure that update_section_map is
called to prevent stale data being referenced.  For the matter at hand
this marking is being done via a call to objfiles_changed.

Before commit 3e29f34 objfiles_changed could be called after all of
the objfile pointers were processed in reread_symbols since section
data references via the program-space data pointer would not occur in
the calls of read_symbols performed by reread_symbols.

With commit 3e29f34 MIPS target specific calls to find_pc_section were
added to the code for DWARF information processing, which is called
via read_symbols.  Thus in reread_symbols the call to objfiles_changed
needs to be called before calling read_symbols, otherwise stale
section data can be referenced.

Thanks to Luis Machado for providing text for the main comment
associated with the change.

gdb/
2017-??-??  Doug Gilmore  <Doug.Gilmore@Doug.Gilmore@imgtec.com>
    * symfile.c (reread_symbols): Fix PR 21337.

gdb/testsuite
2017-??-??  Doug Gilmore  <Doug Gilmore@rDoug.Gilmore@imgtec.com>
    PR gdb/r21337
    * gdb.base/pr21337.c: New file.
    * gdb.base/pr21337.exp: New file.
    * gdb.base/pr21337.gdb: New file.
---
 gdb/symfile.c                      | 20 +++++++++++--
 gdb/testsuite/gdb.base/pr21337.c   |  4 +++
 gdb/testsuite/gdb.base/pr21337.exp | 57 ++++++++++++++++++++++++++++++++++++++
 gdb/testsuite/gdb.base/pr21337.gdb | 13 +++++++++
 4 files changed, 91 insertions(+), 3 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/pr21337.c
 create mode 100644 gdb/testsuite/gdb.base/pr21337.exp
 create mode 100644 gdb/testsuite/gdb.base/pr21337.gdb

diff --git a/gdb/symfile.c b/gdb/symfile.c
index 846aabe..0492f56 100644
--- a/gdb/symfile.c
+++ b/gdb/symfile.c
@@ -2576,6 +2576,9 @@ reread_symbols (void)
       /* Free the obstacks for non-reusable objfiles.  */
       psymbol_bcache_free (objfile->psymbol_cache);
       objfile->psymbol_cache = psymbol_bcache_init ();
+
+      /* NB: after this call to obstack_free, objfiles_changed
+         will need to be called (see discussion below).  */
       obstack_free (&objfile->objfile_obstack, 0);
       objfile->sections = NULL;
       objfile->compunit_symtabs = NULL;
@@ -2628,6 +2631,20 @@ reread_symbols (void)
       clear_complaints (&symfile_complaints, 1, 1);
 
       objfile->flags &= ~OBJF_PSYMTABS_READ;
+
+      /* We are about to read new symbols and potentially also DWARF
+         information.  Some targets may want to pass addresses read from
+         DWARF DIE's through an adjustment function before saving them, like
+         MIPS, which may call into "find_pc_section".  When called, that
+         function will make use of per-objfile program space data.
+
+         Since we discarded our section information above, we have dangling
+         pointers in the per-objfile program space data structure.  Force
+         GDB to update the section mapping information by letting it know
+         the objfile has changed, making the dangling pointers point to
+         correct data again.  */
+      objfiles_changed ();
+
       read_symbols (objfile, 0);
 
       if (!objfile_has_symbols (objfile))
@@ -2660,9 +2677,6 @@ reread_symbols (void)
 
   if (!new_objfiles.empty ())
     {
-      /* Notify objfiles that we've modified objfile sections.  */
-      objfiles_changed ();
-
       clear_symtab_users (0);
 
       /* clear_objfile_data for each objfile was called before freeing it and
diff --git a/gdb/testsuite/gdb.base/pr21337.c b/gdb/testsuite/gdb.base/pr21337.c
new file mode 100644
index 0000000..f8b643a
--- /dev/null
+++ b/gdb/testsuite/gdb.base/pr21337.c
@@ -0,0 +1,4 @@
+int main()
+{
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.base/pr21337.exp b/gdb/testsuite/gdb.base/pr21337.exp
new file mode 100644
index 0000000..d7718b4
--- /dev/null
+++ b/gdb/testsuite/gdb.base/pr21337.exp
@@ -0,0 +1,57 @@
+# Copyright 1998-2017 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+set prototypes 1
+
+# build the test case
+
+set testfile "pr21337"
+set srcfile ${testfile}.c
+# Cygwin needs $EXEEXT.
+set binfile [standard_output_file ${testfile}$EXEEXT]
+# set binfile ${testfile}
+
+set gdbfile [standard_output_file ${testfile}.gdb]
+
+if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug nowarnings}] != "" } {
+    untested "failed to compile first testcase"
+    return -1
+}
+
+# Using the source command to read commands from a file is important,
+# otherwise section data is freed and reallocated using the same
+# memory locations and the bug is not exposed.
+
+set ifd [open "$srcdir/$subdir/${testfile}.gdb" r]
+set ofd [open $gdbfile w]
+while {[gets $ifd line] >= 0} {
+    regsub $testfile $line $binfile tmp
+    puts $ofd $tmp
+}
+close $ifd
+close $ofd
+
+gdb_start
+
+if [is_remote target] {
+    unsupported $test
+} else {
+    gdb_test "source $gdbfile" ".*source-command-completed.*" "source $testfile.gdb"
+    gdb_test "source $gdbfile" ".*source-command-completed.*" "source $testfile.gdb"
+}
+
+# End of tests.
+
+return 0
diff --git a/gdb/testsuite/gdb.base/pr21337.gdb b/gdb/testsuite/gdb.base/pr21337.gdb
new file mode 100644
index 0000000..42fac26
--- /dev/null
+++ b/gdb/testsuite/gdb.base/pr21337.gdb
@@ -0,0 +1,13 @@
+file pr21337
+shell sleep 1; touch pr21337
+run
+file
+file pr21337
+shell sleep 1; touch pr21337
+run
+file
+file pr21337
+shell sleep 1; touch pr21337
+run
+file
+p "source-command-completed"
-- 
1.9.1