From patchwork Fri Mar 22 12:39:33 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Pedro Alves X-Patchwork-Id: 31940 Received: (qmail 83708 invoked by alias); 22 Mar 2019 12:39:40 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 83412 invoked by uid 89); 22 Mar 2019 12:39:39 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_NONE autolearn=ham version=3.3.1 spammy=franco, Franco, hang, WDYT X-HELO: mail-wr1-f52.google.com Received: from mail-wr1-f52.google.com (HELO mail-wr1-f52.google.com) (209.85.221.52) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 22 Mar 2019 12:39:37 +0000 Received: by mail-wr1-f52.google.com with SMTP id w1so2212067wrp.2 for ; Fri, 22 Mar 2019 05:39:37 -0700 (PDT) Return-Path: Received: from ?IPv6:2001:8a0:f913:f700:56ee:75ff:fe8d:232b? ([2001:8a0:f913:f700:56ee:75ff:fe8d:232b]) by smtp.gmail.com with ESMTPSA id q17sm6622649wrx.38.2019.03.22.05.39.34 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 22 Mar 2019 05:39:34 -0700 (PDT) Subject: [PATCH] Fix testsuite hangs when gdb_test_multiple body errors out (Re: GDB 8.2.90 available for testing) To: Pedro Franco de Carvalho , Joel Brobecker , gdb-patches@sourceware.org References: <20190227055112.4A5E782D7B@joel.gnat.com> <8736ny7f8u.fsf@linux.vnet.ibm.com> From: Pedro Alves Message-ID: <4d855905-32ce-ba4b-72f5-037f1796b37e@redhat.com> Date: Fri, 22 Mar 2019 12:39:33 +0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <8736ny7f8u.fsf@linux.vnet.ibm.com> Hi Pedro, Thanks for the detailed report. On 03/07/2019 05:42 PM, Pedro Franco de Carvalho wrote: > I'm not sure if there is a proper way to work around this in GDB, but it > would be useful so that the testsuite doesn't hang in these cases. I spent a while trying to fix this, and I came up with the patch below. WDYT? From 17e8f28072cce040524eab7b85b8767764a54cd2 Mon Sep 17 00:00:00 2001 From: Pedro Alves Date: Fri, 22 Mar 2019 11:33:19 +0000 Subject: [PATCH] Fix testsuite hangs when gdb_test_multiple body errors out This commit fixes a regression in the testsuite itself, triggered by errors being raised from within gdb_test_multiple, originally reported by Pedro Franco de Carvalho's at . Parts of the commit message are based on his report. This started happening due to a commit that was introduced recently, and it can cause the testsuite to hang. The commit that triggers this is: fe1a5cad302b5535030cdf62895e79512713d738 [gdb/testsuite] Log wait status on process no longer exists error That commit introduces a new "eof" block in gdb_test_multiple. That is not incorrect itself, but dejagnu's remote_expect is picking that block as the "default" action when an error is raised from within the commands inside a call to gdb_test_multiple: # remote_expect works basically the same as standard expect, but it # also takes care of getting the file descriptor from the specified # host and also calling the timeout/eof/default section if there is an # error on the expect call. # proc remote_expect { board timeout args } { I find that "feature" surprising, and I don't really know why it exists, but this means that the eof section that remote_expect picks as the error block can be executed even when there was no actual eof and the GDB process is still running, so the wait introduced in the commit that tries to get the exit status of GDB hangs forever, while GDB itself waits for input. This only happens when there are internal testsuite errors (not testcase failures). This can be reproduced easily with a testcase such as: gdb_start gdb_test_multiple "show version" "show version" { -re ".*" { error "forced error" } } I think that working around this in GDB is useful so that the testsuite doesn't hang in these cases. Adding an empty "default" block at the end of the expect body in gdb_test_multiple doesn't work, because dejagnu gives preference to "eof" blocks: if { $x eq "eof" } { set save_next 1 } elseif { $x eq "default" || $x eq "timeout" } { if { $error_sect eq "" } { set save_next 1 } } And we do have "eof" blocks. So we need to make sure that the last "eof" block is safe to use as the default error block. It's also pedantically incorrect to print "ERROR: Process no longer exists" which is what we'd get if the last eof block we have was selected (more below on this). So this commit solves this by appending an "eof" with an empty spawn_id list, so that it won't ever match. Now, why is the first "eof" block selected today as the error block, instead of the last one? The reason is that remote_expect, while parsing the body to select the default block to execute after an error, is affected by the comments in the body (since they are also parsed). If this comment in gdb_test_multiple # patterns below apply to any spawn id specified. is changed to # The patterns below apply to any spawn id specified. then the second eof block is selected and there is no hang. Any comment at that same place with an even-numbered of tokens also works. This is IMO a coincidence caused by how comments work in TCL. Comments should only appear in places where a command can appear. And here, remote_expect is parsing a list of options, not commands, so it's not unreasonable to not parse comments, similarly to how this: set a_list { an_element # another_element } results in a list with three elements, not one element. The fact that comments with an even number of tokens work is just a coincidence of how remote_expect's little state machine is implemented. I thought we could solve this by stripping out comment lines in gdb_expect, but I didn't find an easy way to do that. Particularly, a couple naive approaches I tried run into complications. For example, we have gdb_test calls with regular expressions that include sequences like "\r\n#", and by the time we get to gdb_expect, the \r\n have already been expanded to a real newline, so just splitting the whole body at newline boundaries, looking for lines that start with # results in incorrectly stripping out half of the gdb_text regexp. I think it's better (at least in this commit), to move the comments out of the list, because it's much simpler and risk free. gdb/ChangeLog: yyyy-mm-dd Pedro Alves * lib/gdb.exp (gdb_test_multiple): Split appends to $code and move comments outside list. Append '-i "" eof' section. --- gdb/testsuite/lib/gdb.exp | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp index 6800c74187..4600d2c347 100644 --- a/gdb/testsuite/lib/gdb.exp +++ b/gdb/testsuite/lib/gdb.exp @@ -906,10 +906,13 @@ proc gdb_test_multiple { command message user_code } { } } append code $processed_code + + # Reset the spawn id, in case the processed code used -i. append code { - # Reset the spawn id, in case the processed code used -i. -i "$gdb_spawn_id" + } + append code { -re "Ending remote debugging.*$gdb_prompt $" { if ![isnative] then { warning "Can`t communicate to remote target." @@ -990,8 +993,10 @@ proc gdb_test_multiple { command message user_code } { } return -1 } + } - # Patterns below apply to any spawn id specified. + # Now patterns that apply to any spawn id specified. + append code { -i $any_spawn_id eof { perror "Process no longer exists" @@ -1013,6 +1018,20 @@ proc gdb_test_multiple { command message user_code } { } } + # remote_expect calls the eof section if there is an error on the + # expect call. We already have "eof" sections above, and we don't + # want them to get called in that situation. Since the last "eof" + # section becomes the error section, here we define another "eof" + # section, but with an empty spawn_id list, so that it won't ever + # match anything. + append code { + -i "" eof { + # This comment is here because the eof section must not be + # the empty string, otherwise remote_expect won't realize + # it exists. + } + } + set result 0 set code [catch {gdb_expect $code} string] if {$code == 1} {