From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 28709 invoked by alias); 14 Dec 2012 18:47:28 -0000 Received: (qmail 28690 invoked by uid 22791); 14 Dec 2012 18:47:25 -0000 X-SWARE-Spam-Status: No, hits=-7.6 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,KHOP_THREADED,RCVD_IN_DNSWL_HI,RP_MATCHES_RCVD,SPF_HELO_PASS,TW_EG X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 14 Dec 2012 18:47:13 +0000 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id qBEIlCuK005137 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Fri, 14 Dec 2012 13:47:12 -0500 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id qBEIlAhq020127; Fri, 14 Dec 2012 13:47:11 -0500 Message-ID: <50CB742E.9090506@redhat.com> Date: Fri, 14 Dec 2012 18:47:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/17.0 Thunderbird/17.0 MIME-Version: 1.0 To: Kaushik Phatak CC: "gdb-patches@sourceware.org" Subject: Re: [RFA 4/5] New port: CR16: gdbserver References: In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit 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 X-SW-Source: 2012-12/txt/msg00513.txt.bz2 On 10/04/2012 11:23 AM, Kaushik Phatak wrote: > + > +static void > +cr16_collect_ptrace_register (struct regcache *regcache, int regno, char *buf) > +{ > + unsigned long pc; > + > + memset (buf, 0, sizeof (long)); > + collect_register_by_name (regcache, "pc", &pc); > + if(regno == PC_REGNUM) > + { > + pc = pc >> 1; > + cr16_set_pc(regcache, pc); > + } > + collect_register (regcache, regno, buf); > +} Formatting is wrong here. > + > +static void > +cr16_supply_ptrace_register (struct regcache *regcache, > + int regno, const char *buf) > +{ > + unsigned long pc; > + supply_register (regcache, regno, buf ); > + collect_register_by_name (regcache, "pc", &pc); > + > + /* For PC, leftshift the output as only top 21 bits are stored > + This will make the value human readable for the host */ > + if(regno == PC_REGNUM) > + { > + pc = pc << 1; > + cr16_set_pc(regcache, pc); > + } Here too. But what really caught my eye was the shifts and the cr16_set_pc calls. They surprised me. I've been staring at this for a good 10 minutes, and I can't get my head around it. It doesn't look like these functions are idempotent, which is a sign of things not being right. Is the shift visible in GDB or not? Is this a ptrace quirk, or an architecture quirk? If the latter, why isn't GDB itself, and the cr16_set_pc cr16_get_pc routines in gdbserver handling this? > + (*the_target->read_memory) (where, (unsigned char *) &insn, > + cr16_breakpoint_len); Most ports fail to do this, but the above may fail. That should be checked here. > + if (insn == cr16_breakpoint) > + { > + return 1; > + } Single-line statements get no {}'. > +struct linux_target_ops the_low_target = { '{' goes on start of next line. Several other places in the patch with that issue. > +++ ./gdb-7.5_working/gdb/gdbserver/linux-low.c 2012-10-01 16:21:48.000000000 +0530 > @@ -4809,7 +4810,7 @@ linux_stopped_data_address (void) > #if ! (defined(PT_TEXT_ADDR) \ > || defined(PT_DATA_ADDR) \ > || defined(PT_TEXT_END_ADDR)) > -#if defined(__mcoldfire__) > +#if defined(__mcoldfire__) || (__CR16__) > /* These should really be defined in the kernel's ptrace.h header. */ > #define PT_TEXT_ADDR 49*4 > #define PT_DATA_ADDR 50*4 Please rebase against GDB mainline. These should be defined in instead, and linux-low.c should be picking those up. > diff -uprN gdb-7.5/gdb/regformats/reg-cr16.dat ./gdb-7.5_working/gdb/regformats/reg-cr16.dat > --- gdb-7.5/gdb/regformats/reg-cr16.dat 1970-01-01 05:30:00.000000000 +0530 > +++ ./gdb-7.5_working/gdb/regformats/reg-cr16.dat 2012-09-13 14:45:02.000000000 +0530 > @@ -0,0 +1,18 @@ > +name:cr16 > +expedite:psr Why only psr? That's surprising. > +32:r0and1 > +32:r2and3 > +32:r4and5 > +32:r6and7 > +32:r8and9 > +32:r10and11 Eh. What's the rationale for this? Peeking at the GDB patch, I saw no pseudo registers support. -- Pedro Alves