Fix PR 21337 v2: segfault when re-reading symbols with remote debugging.

  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
  

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