Re: [PATCH for-6.1 v5 12/15] accel/tcg: Move breakpoint recognition outside translation

["Alex Bennée" <alex.bennee@linaro.org>]

Richard Henderson <richard.henderson@linaro.org> writes:

> Trigger breakpoints before beginning translation of a TB
> that would begin with a BP.  Thus we never generate code
> for the BP at all.
>
> Single-step instructions within a page containing a BP so
> that we are sure to check each insn for the BP as above.
>
> We no longer need to flush any TBs when changing BPs.
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/286
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/404
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/489
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

However...
>  
> +static bool check_for_breakpoints(CPUState *cpu, target_ulong pc,
> +                                  uint32_t *cflags)
> +{
> +    CPUBreakpoint *bp;
> +    bool match_page = false;
> +
> +    if (likely(QTAILQ_EMPTY(&cpu->breakpoints))) {
> +        return false;
> +    }
> +
> +    QTAILQ_FOREACH(bp, &cpu->breakpoints, entry) {
> +        /*
> +         * If we have an exact pc match, trigger the breakpoint.
> +         * Otherwise, note matches within the page.
> +         */
> +        if (pc == bp->pc) {
> +            bool match_bp = false;
> +
> +            if (bp->flags & BP_GDB) {
> +                match_bp = true;
> +            } else if (bp->flags & BP_CPU) {
> +#ifdef CONFIG_USER_ONLY
> +                g_assert_not_reached();
> +#else
> +                CPUClass *cc = CPU_GET_CLASS(cpu);
> +                assert(cc->tcg_ops->debug_check_breakpoint);
> +                match_bp = cc->tcg_ops->debug_check_breakpoint(cpu);
> +#endif
> +            }
> +
> +            if (match_bp) {
> +                cpu->exception_index = EXCP_DEBUG;
> +                return true;
> +            }
> +        } else if (((pc ^ bp->pc) & TARGET_PAGE_MASK) == 0) {
> +            match_page = true;
> +        }
> +    }
> +
> +    /*
> +     * Within the same page as a breakpoint, single-step,
> +     * returning to helper_lookup_tb_ptr after each looking
> +     * for the actual breakpoint.
> +     *
> +     * TODO: Perhaps better to record all of the TBs associated
> +     * with a given virtual page that contains a breakpoint, and
> +     * then invalidate them when a new overlapping breakpoint is
> +     * set on the page.  Non-overlapping TBs would not be
> +     * invalidated, nor would any TB need to be invalidated as
> +     * breakpoints are removed.
> +     */
> +    if (match_page) {
> +        *cflags = (*cflags & ~CF_COUNT_MASK) | CF_NO_GOTO_TB | 1;
> +    }
> +    return false;
> +}
> +

This seems to have a really negative effect on:

 ./tests/venv/bin/avocado run tests/acceptance/reverse_debugging.py:ReverseDebugging_AArch64.test_aarch64_virt

Even bumping the timeout up from 10 to 300 isn't enough to get it
working. I'm not sure if this is a time or a hang.

-- 
Alex Bennée