From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 60432 invoked by alias); 12 May 2017 03:29:45 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 60396 invoked by uid 89); 12 May 2017 03:29:43 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-24.0 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,SPF_HELO_PASS,SPF_SOFTFAIL autolearn=ham version=3.3.2 spammy= X-HELO: simark.ca Received: from simark.ca (HELO simark.ca) (158.69.221.121) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 12 May 2017 03:29:40 +0000 Received: by simark.ca (Postfix, from userid 33) id 3AE201E4B5; Thu, 11 May 2017 23:29:42 -0400 (EDT) To: Doug Gilmore Subject: Re: [PATCH] Fix PR 21337 v2: segfault when re-reading symbols with remote debugging. X-PHP-Originating-Script: 33:rcube.php MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII; format=flowed Content-Transfer-Encoding: 7bit Date: Fri, 12 May 2017 03:29:00 -0000 From: Simon Marchi Cc: Luis Machado , gdb-patches@sourceware.org In-Reply-To: <28fcce08-cab6-1e63-80d7-1d61c688cc10@imgtec.com> References: <20511c76-c816-d31d-5144-749eac9fc470@imgtec.com> <3c5ce0a0-72e5-4460-5555-ad2214866260@imgtec.com> <5c494cc147f71dd8246572aa0b815c9f@polymtl.ca> <7e9595026acbfd2f1a7bff321fa255e1@polymtl.ca> <5b5cc0a61e434a3406cbb25c16b8a550@polymtl.ca> <28fcce08-cab6-1e63-80d7-1d61c688cc10@imgtec.com> Message-ID: <09492e58ce0daf1efee14636bc5312cc@polymtl.ca> X-Sender: simon.marchi@polymtl.ca User-Agent: Roundcube Webmail/1.2.5 X-IsSubscribed: yes X-SW-Source: 2017-05/txt/msg00307.txt.bz2 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 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 > 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 > . > + > +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