From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 17144 invoked by alias); 25 Jun 2008 12:32:21 -0000 Received: (qmail 17135 invoked by uid 22791); 25 Jun 2008 12:32:20 -0000 X-Spam-Check-By: sourceware.org Received: from rock.gnat.com (HELO rock.gnat.com) (205.232.38.15) by sourceware.org (qpsmtpd/0.31) with ESMTP; Wed, 25 Jun 2008 12:31:47 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id 682722A97BE; Wed, 25 Jun 2008 08:31:45 -0400 (EDT) Received: from rock.gnat.com ([127.0.0.1]) by localhost (rock.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id jt+0voUBztBv; Wed, 25 Jun 2008 08:31:45 -0400 (EDT) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id F05292A97B2; Wed, 25 Jun 2008 08:31:44 -0400 (EDT) Received: by joel.gnat.com (Postfix, from userid 1000) id 917A7E7ACD; Wed, 25 Jun 2008 08:31:44 -0400 (EDT) Date: Wed, 25 Jun 2008 12:54:00 -0000 From: Joel Brobecker To: Luis Machado Cc: Pedro Alves , Daniel Jacobowitz , gdb-patches@sourceware.org Subject: Re: [PATCH] PPC - Stepping off breakpoints in non-stop mode Message-ID: <20080625123144.GA3700@adacore.com> References: <1209753019.7131.29.camel@gargoyle> <20080605200249.GH25085@caradoc.them.org> <1212768014.10042.60.camel@gargoyle> <200806241503.59231.pedro@codesourcery.com> <1214317934.10496.18.camel@gargoyle> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1214317934.10496.18.camel@gargoyle> User-Agent: Mutt/1.4.2.2i 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: 2008-06/txt/msg00424.txt.bz2 Hi Luis, > 2008-06-24 Luis Machado > > * ppc-tdep.h: Define PPC_MAX_INSN_LEN, BRANCH_MASK, B_INSN, BC_INSN, > LWARX_MASK, LWARX_INSTRUCTION, LDARX_INSTRUCTION, STWCX_MASK, > STWCX_INSTRUCTION, STDCX_INSTRUCTION, BXL_INSN, BP_MASK and BP_INSN. > * rs6000-tdep.c (ppc_displaced_step_fixup): New function. > (deal_with_atomic_sequence): Update BC masks. > (rs6000_gdbarch_init): Init displaced stepping infra-structure. > Remove LWARX_MASK, LWARX_INSTRUCTION, LDARX_INSTRUCTION, STWCX_MASK, > STWCX_INSTRUCTION, STDCX_INSTRUCTION, BC_MASK and BC_INSTRUCTION. Overall, this looks good to me, I just have a few questions before we commit. Thanks much to Pedro for his informal review, btw. > +/* The length of the longest ppc instruction. */ > +#define PPC_MAX_INSN_LEN (4) > /* Instruction size. */ > #define PPC_INSN_SIZE 4 Can we ditch the PPC_MAX_INSN_LEN macro and only use PPC_INSN_SIZE? Right now, you define the latter but never use it. Right now, you only use PPC_MAX_INSN_LEN, which I like less than PPC_INSN_SIZE, because AFAIK all instructions on powerpc are the same size. > +/* Instruction masks used during single-stepping of atomic sequences. */ > +#define LWARX_MASK 0xfc0007fe > +#define LWARX_INSTRUCTION 0x7c000028 > +#define LDARX_INSTRUCTION 0x7c0000A8 > +#define STWCX_MASK 0xfc0007ff > +#define STWCX_INSTRUCTION 0x7c00012d > +#define STDCX_INSTRUCTION 0x7c0001ad Why did you move these macros to the .h file? Right now, they are only used inside rs6000-tdep.c, so we could keep them there. > +/* Instruction masks for displaced stepping. */ > +#define BRANCH_MASK 0xfc000000 > +#define BP_MASK 0xFC0007FE > +#define B_INSN 0x48000000 > +#define BC_INSN 0x40000000 > +#define BXL_INSN 0x4c000000 > +#define BP_INSN 0x7C000008 Same for these macros, we could certainly keep them inside rs6000-tdep.c. I'm not opposed to having them in the .h file, if you prefer it that way. But I generally prefer to keep definitions inside the .c file if they are not used elsewhere. That way, I instantly know that this macro is only used within that unit. > + ULONGEST opcode = 0; > + LONGEST offset = PPC_MAX_INSN_LEN; /* Default offset for non PC-relative instructions. */ The comment is a little bit too long. I know it would have been nice to keep the comment on the same line in this case, but I think you'll have to put it above. You'll problably want to reword it a bit to better fit with the new order (comment-code vs code-comment). For instance: /* The default offset for non PC-relative instructions. */ LONGEST offset = PPC_INSN_SIZE; > + /* LK bit Indicates whether we should set the link register to point > + to the next instruction or not. */ > + gdb_byte link_register_bit = (gdb_byte) (insn & 0x1); I don't think you need to use a gdb_byte in this case. Wouldn't it be simpler to use an int? Hopefully, that will allow you to avoid this cast. Otherwise, it looks like this constant is only used once in your code: if (link_register_bit) So another alternative is is to embed it inside the "if" statement with a comment. Something like this: if (insn & 0x1) { /* The LK bit is set. It indicates that we should set the link register to point to the next instruction. */ > + /* AA bit indicating whether this is an absolute addressing or > + PC-relative. */ > + gdb_byte absolute_addr_bit = (gdb_byte) (insn & 0x2); Same here. > + /* If we're here, it means we have a branch to LR or CTR. If the > + branch was taken, the offset is probably greater than 4 (the next > + instruction), so it's safe to assume that a offset of 4 means we ^ "an offset" > + did not take the branch. */ -- Joel