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

[Joel Brobecker <brobecker@adacore.com>]

Hi Jan,

> > >  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? 

With the addition of the reference in the commit message, I'd say
let's make the change. I don't think the risk is very high, and
I don't want an issue that's most likely non-existant to cause
enough FUD that we'd ignore a patch that looks correct.
Worse case scenario, we revert!

-- 
Joel