The actual number of bytes advanced need not be 100% exact,
but we should not cross a page when the insn would not.

If rvc is enabled, the minimum insn size is 2.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/riscv/translate.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/riscv/translate.c b/target/riscv/translate.c
index deda0c8a44..5527f37ada 100644
--- a/target/riscv/translate.c
+++ b/target/riscv/translate.c
@@ -973,7 +973,7 @@ static bool riscv_tr_breakpoint_check(DisasContextBase *dcbase, CPUState *cpu,
        [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.  */
-    ctx->base.pc_next += 4;
+    ctx->base.pc_next += 2;
     return true;
 }
 
-- 
2.25.1

On Sat, 17 Jul 2021 at 23:18, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> The actual number of bytes advanced need not be 100% exact,
> but we should not cross a page when the insn would not.
>
> If rvc is enabled, the minimum insn size is 2.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/riscv/translate.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/target/riscv/translate.c b/target/riscv/translate.c
> index deda0c8a44..5527f37ada 100644
> --- a/target/riscv/translate.c
> +++ b/target/riscv/translate.c
> @@ -973,7 +973,7 @@ static bool riscv_tr_breakpoint_check(DisasContextBase *dcbase, CPUState *cpu,
>         [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.  */
> -    ctx->base.pc_next += 4;
> +    ctx->base.pc_next += 2;
>      return true;
>  }

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

(What goes wrong if we just say "always use a TB size of 1 regardless
of target arch" rather than having the arch return the worst case
minimum insn length?)

thanks
-- PMM

On 7/17/21 1:35 PM, Peter Maydell wrote:
> (What goes wrong if we just say "always use a TB size of 1 regardless
> of target arch" rather than having the arch return the worst case
> minimum insn length?)

Hmm, possibly nothing.  Perhaps I should try that and see what happens...


r~

On Sun, 18 Jul 2021 at 19:02, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 7/17/21 1:35 PM, Peter Maydell wrote:
> > (What goes wrong if we just say "always use a TB size of 1 regardless
> > of target arch" rather than having the arch return the worst case
> > minimum insn length?)
>
> Hmm, possibly nothing.  Perhaps I should try that and see what happens...

Some of the comments in these patches suggest it might trigger
the warning in the disassembler about length mismatches; possibly
also you might get duff (truncated) disassembly output? I suspect
that's probably the extent of the problem. I guess these days the
plugin API might get confused -- does it treat one of these
nothing-there TBs as "nothing there" or does it try to work with
the possibly-half-an-insn ?

-- PMM

On 7/18/21 8:16 AM, Peter Maydell wrote:
> On Sun, 18 Jul 2021 at 19:02, Richard Henderson
> <richard.henderson@linaro.org> wrote:
>>
>> On 7/17/21 1:35 PM, Peter Maydell wrote:
>>> (What goes wrong if we just say "always use a TB size of 1 regardless
>>> of target arch" rather than having the arch return the worst case
>>> minimum insn length?)
>>
>> Hmm, possibly nothing.  Perhaps I should try that and see what happens...
> 
> Some of the comments in these patches suggest it might trigger
> the warning in the disassembler about length mismatches; possibly
> also you might get duff (truncated) disassembly output? I suspect
> that's probably the extent of the problem.

We should be able to work around this by looking at tb->icount.

After patch 13, when breakpoints are always at the beginning of the TB, we'll always have 
tb->icount == 0.

Thinking about this further, with the breakpoint at the head of the TB, there's really no 
point in emitting code for breakpoints at all.  Once we've recognized that there is a 
breakpoint at the current PC, we should just raise the exception.

IIRC only i386 and arm have arch-specific conditional breakpoints.  And, given that all 
cpu state is in sync when looking for bp's, we could probably make do with a callback 
instead of any code generation.

Let me see what I can do...


r~