On 14 September 2015 at 11:51, Sergey Fedorov <serge.f...@gmail.com> wrote:
> A QEMU breakpoint match is not definitely an architectural breakpoint
> match. If an exception is generated unconditionally during translation,
> it is hardly possible to ignore it in the debug exceptoin hanlder.
"exception".
>
> Generate a call to helper to check CPU breakpoints and raise an
> exception only if any breakpoint architecturally matches.
>
> Signed-off-by: Sergey Fedorov <serge.f...@gmail.com>
> ---
> target-arm/helper.h | 2 ++
> target-arm/op_helper.c | 20 +++++++++++++++++++-
> target-arm/translate-a64.c | 12 +++++++-----
> target-arm/translate.c | 12 +++++++-----
> 4 files changed, 35 insertions(+), 11 deletions(-)
>
> diff --git a/target-arm/helper.h b/target-arm/helper.h
> index 827b33d..c2a85c7 100644
> --- a/target-arm/helper.h
> +++ b/target-arm/helper.h
> @@ -54,6 +54,8 @@ DEF_HELPER_1(yield, void, env)
> DEF_HELPER_1(pre_hvc, void, env)
> DEF_HELPER_2(pre_smc, void, env, i32)
>
> +DEF_HELPER_1(check_breakpoints, void, env)
> +
> DEF_HELPER_3(cpsr_write, void, env, i32, i32)
> DEF_HELPER_1(cpsr_read, i32, env)
>
> diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c
> index b298e57..24dcefd 100644
> --- a/target-arm/op_helper.c
> +++ b/target-arm/op_helper.c
> @@ -877,6 +877,15 @@ bool arm_debug_check_watchpoint(CPUState *cs)
> return check_watchpoints(cpu);
> }
>
> +void HELPER(check_breakpoints)(CPUARMState *env)
> +{
> + ARMCPU *cpu = arm_env_get_cpu(env);
> +
> + if (check_breakpoints(cpu)) {
> + HELPER(exception_internal(env, EXCP_DEBUG));
> + }
> +}
> +
> void arm_debug_excp_handler(CPUState *cs)
> {
> /* Called by core code when a watchpoint or breakpoint fires;
> @@ -904,7 +913,16 @@ void arm_debug_excp_handler(CPUState *cs)
> arm_debug_target_el(env));
> }
> } else {
> - if (check_breakpoints(cpu)) {
> + CPUBreakpoint *bp;
> + uint64_t pc = is_a64(env) ? env->pc : env->regs[15];
> +
> + QTAILQ_FOREACH(bp, &cs->breakpoints, entry) {
> + if (bp->pc == pc && !(bp->flags & BP_CPU)) {
> + return;
> + }
> + }
This extra code looks right, but isn't it fixing a different bug?
> +
> + {
Rather than adding the extra block I would just de-indent the code
that used to live inside the if() and move the variable declaration
to the top of the new block.
> bool same_el = (arm_debug_target_el(env) == arm_current_el(env));
> if (extended_addresses_enabled(env)) {
> env->exception.fsr = (1 << 9) | 0x22;
> diff --git a/target-arm/translate-a64.c b/target-arm/translate-a64.c
> index 5c13e15..a5927fd 100644
> --- a/target-arm/translate-a64.c
> +++ b/target-arm/translate-a64.c
> @@ -11000,11 +11000,13 @@ void gen_intermediate_code_internal_a64(ARMCPU *cpu,
> if (unlikely(!QTAILQ_EMPTY(&cs->breakpoints))) {
> QTAILQ_FOREACH(bp, &cs->breakpoints, entry) {
> if (bp->pc == dc->pc) {
> - gen_exception_internal_insn(dc, 0, EXCP_DEBUG);
> - /* Advance PC so that clearing the breakpoint will
> - invalidate this TB. */
> - dc->pc += 2;
> - goto done_generating;
> + if (bp->flags & BP_CPU) {
> + gen_helper_check_breakpoints(cpu_env);
> + break;
> + } else {
> + gen_exception_internal_insn(dc, 0, EXCP_DEBUG);
> + goto done_generating;
> + }
You seem to have dropped the "advance the PC" code -- why is that ok?
thanks
-- PMM
