From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 111899 invoked by alias); 19 Feb 2016 15:18:26 -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 111885 invoked by uid 89); 19 Feb 2016 15:18:26 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.6 required=5.0 tests=AWL,BAYES_00,RP_MATCHES_RCVD,SPF_PASS autolearn=ham version=3.3.2 spammy=*arm* X-HELO: xyzzy.0x04.net Received: from xyzzy.0x04.net (HELO xyzzy.0x04.net) (109.74.193.254) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 19 Feb 2016 15:18:24 +0000 Received: from hogfather.0x04.net (89-65-66-135.dynamic.chello.pl [89.65.66.135]) by xyzzy.0x04.net (Postfix) with ESMTPS id C45B23FED6; Fri, 19 Feb 2016 16:19:13 +0100 (CET) Received: from [192.168.13.108] (89-65-66-135.dynamic.chello.pl [89.65.66.135]) by hogfather.0x04.net (Postfix) with ESMTPSA id 1E507580088; Fri, 19 Feb 2016 16:18:21 +0100 (CET) Subject: Re: [PATCH] gdb.trace: Move more target dependencies to trace-support.exp To: Antoine Tremblay References: <1455884629-23354-1-git-send-email-koriakin@0x04.net> Cc: gdb-patches@sourceware.org From: =?UTF-8?Q?Marcin_Ko=c5=9bcielnicki?= Message-ID: <56C7323C.4050707@0x04.net> Date: Fri, 19 Feb 2016 15:18:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.5.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: 2016-02/txt/msg00603.txt.bz2 On 19/02/16 16:05, Antoine Tremblay wrote: > > Marcin Kościelnicki writes: > >> While groveling through the old PPC64 tracepoint support patch, I've >> noticed a few target dependencies in the testsuite that both me and >> Antoine missed for s390 and ARM tracepoints, respectively. This patch >> moves them all to one place, so that anyone working on a new target >> will hopefully see the whole set of needed changes. > > Thanks for this, it seems I indeed had missed a few, it also fixes > another issue in the arm tracepoints were I was using is_aarch32_target > rather then istarget[*arm*]. > > It's much more clear like that. > >> >> I've also removed a check for fast tracepoint support in ftrace.exp - >> it used hardcoded targets and wasn't doing anything useful anyway, >> since unsupported architectures blow up on link due to missing >> the IPA library before they ever get to that check. >> > > Maybe this should be another patch ? , Obvious ? Very well, I'll cut it up. > >> For some strange reason, the call_insn setting code already knew about >> arm, powerpc, s390, and mips - I went ahead and added the remaining >> information about those. I'm not particularly sure if I got mips right, >> but that won't matter anyway until someone actually writes tracepoint >> support for that. >> >> In addition, the PPC64 tracepoint patch added \y at the end of the call_insn >> pattern - without that, it embarassed itself and matched the 'bl' in >> "Dump of assem*bl*er code for function" as the powerpc call opcode. Since >> that sounds like a generally good idea, I've added \y before and after >> call_insn for every target. As a result, I had to change x86_64's mnemonic >> to 'callq'. >> > > Also this could be another patch ? Likewise. > >> Tested on x86_64, i386, ppc, ppc64, ppc64le, s390, s390x. Would be good >> to test it on aarch64. >> > > >> diff --git a/gdb/testsuite/lib/trace-support.exp b/gdb/testsuite/lib/trace-support.exp >> index f593c43..829786b 100644 >> --- a/gdb/testsuite/lib/trace-support.exp >> +++ b/gdb/testsuite/lib/trace-support.exp >> @@ -20,26 +20,99 @@ >> >> >> # >> -# Program counter / stack pointer / frame pointer for supported targets. >> -# Used in many tests, kept here to avoid duplication. >> +# Target-specific information. Used in many tests, kept here >> +# to avoid duplication and make it easier to add a new target. >> # >> >> if [is_amd64_regs_target] { >> + # Frame pointer. >> set fpreg "rbp" >> + # Stack pointer. >> set spreg "rsp" >> + # Program counter. >> set pcreg "rip" >> + # How to collect the first argument to a function. Used to test >> + # register usage in tracepoint conditions. >> + set arg0exp "\$rdi" >> + # The mnemonic of the usual, unconditional call instruction. >> + set call_insn "callq" >> + # Number of the PC register. >> + set pcnum 16 >> + # Number of any GPR (it's supposed not to be some register that's not >> + # collected by default). >> + set gpr0num 0 >> } elseif [is_x86_like_target] { >> set fpreg "ebp" >> set spreg "esp" >> set pcreg "eip" >> + set arg0exp "*(int *) (\$ebp + 8)" >> + set call_insn "call" >> + set pcnum 8 >> + set gpr0num 0 >> } elseif [is_aarch64_target] { >> set fpreg "x29" >> set spreg "sp" >> set pcreg "pc" >> + set arg0exp "\$x0" >> + set call_insn "bl" >> + set pcnum 32 >> + set gpr0num 0 >> +} elseif [istarget "arm*-*-*"] { >> + set fpreg "r11" > > There is no fp as far as I can tell looking at arm aapcs maybe we can > use sp here ? It's just to collect a register that has a value in the > tests. There isn't really a fp according to s390 ABI either, but r11 is what gcc uses when it uses a frame pointer (coincidentally for both s390 and arm). I figured I'd play it safe, but might not be necessary. > >> + set spreg "r13" > > Need to use sp here, as r13 is not defined in arm-core.xml, using r13 > causes a 'r13' is a user-register; GDB cannot yet trace user-register > contents. error when collecting. > >> + set pcreg "r15" > > Use pc here for the same reasons as above. Whoops, thanks for noticing that. > >> + set arg0exp "\$r0" >> + set call_insn "bl" >> + set pcnum 15 >> + set gpr0num 0 > > Also in general since arm tracepoints are not there yet, this should be > part of my tracepoint patch I think, so you can ommit it here and I will > add it. Alright, I'll drop that part and leave it to you. > >> +} elseif [istarget "powerpc*-*-*"] { >> + set fpreg "r31" >> + set spreg "r1" >> + set pcreg "pc" >> + set arg0exp "\$r3" >> + set call_insn "bl" >> + set pcnum 64 >> + set gpr0num 0 >> +} elseif [istarget "s390*-*-*"] { >> + set fpreg "r11" >> + set spreg "r15" >> + set pcreg "pc" >> + set arg0exp "\$r2" >> + set call_insn "brasl" >> + # Strictly speaking, this is PSWA, not PC. >> + set pcnum 1 >> + set gpr0num 2 >> +} elseif { [istarget "mips*-*-*"] } { >> + set fpreg "s8" >> + set spreg "sp" >> + set pcreg "pc" >> + set arg0exp "\$a0" >> + # Skip the delay slot after the instruction used to make a call >> + # (which can be a jump or a branch) if it has one. >> + # >> + # JUMP (or BRANCH) foo >> + # insn1 >> + # insn2 >> + # >> + # Most MIPS instructions used to make calls have a delay slot. >> + # These include JAL, JALS, JALX, JALR, JALRS, BAL and BALS. >> + # In this case the program continues from `insn2' when `foo' >> + # returns. The only exception is JALRC, in which case execution >> + # resumes from `insn1' instead. >> + set call_insn {jalrc|[jb]al[sxr]*[ \t][^\r\n]+\r\n} >> + set pcnum 37 >> + set gpr0num 0 >> } else { >> + # Defaults. Probably won't work, but we don't want to error out >> + # here on unsupported platforms, since this file is imported to check >> + # for supported platforms in the first place. >> set fpreg "fp" >> set spreg "sp" >> set pcreg "pc" >> + set arg0exp "\$a0" >> + set call_insn "call" >> + set pcnum -1 >> + set gpr0num -1 >> } >> >> # > > Otherwise I've tested this on arm with the sp/fp/pc changes and the > tracepoint patch , and it passes tests except for collecting some > registers locals but I think this is unrelated and will investigate. > > Thanks, > Antoine >