From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 90406 invoked by alias); 22 Jan 2020 17:35:35 -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 90391 invoked by uid 89); 22 Jan 2020 17:35:33 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-23.2 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.1 spammy= X-HELO: mail-qk1-f193.google.com Received: from mail-qk1-f193.google.com (HELO mail-qk1-f193.google.com) (209.85.222.193) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 22 Jan 2020 17:35:32 +0000 Received: by mail-qk1-f193.google.com with SMTP id x1so469060qkl.12 for ; Wed, 22 Jan 2020 09:35:31 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=subject:to:cc:references:from:message-id:date:user-agent :mime-version:in-reply-to:content-language:content-transfer-encoding; bh=kNzrOebTvXxkxqENke1QFgThqSv6g5rvtdRGmXnxwKw=; b=ydkvlDw+vrr6Pkht3Wl37fD+kj4GHK3OtjtAgeHhuxuR3xtd247tPoyytAPF8dhUI0 tJ9Ck2czazXPl3sHdMgGI+d6ExiXPvgqEt8hEEDh1TzWs9PIeSTeL2wuN1FksHmLDq0o 5qGLXf4b6pJcPiUr9Q04wG5LxrQz7eMAY94cG/ZCDhwmApxU+JKXet3CixM3X4Nv8nl5 EOElHjIc/9Rz1jaL7y8JpNXK4FuHOFgYu7hlrOh5NyJz0vJpjUUqNyr3UZXTSbn/WTkC UbKfux07a8sOCIMIfjohognIPL5bPgH6z24nV5bzHsorZrRs9Vccgf02j8txRtkHogNL nejg== Return-Path: Received: from [192.168.0.185] ([177.158.84.16]) by smtp.gmail.com with ESMTPSA id r66sm19166868qkd.125.2020.01.22.09.35.27 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 22 Jan 2020 09:35:29 -0800 (PST) Subject: Re: [PATCH] Harden gdb.base/step-over-syscall.exp To: Simon Marchi , gdb-patches@sourceware.org Cc: alan.hayward@arm.com References: <20200115203645.26360-1-luis.machado@linaro.org> <66fc6535-755d-ffae-627b-fd8925294fb6@simark.ca> From: Luis Machado Message-ID: <37cdaf65-f7d5-107d-015a-3c65452bc03b@linaro.org> Date: Wed, 22 Jan 2020 17:48:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.4.1 MIME-Version: 1.0 In-Reply-To: <66fc6535-755d-ffae-627b-fd8925294fb6@simark.ca> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2020-01/txt/msg00684.txt.bz2 On 1/22/20 11:45 AM, Simon Marchi wrote: > On 2020-01-15 3:36 p.m., Luis Machado wrote: >> diff --git a/gdb/testsuite/gdb.base/step-over-syscall.exp b/gdb/testsuite/gdb.base/step-over-syscall.exp >> index b373c169c0..4d9488b1d4 100644 >> --- a/gdb/testsuite/gdb.base/step-over-syscall.exp >> +++ b/gdb/testsuite/gdb.base/step-over-syscall.exp >> @@ -46,7 +46,8 @@ proc_with_prefix check_pc_after_cross_syscall { syscall syscall_insn_next_addr } >> >> proc setup { syscall } { >> global gdb_prompt syscall_insn >> - >> + global hex >> + set next_insn_addr 0 > > I would suggest using -1 as the initial value, as 0 is (in theory) a valid address. > Thanks. Fixed this as well as the other occurrences. >> set testfile "step-over-$syscall" >> >> clean_restart $testfile >> @@ -62,7 +63,7 @@ proc setup { syscall } { >> gdb_test_no_output "set displaced-stepping off" \ >> "set displaced-stepping off during test setup" >> >> - gdb_test "break $syscall" "Breakpoint \[0-9\]* at .*" >> + gdb_test "break \*$syscall" "Breakpoint \[0-9\]* at .*" >> >> gdb_test "continue" "Continuing\\..*Breakpoint \[0-9\]+, (.* in |__libc_|)$syscall \\(\\).*" \ >> "continue to $syscall (1st time)" >> @@ -75,37 +76,72 @@ proc setup { syscall } { >> # Hit the breakpoint on $syscall for the second time. In this time, >> # the address of syscall insn and next insn of syscall are recorded. >> >> - gdb_test "display/i \$pc" ".*" >> - >> - # Single step until we see a syscall insn or we reach the >> - # upper bound of loop iterations. >> - set msg "find syscall insn in $syscall" >> - set steps 0 >> - set max_steps 1000 >> - gdb_test_multiple "stepi" $msg { >> - -re ".*$syscall_insn.*$gdb_prompt $" { >> - pass $msg >> + # Check if the first instruction we stopped at is the syscall one. >> + set syscall_insn_addr 0 >> + set test "fetch first stop pc" >> + gdb_test_multiple "display/i \$pc" $test { >> + -re "display/i .*: x/i .*=> ($hex) .*:.*$syscall_insn.*$gdb_prompt $" { >> + set syscall_insn_addr $expect_out(1,string) >> + pass $test >> } >> - -re "x/i .*=>.*\r\n$gdb_prompt $" { >> - incr steps >> - if {$steps == $max_steps} { >> - fail $msg >> - } else { >> - send_gdb "stepi\n" >> - exp_continue >> + -re "display/i.*" { >> + pass $test >> + } > > This probably fails with "make check-read1". If the characters come in one > by one, you'll get eventually get "display/i" in the buffer, which will match > the second regexp. > True. Let me think of a better way to handle this particular case. >> + } >> + >> + # If we are not at the syscall instruction yet, keep looking for it with >> + # stepi commands. >> + if {$syscall_insn_addr == 0} { >> + # Single step until we see a syscall insn or we reach the >> + # upper bound of loop iterations. >> + set msg "find syscall insn in $syscall" >> + set steps 0 >> + set max_steps 1000 >> + gdb_test_multiple "stepi" $msg { >> + -re ".*$syscall_insn.*$gdb_prompt $" { >> + pass $msg >> + } >> + -re "x/i .*=>.*\r\n$gdb_prompt $" { >> + incr steps >> + if {$steps == $max_steps} { >> + fail $msg >> + } else { >> + send_gdb "stepi\n" >> + exp_continue >> + } >> } >> } > > Maybe I'm worrying too much, but another way this could fail (or actually fail to catch > a failure) is if the regexp misses that syscall instruction, but catches another syscall > later, at some point during the 1000 stepi. Would it be good to verify that we are at the > syscall we expect, by by checking the syscall number? That would require knowing the > register name that holds the syscall number, and the expected syscall numbers for fork, > vfork and exec, for each architecture. Those things don't change over time, and we already > have an architecture-specific definition ($syscall_insn), so I don't think it would be > problematic to hardcode them in the test too. > I'll give this a try while at it. >> + >> + if {$steps == $max_steps} { >> + return { -1, -1 } >> + } >> + >> + set syscall_insn_addr [get_hexadecimal_valueof "\$pc" "0" \ >> + "pc before stepi"] >> } >> >> - if {$steps == $max_steps} { >> - return { -1, -1 } >> + # We have found the syscall instruction. Now record the next instruction. >> + # Use the X command instead of stepi since we can't guarantee >> + # stepi is working properly. >> + set test "pc after syscall instruction" >> + gdb_test_multiple "x/2i \$pc" $test { >> + -re "x/2i .*=> $hex .*:.*$syscall_insn.* ($hex) .*:.*$gdb_prompt $" { >> + set next_insn_addr $expect_out(2,string) >> + pass $test >> + } > > For consistency, you might as well get the syscall instruction address from there too. > Done. >> } >> >> - set syscall_insn_addr [get_hexadecimal_valueof "\$pc" "0" \ >> - "pc before stepi"] >> if {[gdb_test "stepi" "x/i .*=>.*" "stepi $syscall insn"] != 0} { >> return { -1, -1 } >> } >> + >> + set pc_after_stepi [get_hexadecimal_valueof "\$pc" "0" \ >> + "pc after stepi with x command"] >> + >> + if {$next_insn_addr != $pc_after_stepi} { >> + fail "pc after stepi matches insn addr after syscall" >> + } > > Use gdb_assert, so that we get a PASS if it works. > > gdb_assert {$next_insn_addr == $pc_after_stepi} \ > "pc after stepi matches insn addr after syscall" > Fixed now. Thanks!