Re: [PATCH] PPC - Stepping off breakpoints in non-stop mode

[Joel Brobecker <brobecker at adacore dot com>]

Hi Luis,

> 2008-06-24  Luis Machado  <luisgpm@br.ibm.com>
> 
> 	* 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