Re: [PATCH v2 1/2] ppc: use 'trap' ('tw, 31, 0, 0', 0x7fe00008) as breakpoint instruction

[Jan Vrany <jan.vrany@labware.com>]

Hi Joel

On Sat, 2021-11-27 at 10:45 +0400, Joel Brobecker wrote:
> Hi Jan,
> 
> On Wed, Nov 24, 2021 at 01:09:25PM +0000, Jan Vrany via Gdb-patches
> wrote:
> > GDB used to use "tw 12, r2, r2" as a breakpoint instruction. While
> > it
> > works, the PowerPC specifies 'tw, 31, 0, 0' (0x7fe00008) as the
> > canonical unconditional trap.
> > ---
> >  gdb/rs6000-tdep.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c
> > index 87a494e0bb8..43880fa4426 100644
> > --- a/gdb/rs6000-tdep.c
> > +++ b/gdb/rs6000-tdep.c
> > @@ -824,8 +824,8 @@ rs6000_fetch_pointer_argument (struct
> > frame_info *frame, int argi,
> >  
> >  /* Sequence of bytes for breakpoint instruction.  */
> >  
> > -constexpr gdb_byte big_breakpoint[] = { 0x7d, 0x82, 0x10, 0x08 };
> > -constexpr gdb_byte little_breakpoint[] = { 0x08, 0x10, 0x82, 0x7d
> > };
> > +constexpr gdb_byte big_breakpoint[] = { 0x7f, 0xe0, 0x00, 0x08 };
> > +constexpr gdb_byte little_breakpoint[] = { 0x08, 0x00, 0xe0, 0x7f
> > };
> >  
> >  typedef BP_MANIPULATION_ENDIAN (little_breakpoint, big_breakpoint)
> >    rs6000_breakpoint;
> 
> I'm going to say OK, but I'd like to know what your source is
> for this, so we can document that change. Part of me wonders
> why we're making a change like this one on some code which
> works, which is always a risk, no matter how small, unless we have
> enough evidence that this is going to be helpful in practice.

According to Power ISA 3.0 B spec [1], section 3.3.11 Fixed-Point Trap
Instructions (also section C.6 Trap Mnemonics), unconditional trap 
is encoded as `tw 31, 0, 0`. While debugging a problem on POWER
(gdb not stopping on a `trap` instruction) I found that GDB is using
`tdnl r2, r2` (trap if not less than) which works but it is different
than unconditional trap as specified. I could not find out why 
`tdnl` was used instead of unconditional `trap`. I thought it is 
more "natural" to use canonical unconditional trap. So this is merely
a "cosmetic" change. 

I agree that I should have included reference to the spec in the 
commit message and maybe also add comment to the code. Will do in 
next version (should we decide to keep this patch).

Now, I do understand the risk of changing something that works. I'm
happy to drop this patch since the next one in this miniseries 
fixes the problem we were experiencing properly (that is, GDB not
stopping on software-planted breakpoint instructions).

What do you think? 

Thanks! 

Jan

[1]:
https://openpowerfoundation.org/?resource_lib=power-isa-version-3-0

> 
> Thank you,