[Ltrace-devel] [PATCH 7/8] mipsel: Singlestep over breakpoints

[Edgar E. Iglesias]

On Thu, Sep 27, 2012 at 01:36:48AM +0200, Petr Machata wrote:
> edgar.iglesias at gmail.com writes:
> 
> > +#define ARCH_HAVE_ATOMIC_SINGLESTEP
> 
> I think I should rename this to ARCH_HAVE_SOFT_SINGLESTEP or some such.
> The original intention was to support stepping over lwarx on PPC, but
> both MIPS and ARM will need to use this to emulate singlestepping.

Yes, that sound like a good change. If you wan't me to spinn a v3 with
such a change I can do that but it's maybe easier to make a new patch
later on top of my mipsel series.

> 
> > +int
> > +arch_atomic_singlestep(struct Process *proc, struct breakpoint *sbp,
> 
> [...]
> 
> > +		arch_addr_t baddr = (void *) newpcs[nr];
> 
> The cast should be arch_addr_t.  The fact that arch_addr_t is void *
> under the hood is itself something that needs to be fixed eventually.
> 
> > +		/* Not sure what to do here. We've already got a bp.  */
> > +		if (dict_find_entry(proc->breakpoints, baddr)) {
> > +			fprintf(stderr, "skip %p %p\n", baddr, add_cb_data);
> > +			continue;
> > +		}
> 
> There's no good answer.  Ltrace doesn't currently handle duplicate
> breakpoints gracefully.  I write about it a bit in one of the other
> mails.
> 
> > -#if defined __sparc__  || defined __ia64___ || defined __mips__
> > +#if defined __sparc__  || defined __ia64___
> 
> Awesome ;) That __ia64___ is likely a typo (note the three underscores),
> so that leaves just Sparc before this ifdef is good to go!

haha, I didn't notice that :)

Thanks for the good review, I've gone through your comments and fixed things
where you requested changes. Will post a v2 in a couple of minutes.

Cheers