Fix PR 21337 v2: segfault when re-reading symbols with remote debugging.
Commit Message
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
Comments
Hi Doug,
On 2017-05-10 19:26, Doug Gilmore wrote:
> 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.
Thanks for the commit log.
> gdb/
> 2017-??-?? Doug Gilmore <Doug.Gilmore@Doug.Gilmore@imgtec.com>
Your email address has twice the username part (in both ChangeLog
entries).
> * symfile.c (reread_symbols): Fix PR 21337.
This should state more precisely what actually changed:
* symfile.c (reread_symbols): Call objfiles_changed just before
read_symbols.
>
> gdb/testsuite
> 2017-??-?? Doug Gilmore <Doug Gilmore@rDoug.Gilmore@imgtec.com>
> PR gdb/r21337
Spurious "r" before the PR number.
> * 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()
Nit: to match the coding style of GDB:
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
Even though there are a few instances of this, we no longer name the
test cases using the PR number. Instead, let's try to find a relevant
name.
Also, you seem to have based your test case on an old file that does not
quite meet today's standards (especially the initial setup/build part).
For an example of test that follows today's practices, look at
gdb.base/step-over-exit.exp, for example.
> @@ -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
What does this do?
> +
> +# build the test case
> +
> +set testfile "pr21337"
> +set srcfile ${testfile}.c
> +# Cygwin needs $EXEEXT.
> +set binfile [standard_output_file ${testfile}$EXEEXT]
> +# set binfile ${testfile}
This should be replaced with a call to standard_testfile.
> +
> +set gdbfile [standard_output_file ${testfile}.gdb]
> +
> +if { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}"
> executable {debug nowarnings}] != "" } {
No need for "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.
I don't really know what's different when sourcing a file than typing
commands. But if your experience is that sourcing reproduces the crash
reliably while sending commands does not, I'm fine with that.
> +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
Would it be simpler to have a procedure to generate the .gdb file with
the right filename from the start? Something like:
proc generate_cmd_file { } {
set ofd [open $gdbfile w]
puts $ofd "file ${binfile}"
puts $ofd "shell sleep 1; touch ${binfile}"
...
}
> +
> +gdb_start
What is your intent in calling gdb_start? I think what you want is just
to start GDB itself, so that it can source the command file below. If
so, you should call clean_restart:
clear_restart ${binfile}
Among other things, this handles the extended-remote target (calling
"set remote exec-file"), which this test case should be able to support.
As in gdb.base/step-over-exit.exp, you can call prepare_for_testing
which does the build + clean_restart for you.
> +
> +if [is_remote target] {
Since your test relies on being able to "run" a program, the right check
would be [use_gdb_stub]:
if [use_gdb_stub] {
> + unsupported $test
> +} else {
> + gdb_test "source $gdbfile" ".*source-command-completed.*" "source
> $testfile.gdb"
> + gdb_test "source $gdbfile" ".*source-command-completed.*" "source
> $testfile.gdb"
Why is it needed to source the file twice?
> +}
> +
> +# End of tests.
> +
> +return 0
This comment and return statement are not necessary.
> 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"
Thanks for adding a test case!
Simon
On 05/11/2017 08:29 PM, Simon Marchi wrote:
> Hi Doug,
>
> On 2017-05-10 19:26, Doug Gilmore wrote:
> > 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.
>
> Thanks for the commit log.
>
>..
> Your email address has twice the username part (in both ChangeLog entries).
>
>..
> This should state more precisely what actually changed:
>
> * symfile.c (reread_symbols): Call objfiles_changed just before read_symbols.
>
> Spurious "r" before the PR number.
>..
Hi Simon
Whoops, thanks will update accordingly.
>
> ..
> Nit: to match the coding style of GDB:
>
> int
> main ()
OK
> ...
> Also, you seem to have based your test case on an old file that does
> not quite meet today's standards (especially the initial setup/build
> part). For an example of test that follows today's practices, look
> at gdb.base/step-over-exit.exp, for example.
Sounds good, I'll take a look.
>
> ...
> > +if { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug nowarnings}] != "" } {
>
> No need for "nowarnings".
OK
> I don't really know what's different when sourcing a file than
> typing commands. But if your experience is that sourcing reproduces
> the crash reliably while sending commands does not, I'm fine with
> that.
Right, it is really helpful in reproducing the issue reliably.
> ...
> Would it be simpler to have a procedure to generate the .gdb file with the right filename from the start? Something like:
>
> proc generate_cmd_file { } {
> set ofd [open $gdbfile w]
>
> puts $ofd "file ${binfile}"
> puts $ofd "shell sleep 1; touch ${binfile}"
> ...
> }
OK
>
> > +
> > +gdb_start
> ...
>
> What is your intent in calling gdb_start? I think what you want is
> just to start GDB itself, so that it can source the command file
> below. If so, you should call clean_restart:
> clear_restart ${binfile}
OK
> Among other things, this handles the extended-remote target (calling
> "set remote exec-file"), which this test case should be able to
> support. As in gdb.base/step-over-exit.exp, you can call
> prepare_for_testing which does the build + clean_restart for you.
OK
>
> > +
> > +if [is_remote target] {
>
> Since your test relies on being able to "run" a program, the right check would be [use_gdb_stub]:
>
> if [use_gdb_stub] {
>
> > + unsupported $test
> > +} else {
> > + gdb_test "source $gdbfile" ".*source-command-completed.*" "source
> > $testfile.gdb"
> > + gdb_test "source $gdbfile" ".*source-command-completed.*" "source
> > $testfile.gdb"
>
> Why is it needed to source the file twice?
In some situations I have seen the failure to occur only on the second invocation.
>
> > +}
> > +
> > +# End of tests.
> > +
> > +return 0
>
> This comment and return statement are not necessary.
OK
>
> ..
>
> Thanks for adding a test case!
>
> Simon
Will update the patch per your comments.
Thanks,
Doug
@@ -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
new file mode 100644
@@ -0,0 +1,4 @@
+int main()
+{
+ return 0;
+}
new file mode 100644
@@ -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
new file mode 100644
@@ -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"