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

[Simon Marchi <simon.marchi@polymtl.ca>]

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