From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 92076 invoked by alias); 12 May 2017 19:24:34 -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 91983 invoked by uid 89); 12 May 2017 19:24:26 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.7 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_NONE,RP_MATCHES_RCVD,SPF_PASS autolearn=ham version=3.3.2 spammy=Among X-HELO: mailapp01.imgtec.com Received: from mailapp01.imgtec.com (HELO mailapp01.imgtec.com) (195.59.15.196) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 12 May 2017 19:24:20 +0000 Received: from hhmail02.hh.imgtec.org (unknown [10.100.10.20]) by Forcepoint Email with ESMTPS id 90EB7FE8AB752; Fri, 12 May 2017 20:24:15 +0100 (IST) Received: from HHMAIL-X.hh.imgtec.org (10.100.10.113) by hhmail02.hh.imgtec.org (10.100.10.20) with Microsoft SMTP Server (TLS) id 14.3.294.0; Fri, 12 May 2017 20:24:20 +0100 Received: from BAMAIL02.ba.imgtec.org (10.20.40.28) by HHMAIL-X.hh.imgtec.org (10.100.10.113) with Microsoft SMTP Server (TLS) id 14.3.294.0; Fri, 12 May 2017 20:24:19 +0100 Received: from [10.20.2.42] (10.20.2.42) by bamail02.ba.imgtec.org (10.20.40.28) with Microsoft SMTP Server id 14.3.266.1; Fri, 12 May 2017 12:24:17 -0700 From: Doug Gilmore To: Simon Marchi CC: Luis Machado , Subject: Re: [PATCH] Fix PR 21337 v2: segfault when re-reading symbols with remote debugging. 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> <09492e58ce0daf1efee14636bc5312cc@polymtl.ca> Message-ID: <48b9619c-a8b2-2920-4a8e-e9991e0aefaf@imgtec.com> Date: Fri, 12 May 2017 19:24:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <09492e58ce0daf1efee14636bc5312cc@polymtl.ca> Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit X-SW-Source: 2017-05/txt/msg00313.txt.bz2 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