Re: [Qemu-devel] [PATCH v9 04/26] target: [tcg] Add generic translation framework

[Richard Henderson <rth@twiddle.net>]

On 06/25/2017 01:59 AM, Lluís Vilanova wrote:
> +static inline void translate_block_tcg_check(const DisasContextBase *db)
> +{
> +    if (tcg_check_temp_count()) {
> +        error_report("warning: TCG temporary leaks before "TARGET_FMT_lx,
> +                     db->pc_next);
> +    }
> +}
> +
> +void translate_block(const TranslatorOps *ops, DisasContextBase *db,
> +                     CPUState *cpu, TCGv_env *tcg_cpu, TranslationBlock *tb)

tcg_cpu isn't the best name -- it doesn't reference a version of cpu.  Of 
course, you can always get at tcg_ctx.tcg_env, so there's no point in passing 
it anyway.

> +    /* Sanity-check ops */
> +    if (ops->disas_insn == NULL) {
> +        error_report("Missing ops->disas_insn");
> +        abort();
> +    }

Why?  Surely an immediate crash by calling to null is just as easy to debug.

And, bikeshedding, perhaps translate_insn is a better name.  On the first read 
through I assumed this was related to the disassembly log.

> +    while (true) {
> +        CPUBreakpoint *bp;
> +
> +        db->num_insns++;
> +        if (ops->insn_start) {
> +            ops->insn_start(db, cpu);
> +        }

This *must* be defined.  A target cannot skip emitting insn_start or unwinding 
won't work.

> +
> +        /* Early exit before breakpoint checks */
> +        if (unlikely(db->is_jmp != DJ_NEXT)) {
> +            break;
> +        }

This must be done at the end of the loop, not at the beginning of the next 
loop, after we've already emitted insn_start for the following insn.

That said, you already do have that check below, so what is this intended to do?

> +        /* Pass breakpoint hits to target for further processing */
> +        bp = NULL;
> +        do {
> +            bp = cpu_breakpoint_get(cpu, db->pc_next, bp);
> +            if (unlikely(bp) && ops->breakpoint_check) {
> +                BreakpointCheckType bp_check = ops->breakpoint_check(

Is there any point in any of these hooks being null?
An empty function in most cases, or here, one that returns BC_MISS.

> +                    db, cpu, bp);
> +                if (bp_check == BC_HIT_INSN) {
> +                    /* Hit, keep translating */
> +                    /*
> +                     * TODO: if we're never going to have more than one BP in a
> +                     *       single address, we can simply use a bool here.
> +                     */
> +                    break;
> +                } else if (bp_check == BC_HIT_TB) {
> +                    goto done_generating;
> +                } else {
> +                    error_report("Unexpected BreakpointCheckType %d", bp_check);
> +                    abort();

What happened to BC_MISS?  And surely better structured as a switch with

     default:
         g_assert_not_reached();

rather than custom logging.

> +#ifdef DEBUG_DISAS
> +    if (qemu_loglevel_mask(CPU_LOG_TB_IN_ASM) &&
> +        qemu_log_in_addr_range(db->pc_first)) {
> +        int flags;
> +        if (ops->disas_flags) {
> +            flags = ops->disas_flags(db);
> +        } else {
> +            flags = 0;
> +        }
> +        qemu_log_lock();
> +        qemu_log("----------------\n");
> +        qemu_log("IN: %s\n", lookup_symbol(db->pc_first));
> +        log_target_disas(cpu, db->pc_first, db->pc_next - db->pc_first, flags);
> +        qemu_log("\n");
> +        qemu_log_unlock();

I think the hook shouldn't be just the flags, but the whole call to 
log_target_disas, at which point there isn't a reason to have a separate in a 
hook for the flags.  Consider the extra checks done for s390x and hppa.


r~