From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTPS id B21CF3853560 for ; Wed, 29 Jun 2022 14:00:17 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org B21CF3853560 Received: from mimecast-mx02.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-124-DEkqEKJUOhOlTSp7QzwsDA-1; Wed, 29 Jun 2022 10:00:15 -0400 X-MC-Unique: DEkqEKJUOhOlTSp7QzwsDA-1 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.rdu2.redhat.com [10.11.54.3]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 0A6512932489 for ; Wed, 29 Jun 2022 14:00:15 +0000 (UTC) Received: from [10.97.116.55] (ovpn-116-55.gru2.redhat.com [10.97.116.55]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 569D81121314; Wed, 29 Jun 2022 14:00:14 +0000 (UTC) Message-ID: <3d59e300-1148-6013-dca7-192ae0b45ee1@redhat.com> Date: Wed, 29 Jun 2022 11:00:12 -0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.10.0 Subject: Re: [PATCHv3] gdb/testsuite: modernize gdb.base/maint.exp To: Andrew Burgess , gdb-patches@sourceware.org References: <20220509180431.31032-1-blarsen@redhat.com> <20220628183644.34139-1-blarsen@redhat.com> <87wnczdai8.fsf@redhat.com> From: Bruno Larsen In-Reply-To: <87wnczdai8.fsf@redhat.com> X-Scanned-By: MIMEDefang 2.78 on 10.11.54.3 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-12.8 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, NICE_REPLY_A, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_NONE, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) 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: Wed, 29 Jun 2022 14:00:23 -0000 On 6/29/22 07:33, Andrew Burgess wrote: > Bruno Larsen via Gdb-patches writes: > >> gdb.base/maint.exp was using several gdb_expect statements, probably >> because this test case predates the existance of gdb_test_multiple. This >> commit updates the test case to use gdb_test_multiple, making it more >> resilient to internal errors and such. >> >> The only gdb_expect left in the testcase is one that specifically looks >> for an internal error being triggered as a PASS. >> --- >> >> Changes for version 3: >> * changed patterns when testing psymbols, now using trailing >> parenthesis >> * used -lbl when running "check psymtabs" >> * made use of boolean variables and gdb_assert when testing >> psymtabs >> * Simplified msymbols and psymbosl test to 3 tests, instead of >> nested tests >> >> Changes for v2: >> - Addressed Andrew's comments >> - rebased on current master. >> >> --- >> gdb/testsuite/gdb.base/maint.exp | 142 ++++++++++--------------------- >> 1 file changed, 43 insertions(+), 99 deletions(-) >> >> diff --git a/gdb/testsuite/gdb.base/maint.exp b/gdb/testsuite/gdb.base/maint.exp >> index 2817c6eafb9..aea8a10df0c 100644 >> --- a/gdb/testsuite/gdb.base/maint.exp >> +++ b/gdb/testsuite/gdb.base/maint.exp >> @@ -151,22 +151,20 @@ set have_psyms [expr ! ( $have_gdb_index || $readnow_p )] >> # unless there is some problem with the symtabs and psymtabs >> # so that branch will really never be covered in this tests here!! >> # >> +# When there is a problem, there may be loads of output, which can >> +# overwhelm the expect buffer. Splitting it seems to fix those >> +# issues. >> + >> +set seen_command false >> +gdb_test_multiple "maint check-psymtabs" "" -lbl { > > Bruno, > > Thanks for continuing to work on this... > > Sorry to be a pain, but I don't think -lbl will actually do what you > want here. To me the implementation of -lbl just looks weird, and I'm > not sure how it is expected to help. Don't worry. This is why reviews are useful! > > Your patterns are: > > -re "^maint check-psymtabs\r\n" { > set seen_command true > exp_continue > } > > -re "^$gdb_prompt $" { > gdb_assert { $seen_command } $gdb_test_name > } > > And -lbl adds: > > -re "\r\n\[^\r\n\]*(?=\r\n)" { > exp_continue > } > > The problem I think exists here is that your first pattern runs from the > star of the buffer and consumes the trailing \r\n. Your prompt pattern > also expects to start from the start of the buffer. > > If the output is: > > maint check-psymtabs\r\n > (gdb) > > Then these patterns will work fine. But if your output is: > > maint check-psymtabs\r\n > Some other line here\r\n > (gdb) > > Then your first pattern will consume the first line, including the > trailing \r\n. The -lbl pattern will then fail to match the second line > as the -lbl pattern expects a leading \r\n. And now your pattern will > fail to match the third line as the second line is still in the buffer. > > The -lbl pattern always leaves a trailing \r\n in the expect buffer, > which will cause your prompt pattern to fail. > > Personally, I'd just drop the use of -lbl and add what I think is the > more correct pattern: > > -re "^\[^\r\n\]+\r\n" { > exp_continue > } > > which is what you originally proposed. But if you really want to make > use of -lbl then you can change your patterns to: > > -re "^maint check-psymtabs(?=\r\n)" { > set seen_command true > exp_continue > } > > -re "\r\n$gdb_prompt $" { > gdb_assert { $seen_command } $gdb_test_name > } > > I don't think that's an improvement, but I think that should work. This makes sense. I didn't really look at how -lbl worked because I wanted to reduce complexity on the gdb_test_multiple call, but if I need to check end of line without consuming, I don't think we're really reducing the complexity of anything. I'll revert this to how it was on v2. > >> + -re "^maint check-psymtabs\r\n" { >> + set seen_command true >> + exp_continue >> + } >> >> -# guo: on linux this command output is huge. for some reason splitting up >> -# the regexp checks works. >> -# >> -send_gdb "maint check-psymtabs\n" >> -gdb_expect { >> - -re "^maint check-psymtabs" { >> - gdb_expect { >> - -re "$gdb_prompt $" { >> - pass "maint check-psymtabs" >> - } >> - timeout { fail "(timeout) maint check-psymtabs" } >> - } >> + -re "^$gdb_prompt $" { >> + gdb_assert { $seen_command } $gdb_test_name >> } >> - -re ".*$gdb_prompt $" { fail "maint check-psymtabs" } >> - timeout { fail "(timeout) maint check-psymtabs" } >> } >> >> # This command does not produce any output unless there is some problem >> @@ -272,63 +270,33 @@ if { $have_psyms } { >> "maint print psymbols -pc" \ >> "maint print psymbols -pc main $psymbols_output"] >> foreach { test_name command } $test_list { >> - send_gdb "$command\n" >> - gdb_expect { >> - -re "^maint print psymbols \[^\n\]*\r\n$gdb_prompt $" { >> - send_gdb "shell ls $psymbols_output\n" >> - gdb_expect { >> - -re "$psymbols_output_re\r\n$gdb_prompt $" { >> - # We want this grep to be as specific as possible, >> - # so it's less likely to match symbol file names in >> - # psymbols_output. Yes, this actually happened; >> - # poor expect got tons of output, and timed out >> - # trying to match it. --- Jim Blandy >> - send_gdb "shell grep 'main.*function' $psymbols_output\n" >> - gdb_expect { >> - -re ".main., function, $hex.*$gdb_prompt $" { >> - pass "$test_name 1" >> - } >> - -re ".*main. .., function, $hex.*$gdb_prompt $" { >> - pass "$test_name 2" >> - } >> - -re ".*$gdb_prompt $" { fail "$test_name" } >> - timeout { fail "$test_name (timeout)" } >> - } >> - gdb_test "shell rm -f $psymbols_output" ".*" \ >> - "${test_name}: shell rm -f psymbols_output" >> + gdb_test_multiple "$command" "$test_name" { >> + -re -wrap "^maint print psymbols \[^\n\]*" { >> + gdb_test_multiple "shell grep 'main.*function' $psymbols_output" \ >> + "$test_name internal" { >> + -re -wrap ".main., function, $hex.*" { >> + pass "$test_name (pattern 1)" >> + } >> + -re -wrap ".*main. .., function, $hex.*" { >> + pass "$test_name (pattern 2)" >> } >> - -re ".*$gdb_prompt $" { fail "$test_name" } >> - timeout { fail "$test_name (timeout)" } >> - } >> } >> - -re ".*$gdb_prompt $" { fail "$test_name" } >> - timeout { fail "$test_name (timeout)" } >> } >> + } >> + gdb_test "shell rm -f $psymbols_output" ".*" \ >> + "${test_name}: shell rm -f psymbols_output" >> } > > Again, apologies for only just spotting this, but I think this test can > be further simplified to just: > > foreach { test_name command } $test_list { > with_test_prefix "$test_name" { > gdb_test_no_output "$command" "collect output" > gdb_test_multiple "shell grep 'main.*function' $psymbols_output" \ > "$analyse output" { > -re -wrap ".main., function, $hex.*" { > pass "$gdb_test_name (pattern 1)" > } > -re -wrap ".*main. .., function, $hex.*" { > pass "$gdb_test_name (pattern 2)" > } > } > gdb_test "shell rm -f $psymbols_output" ".*" \ > "shell rm -f psymbols_output" > } > } > > I think we already made a similar change later on in this script, so > this just brings this test into line with the later ones. Alright, makes sense too. v4 coming soon! Cheers! Bruno Larsen > > Thanks, > Andrew > >> } >> >> >> set msymbols_output [standard_output_file msymbols_output] >> set msymbols_output_re [string_to_regexp $msymbols_output] >> -send_gdb "maint print msymbols -objfile ${binfile} $msymbols_output\n" >> -gdb_expect { >> - -re "^maint print msymbols \[^\n\]*\r\n$gdb_prompt $" { >> - send_gdb "shell ls $msymbols_output\n" >> - gdb_expect { >> - -re "$msymbols_output_re\r\n$gdb_prompt $" { >> - gdb_test "shell grep factorial $msymbols_output" \ >> - "\\\[ *$decimal\\\] \[tT\]\[ \t\]+$hex \\.?factorial.*" \ >> - "maint print msymbols, absolute pathname" >> - gdb_test "shell rm -f $msymbols_output" ".*" \ >> - "shell rm -f msymbols_output" >> - } >> - -re ".*$gdb_prompt $" { fail "maint print msymbols" } >> - timeout { fail "maint print msymbols (timeout)" } >> - } >> - } >> - -re ".*$gdb_prompt $" { fail "maint print msymbols" } >> - timeout { fail "maint print msymbols (timeout)" } >> -} >> +gdb_test_no_output "maint print msymbols -objfile ${binfile} $msymbols_output" \ >> + "print msymbols to file, with absolute path" >> +gdb_test "shell grep factorial $msymbols_output" \ >> + "\\\[ *$decimal\\\] \[tT\]\[ \t\]+$hex \\.?factorial.*" \ >> + "maint print msymbols, absolute pathname" >> +gdb_test "shell rm -f $msymbols_output" ".*" "remove absolute path msymbols" >> >> # Check that maint print msymbols allows relative pathnames >> set mydir [pwd] >> @@ -336,18 +304,13 @@ gdb_test "cd [standard_output_file {}]" \ >> "Working directory .*\..*" \ >> "cd to objdir" >> >> -gdb_test_multiple "maint print msymbols -objfile ${testfile} msymbols_output2" "maint print msymbols" { >> - -re "^maint print msymbols \[^\n\]*\r\n$gdb_prompt $" { >> - gdb_test_multiple "shell ls msymbols_output2" "maint print msymbols" { >> - -re "msymbols_output2\r\n$gdb_prompt $" { >> - gdb_test "shell grep factorial msymbols_output2" \ >> - "\\\[ *$decimal\\\] \[tT\]\[ \t\]+$hex \\.?factorial.*" \ >> - "maint print msymbols, relative pathname" >> - gdb_test "shell rm -f msymbols_output2" ".*" >> - } >> - } >> - } >> -} >> +gdb_test_no_output "maint print msymbols -objfile ${testfile} $msymbols_output"\ >> + "print msymbols to file, with relative path" >> +gdb_test "shell grep factorial $msymbols_output" \ >> + "\\\[ *$decimal\\\] \[tT\]\[ \t\]+$hex \\.?factorial.*" \ >> + "maint print msymbols, relative pathname" >> +gdb_test "shell rm -f msymbols_output" ".*" "remove relative path msymbols" >> + >> gdb_test "cd ${mydir}" \ >> "Working directory [string_to_regexp ${mydir}]\..*" \ >> "cd to mydir" >> @@ -365,31 +328,12 @@ set test_list [list \ >> "maint print symbols -pc" \ >> "maint print symbols -pc main $symbols_output"] >> foreach { test_name command } $test_list { >> - send_gdb "$command\n" >> - gdb_expect { >> - -re "^maint print symbols \[^\n\]*\r\n$gdb_prompt $" { >> - send_gdb "shell ls $symbols_output\n" >> - gdb_expect { >> - -re "$symbols_output_re\r\n$gdb_prompt $" { >> - # See comments for `maint print psymbols'. >> - send_gdb "shell grep 'main(.*block' $symbols_output\n" >> - gdb_expect { >> - -re "int main\\(int, char \\*\\*, char \\*\\*\\); block.*$gdb_prompt $" { >> - pass "$test_name" >> - } >> - -re ".*$gdb_prompt $" { fail "$test_name" } >> - timeout { fail "$test_name (timeout)" } >> - } >> - gdb_test "shell rm -f $symbols_output" ".*" \ >> - "$test_name: shell rm -f symbols_output" >> - } >> - -re ".*$gdb_prompt $" { fail "$test_name" } >> - timeout { fail "$test_name (timeout)" } >> - } >> - } >> - -re ".*$gdb_prompt $" { fail "$test_name" } >> - timeout { fail "$test_name (timeout)" } >> - } >> + gdb_test_no_output "$command" "$test_name generate" >> + gdb_test "shell grep 'main(.*block' $symbols_output"\ >> + "int main\\(int, char \\*\\*, char \\*\\*\\); block.*"\ >> + "$test_name read" >> + gdb_test "shell rm -f $symbols_output" ".*" \ >> + "$test_name: shell rm -f symbols_output" >> } >> >> set msg "maint print type" >> -- >> 2.31.1 >