Re: [PATCHv3] aarch64: detect atomic sequences like other ll/sc architectures

[Kyle McMartin <kmcmarti at redhat dot com>]

On Thu, Apr 24, 2014 at 05:55:57AM -0700, Joel Brobecker wrote:
> > 2014-04-22  Kyle McMartin  <kyle@redhat.com>
> > 
> > gdb:
> >         * aarch64-tdep.c (aarch64_deal_with_atomic_sequence): New function.
> >         (aarch64_gdbarch_init): Handle single stepping of atomic sequences
> >         with aarch64_deal_with_atomic_sequence.
> > 
> > gdb/testsuite:
> >         * gdb.arch/aarch64-atomic-inst.c: New file.
> >         * gdb.arch/aarch64-atomic-inst.exp: New file.
> 
> This looks good to me. You just forgot to change the name of
> the new function in the ChangeLog entry.  Also, a couple of
> really minor nits that I happen to spot in your patch below,
> but they only affect formatting, so the patch is pre-approved.
> 

Thanks! I'll fix the nits below & the changelog and send a v4. Would you
be willing to commit?

> > +      /* Check if the instruction is a conditional branch.  */
> > +      if (decode_bcond (loc, insn, &cond, &offset))
> > +	{
> > +
> > +	  if (bc_insn_count >= 1)
> > +	    return 0;
> 
> Did you mean to leave an empty line at the start of the if block?
> Not a problem if yoi meant to, but a little unusual, so I thought
> I'd ask.
> 

No, my mistake, I had some local variables here that I removed.

> 
> > +
> > +	  /* It is, so we'll try to set a breakpoint at the destination.  */
> > +	  breaks[1] = loc + offset;
> > +
> > +	  bc_insn_count++;
> > +	  last_breakpoint++;
> > +	}
> > +
> > +      /* Look for the Store Exclusive which closes the atomic sequence.  */
> > +      if (decode_masked_match (insn, 0x3fc00000, 0x08000000))
> > +	{
> > +          closing_insn = loc;
> > +	  break;
> 
> Indentation should be tabs first, and then spaces. I know it's a PITA,
> but unfortunately something we try to follow in the GDB project. Can
> you fix the "closing_insn" line?
> 

Absolutely, my bad.

> Thanks,
> -- 
> Joel

regards, Kyle