Re: [Qemu-devel] [PATCH] target-*: Advance pc after recognizing a breakpoint

[Richard Henderson]

On 10/16/2015 04:08 AM, Sergey Fedorov wrote:
> On 16.10.2015 04:14, Richard Henderson wrote:
> > On 10/16/2015 03:36 AM, Peter Maydell wrote:
> > > On 14 October 2015 at 22:02, Richard Henderson <address@hidden> wrote:
> > > > On 10/15/2015 06:34 AM, Peter Maydell wrote:
> > > > > This is still the same cryptic comment we have in the
> > > > > targets which do do this. Can we have something
> > > > > that is a bit more explanatory about what is going on and
> > > > > why we need to do this, please?
> > > >
> > > >
> > > > Suggestions?
> > >
> > > ...well, I don't entirely understand the problem it's
> > > fixing, which is why I'm asking for a better comment :-)
> >
> > Heh.  Fair enough.  How about
> >
> >    /* The address covered by the breakpoint must be included in
> >       [tb->pc, tb->pc + tb->size) in order to for it to be
> >       properly cleared -- thus we increment the PC here so that
> >       the logic setting tb->size below does the right thing.  */
> >
> > There are two edge cases that cause the problem with clearing that
> > could be described, but I think that the comment becomes too bulky, as
> > well as confuses the situation for someone cutting-and-pasting the
> > logic to a new port.
>
> Maybe we could rather fix that condition in
> tb_invalidate_phys_page_range()? It seems weird that it can't handle a
> zero-sized TB.

We also need to be able to handle a TB which crosses a page.  E.g. the 
breakpoint is at the page boundary, and we fall through into it from the top. 
This will be true on e.g. x86.  This is not simply true for breakpoint 
insertion/removal, but also page invalidation.

The same fix, adding a byte to the size, handles this as well.


r~