From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-1.mimecast.com (us-smtp-delivery-1.mimecast.com [205.139.110.120]) by sourceware.org (Postfix) with ESMTP id 1109E38708F9 for ; Sat, 20 Jun 2020 16:25:48 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 1109E38708F9 Received: from mail-wr1-f70.google.com (mail-wr1-f70.google.com [209.85.221.70]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-212-ewYNN_lfMuykfIi5zaJDeg-1; Sat, 20 Jun 2020 12:25:45 -0400 X-MC-Unique: ewYNN_lfMuykfIi5zaJDeg-1 Received: by mail-wr1-f70.google.com with SMTP id w4so6643040wrl.13 for ; Sat, 20 Jun 2020 09:25:45 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=yLW7COoQm73niSiXQEO9z/CYvIyKxyNQNBnDAmtf5h0=; b=Jtbz39pOcq9eZylTV6zqOEIIVqaiJlhnAa/i80qrjyXST14kzhArFQNnWuQSJzWXwX /793Zgrw3Bzw0zzrDc9B0q+Q3owbMghdNprSOu3hx2HcKxL1L0RwuivoBOXRvaG2N2OV BaQmGPGBKQ5zdwcmLnR8XxP2v132OftrjsAcpK+x0pCK1zekOzJcow32fNio9SFXGtaJ javW650KH3KftGs/LIp2MKgqvkHc6R59ss2jrOnN/mbXWESK5Rvn4Gwk0tKJ8m1qH929 KGhr8vDk45B1rmVDe3fzi78DazqllXTWJxo6P7SnllbZU3OKWxiVrenDX98ReAiswjdS 1/+A== X-Gm-Message-State: AOAM530Phqy/c6PpcOqF1zsR7ubAKQNDOKpL7syKwAH9VNtZYyjdngQ6 aRErfv1g7NZ2cQ0FMalYHDfApHRqyqgPpxFORcUevDZhXqQoBtFp65YkHMvfgRlhf5oFTcvBAom oQndP37OGpigysemd2O7/qw== X-Received: by 2002:adf:ed87:: with SMTP id c7mr9817500wro.108.1592670344194; Sat, 20 Jun 2020 09:25:44 -0700 (PDT) X-Google-Smtp-Source: ABdhPJx46Ma8CYnX+nD909/Yb2hvslhPEBfHQ9fFUuYDc8kPp8ozRK28PxGE2WNPr+sm1gh6+Ut0hA== X-Received: by 2002:adf:ed87:: with SMTP id c7mr9817483wro.108.1592670343887; Sat, 20 Jun 2020 09:25:43 -0700 (PDT) Received: from ?IPv6:2001:8a0:f922:c400:56ee:75ff:fe8d:232b? ([2001:8a0:f922:c400:56ee:75ff:fe8d:232b]) by smtp.gmail.com with ESMTPSA id l1sm11599475wrb.31.2020.06.20.09.25.42 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Sat, 20 Jun 2020 09:25:43 -0700 (PDT) Subject: Re: [RFAv2] Ensure 'exec-file has changed' check has priority over 'exec-file-mismatch' check To: Philippe Waroquiers , gdb-patches@sourceware.org References: <20200604204200.21266-1-philippe.waroquiers@skynet.be> From: Pedro Alves Message-ID: Date: Sat, 20 Jun 2020 17:25:42 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.2.1 MIME-Version: 1.0 In-Reply-To: <20200604204200.21266-1-philippe.waroquiers@skynet.be> Content-Language: en-US X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-8.8 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H4, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 20 Jun 2020 16:25:49 -0000 On 6/4/20 9:42 PM, Philippe Waroquiers via Gdb-patches wrote: > This is version 2. Compared to first version: > rebased to last master > removed a comment duplicated by error > 'git add-ed' the attach3.c file I forgot. > > Following the implementation of exec-file-mismatch based on build-id, > an attach to a process that runs a modified exec-file was triggering > the exec-file-mismatch handling, giving a warning such as: > warning: Mismatch between current exec-file /bd/home/philippe/gdb/git/build_termours/gdb/testsuite/outputs/gdb.base/attach/attach > and automatically determined exec-file /bd/home/philippe/gdb/git/build_termours/gdb/testsuite/outputs/gdb.base/attach/attach > exec-file-mismatch handling is currently "ask" > as the build-ids differ when an exec-file is recompiled. I wonder whether the warning should mention Build IDs, like - warning: Mismatch between ... + warning: Build ID mismatch between ... > > This patch ensures that the exec-file-mismatch check is done with an up to date > build-id. With this, exec-file-mismatch check will only trigger when the > PID file really differs from the (build-id refreshed) current exec-file. > Note that the additional check does not (yet) reload the symbols if > the exec-file is changed: this reload will happen later if needed. > > gdb/ChangeLog > YYYY-MM-DD Philippe Waroquiers > > * exec.c (validate_exec_file): Ensure the build-id is up to > date by calling reopen_exec_file (that checks file timestamp > to decide to re-read the file). > > gdb/testsuite/ChangeLog > > YYYY-MM-DD Philippe Waroquiers > > * gdb.base/attach.exp: Test priority of 'exec-file' changed > over 'exec-file-mismatch'. > * gdb.base/attach.c: Mark should_exit volatile. > * gdb.base/attach2.c: Likewise. Add a comment explaining > why the sleep cannot be big. > * gdb.base/attach3.c: New file. > --- > gdb/exec.c | 12 +++++-- > gdb/testsuite/gdb.base/attach.c | 2 +- > gdb/testsuite/gdb.base/attach.exp | 56 +++++++++++++++++++++++++++++-- > gdb/testsuite/gdb.base/attach2.c | 4 ++- > gdb/testsuite/gdb.base/attach3.c | 25 ++++++++++++++ > 5 files changed, 93 insertions(+), 6 deletions(-) > create mode 100644 gdb/testsuite/gdb.base/attach3.c > > diff --git a/gdb/exec.c b/gdb/exec.c > index ee13c5e027..fe4f94f634 100644 > --- a/gdb/exec.c > +++ b/gdb/exec.c > @@ -254,8 +254,16 @@ validate_exec_file (int from_tty) > if (current_exec_file == NULL || pid_exec_file == NULL) > return; > > - /* Try validating via build-id, if available. This is the most > - reliable check. */ > + /* Try validating via build-id, if available. This is the most > + reliable check. */ Spurious change. > + > + /* In case current_exec_file was changed, reopen_exec_file ensures > + an up to date build_id (will do nothing if the file timestamp > + did not change). If exec file changed, reopen_exec_file has > + allocated another file name, so get_exec_file again. */ > + reopen_exec_file (); > + current_exec_file = get_exec_file (0); > + I think that ideally we wouldn't reopen the exec file here, but instead move the exec validation to setup_inferior, after the reopen/reread: void setup_inferior (int from_tty) { struct inferior *inferior; inferior = current_inferior (); inferior->needs_setup = 0; /* If no exec file is yet known, try to determine it from the process itself. */ if (get_exec_file (0) == NULL) exec_file_locate_attach (inferior_ptid.pid (), 1, from_tty); else { reopen_exec_file (); reread_symbols (); + validate_exec_file (from_tty); } That would centralize things better, and also, when we later teach GDB to validate the symbol-file 's build ID as well, it would all be done together. We could also query debuginfod for a matching binary around here. But that's easier said than done. Particularly the remote target's initial setup doesn't always end up here in setup_inferior; it's quite messy. I gave it a try, but gave up for now, because it was requiring changing too much of the initial remote connection setup code and taking too long. Can always revisit later. So let's go with what you have. Some comments on the testcase below. But otherwise, with those addressed, please go ahead and push. > const bfd_build_id *exec_file_build_id = build_id_bfd_get (exec_bfd); > if (exec_file_build_id != nullptr) > { > diff --git a/gdb/testsuite/gdb.base/attach.c b/gdb/testsuite/gdb.base/attach.c > index 2e87f9b710..b3c5498401 100644 > --- a/gdb/testsuite/gdb.base/attach.c > +++ b/gdb/testsuite/gdb.base/attach.c > @@ -8,7 +8,7 @@ > #include > > int bidule = 0; > -int should_exit = 0; > +volatile int should_exit = 0; > > int main () > { > diff --git a/gdb/testsuite/gdb.base/attach.exp b/gdb/testsuite/gdb.base/attach.exp > index 32f72e2a9a..84aede7e2a 100644 > --- a/gdb/testsuite/gdb.base/attach.exp > +++ b/gdb/testsuite/gdb.base/attach.exp > @@ -17,12 +17,13 @@ if {![can_spawn_for_attach]} { > return 0 > } > > -standard_testfile attach.c attach2.c > +standard_testfile attach.c attach2.c attach3.c > set binfile2 ${binfile}2 > +set binfile3 ${binfile}3 > set escapedbinfile [string_to_regexp $binfile] > > #execute_anywhere "rm -f ${binfile} ${binfile2}" > -remote_exec build "rm -f ${binfile} ${binfile2}" > +remote_exec build "rm -f ${binfile} ${binfile2} ${binfile3}" > # For debugging this test > # > #log_user 1 > @@ -41,6 +42,13 @@ if { [gdb_compile "${srcdir}/${subdir}/${srcfile2}" "${binfile2}" executable {d > return -1 > } > > +# Build the third file, used to check attach when exec-file has changed. Missing "the" in "when the exec-file". > + > +if { [gdb_compile "${srcdir}/${subdir}/${srcfile3}" "${binfile3}" executable {debug}] != "" } { > + untested "failed to compile attach exec-file changed test" > + return -1 > +} > + > if [get_compiler_info] { > return -1 > } > @@ -515,6 +523,7 @@ proc_with_prefix do_attach_exec_mismatch_handling_tests {} { > global gdb_prompt > global binfile > global binfile2 > + global binfile3 > > clean_restart $binfile > > @@ -577,10 +586,53 @@ proc_with_prefix do_attach_exec_mismatch_handling_tests {} { > # Detach the process. > gdb_test "detach" "Detaching from program: .* detached\\\]" "$test detach attach" > > + # Test that the 'exec-file' changed is checked before exec-file-mismatch. > + set test "mismatch exec-file changed has priority" > + gdb_test_no_output "set exec-file-mismatch ask" > + gdb_test_multiple "attach $testpid" "$test attach1 again, initial exec-file" { The second parameter here, should match ... > + -re "Attaching to program.*exec-file-mismatch handling is currently \"ask\".*Load new symbol table from .*attach\".*\(y or n\)" { > + pass "$test attach1 again" ... this message here. Otherwise a FAIL catch by the internal gdb_test_multiple patters ends up with a difference message from a PASS. Best is to use $gdb_test_name: pass $gdb_test_name > + } > + } > + gdb_test "y" "Reading symbols from .*attach.*" "$test load attach1 again" > + It's usually better if this "y" is done within the -re above, as it's all part of the same test, like: gdb_test_multiple "attach $testpid" "$test attach1 again, initial exec-file" { -re "Attaching to program.*exec-file-mismatch handling is currently \"ask\".*Load new symbol table from .*attach\".*\(y or n\)" { gdb_test "y" "Reading symbols from .*attach.*" $gdb_test_name } } Note this removes the separate "pass" call from within the -re. > + gdb_test "detach" "Detaching from program: .* detached\\\]" "$test detach attach initial exec-file" > + > + # Change the exec-file and attach to a new process using the changed file. > + remote_exec build "mv ${binfile} ${binfile}.initial" > + remote_exec build "mv ${binfile3} ${binfile}" > + # Ensure GDB detects ${binfile} has changed when checking timestamp. > + sleep 1 > + remote_exec build "touch ${binfile}" > + set test_spawn_id3 [spawn_wait_for_attach $binfile] > + set testpid3 [spawn_id_get_pid $test_spawn_id3] > + > + gdb_test "attach $testpid3" "Attaching to program.*attach' has changed; re-reading symbols.*" \ > + "$test attach1 again, after changing exec-file" > + gdb_test "detach" "Detaching from program: .* detached\\\]" "$test detach after attach changed exec-file" > + > + # Now, test the situation when current exec-file has changed > + # and we attach to a pid using another file. > + # Ensure GDB detects ${binfile} has changed when checking timestamp. > + sleep 1 > + remote_exec build "touch ${binfile}" > + > + gdb_test_multiple "attach $testpid2" "$test attach2" { > + -re "Attaching to program.*exec-file-mismatch handling is currently \"ask\".*Load new symbol table from .*attach2\".*\(y or n\)" { > + pass "$test attach2 with exec-file changed and need to load another exec-file" Same message mismatch issue as above. Thanks, Pedro Alves