From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 112892 invoked by alias); 22 Jan 2020 15:20:31 -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 112846 invoked by uid 89); 22 Jan 2020 15:20:29 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-24.7 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=HX-HELO:sk:mail-qk X-HELO: mail-qk1-f196.google.com Received: from mail-qk1-f196.google.com (HELO mail-qk1-f196.google.com) (209.85.222.196) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 22 Jan 2020 15:20:27 +0000 Received: by mail-qk1-f196.google.com with SMTP id q15so12803qke.9 for ; Wed, 22 Jan 2020 07:20:26 -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=2Fqi/uhuZQRdNlgHzxuZS3QDOGcYyoq9Y/JFfrpXmT0=; b=O+P1ohtFym/Mn0GdnAF3cLRAUeLjENZBJuXTIRO/Eh4ZGRHLmwoc/vP8nLGE31MzgZ /swJFUs8OUSMMDFWr/IAohUGMCIVLkm2aagjYSYg6xcQYIaHwbUA3xZm/lLqw9ul2rJf rILRY8ILTt7WdM/oASgQkF5KuL94wzApcxrEakSL4QBPsykFr7W9cYgwEjTwP2L0tb+W jBSQ2+2Yjvf/75Isd5onwiJYQh5AWzqHe5dFLZyhFvAjQPqh3MSDOLgFzRM8AJzC63AX 06kqNzJSDGsFtyHjkt+aDxXHvGzThTWNpw7qLzXTy1Am3zMnqS6oLGwneFviCCJm2pa3 WLkw== Return-Path: Received: from [192.168.0.185] ([177.158.84.16]) by smtp.gmail.com with ESMTPSA id x6sm18786488qkh.20.2020.01.22.07.20.22 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 22 Jan 2020 07:20:24 -0800 (PST) Subject: Re: [PING] [PATCH] Harden gdb.base/step-over-syscall.exp To: Alan Hayward Cc: "gdb-patches\\@sourceware.org" , nd References: <20200115203645.26360-1-luis.machado@linaro.org> From: Luis Machado Message-ID: <870ad1a6-c679-56c3-ea31-a7df80a048ce@linaro.org> Date: Wed, 22 Jan 2020 15:49: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: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit X-IsSubscribed: yes X-SW-Source: 2020-01/txt/msg00678.txt.bz2 On 1/22/20 11:26 AM, Alan Hayward wrote: > > >> On 22 Jan 2020, at 13:30, Luis Machado wrote: >> >> ping? >> >> https://sourceware.org/ml/gdb-patches/2020-01/msg00432.html >> >> On 1/15/20 5:36 PM, Luis Machado wrote: >>> There are a couple problems with this test. >>> First >>> -- >>> gdb.base/step-over-syscall.exp records the address of a syscall instruction >>> within fork/vfork/clone functions and also the address of the instruction >>> after that syscall instruction. >>> It uses these couples addresses to make sure we stepped over a syscall >>> instruction (fork/vfork/clone events) correctly. >>> The way the test fetches the addresses of the instructions is by stepi-ing >>> its way through the fork/vfork/clone functions until it finds a match for >>> a syscall. Then it stepi's once again to get the address of the next >>> instruction. >>> This assumes that stepi-ing over a syscall is working correctly and landing >>> in the right PC. This is not the case for AArch64/Linux, where we're >>> landing a couple instructions after the syscall in some cases. >>> The following patch lets the test execute as before, but adds a new instruction >>> address check using the x command as opposed to stepi. >>> I didn't want to change how the test works since we may also be >>> interested in checking if stepi-ing over the syscall under different >>> conditions (displaced stepping on/off) yields the same results. I don't >>> feel strongly about this, so i'm OK with changing how we compare PC's for >>> the entire test if folks decide it is reasonable. > > I’m happy with leaving it this way too. > >>> Second >>> -- >>> FAIL: gdb.base/step-over-syscall.exp: vfork: displaced=off: continue to vfork (3rd time) (the program exited) >>> FAIL: gdb.base/step-over-syscall.exp: vfork: displaced=off: continue to syscall insn vfork (the program is no longer running) >>> FAIL: gdb.base/step-over-syscall.exp: vfork: displaced=off: single step over vfork (the program is no longer running) >>> Depending on the glibc version we may have different code generated for the >>> fork/vfork/clone functions. >>> I ran into the situation where vfork for newer glibc's on AArch64/Linux is >>> very short, so "break vfork" will put a breakpoint right at the syscall >>> instruction, which is something the testcase isn't expecting (a off-by-1 >>> of sorts). >>> The patch adds extra code to handle this case. If the test detects we're >>> already sitting at a syscall instruction, it records the address and moves >>> on to record the address after that particular instruction. >>> Another measure is to "break *$syscall" instead of "break $syscall". That >>> guarantees we're stopping at the first instruction of the syscall function, >>> if it ever happens that the syscall instruction is the first instruction of >>> those functions. >>> With these changes i can fix some failures for aarch64-linux-gnu and also >>> expose the problems i've reported here: >>> https://sourceware.org/ml/gdb-patches/2019-12/msg01071.html >>> These tests now fail for aarch64-linux-gnu (patch for this is going through >>> reviews): >>> FAIL: gdb.base/step-over-syscall.exp: vfork: displaced=off: pc after stepi matches insn addr after syscall >>> FAIL: gdb.base/step-over-syscall.exp: vfork: displaced=on: pc after stepi matches insn addr after syscall >>> I've queued a test run on the buildbot to make sure things are sane for other >>> architectures, and i've tested it on aarch64-linux-gnu. >>> gdb/testsuite/ChangeLog: >>> 2020-01-15 Luis Machado >>> * gdb.base/step-over-syscall.exp (setup): Check if we're already >>> sitting at a syscall instruction when we hit the syscall function's >>> breakpoint. >>> Check PC against one obtained with the x command. >>> (step_over_syscall): Don't continue to the syscall instruction if >>> we're already there. >>> --- >>> gdb/testsuite/gdb.base/step-over-syscall.exp | 91 ++++++++++++++------ >>> 1 file changed, 66 insertions(+), 25 deletions(-) >>> 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 >>> 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 { > > I don’t like the use of display/i here because it will be displaying the pc after > every command, so could break other tests. Alternatively, you could stepi then x/i $pc. > However, the test already worked this way, so I’m ok with it. > >>> + -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 >>> + } >>> + } >>> + >>> + # 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 >>> + } >>> } >>> } >>> + >>> + 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 >>> + } >>> } >>> - 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" >>> + } >>> + >>> return [list $syscall_insn_addr [get_hexadecimal_valueof "\$pc" \ >>> "0" "pc after stepi"]] > > Minor nit. In the return, you could just use $next_insn_addr instead of > calling get_hexadecimal_valueof again. > Do you mean $pc_after_stepi as opposed to $next_insn_addr? $pc_after_stepi is what we get from the $pc register. $next_insn_addr is what we get from using the X command. >>> } >>> @@ -156,8 +192,13 @@ proc step_over_syscall { syscall } { >>> } >>> } >>> - gdb_test "continue" "Continuing\\..*Breakpoint \[0-9\]+, .*" \ >>> - "continue to syscall insn $syscall" >>> + # Check if the syscall breakpoint is at the syscall instruction >>> + # address. If so, no need to continue, otherwise we will run the >>> + # inferior to completion. >>> + if {$syscall_insn_addr != [get_hexadecimal_valueof "\$pc" "0"]} { >>> + gdb_test "continue" "Continuing\\..*Breakpoint \[0-9\]+, .*" \ >>> + "continue to syscall insn $syscall" >>> + } >>> gdb_test_no_output "set displaced-stepping $displaced" >>> >