| Message ID | 20210717221851.2124573-11-richard.henderson@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | tcg: breakpoint reorg | expand |
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~
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; }
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(-) -- 2.25.1