On 6/6/19 12:30 PM, Michael Rolnik wrote:
> +enum {
> + BS_NONE = 0, /* Nothing special (none of the below) */
> + BS_STOP = 1, /* We want to stop translation for any reason */
> + BS_BRANCH = 2, /* A branch condition is reached */
> + BS_EXCP = 3, /* An exception condition is reached */
> +};
This set of exit conditions is confused and incomplete.
(1) BS_BRANCH and BS_EXCP do the same thing, namely they
equate exactly to DISAS_NORETURN.
(2) BS_NONE equates exactly to DISAS_NEXT.
(3) BS_STOP is probably better named DISAS_EXIT, just so
it matches the other naming above, and that it will
result in a call to tcg_gen_exit_tb.
(4) You're missing a case that applies to indirect branches
which should use tcg_gen_lookup_and_goto_tb().
I suggest this be called DISAS_LOOKUP.
> +static void gen_goto_tb(DisasContext *ctx, int n, target_ulong dest)
> +{
> + TranslationBlock *tb = ctx->tb;
> +
> + if (ctx->singlestep == 0) {
> + tcg_gen_goto_tb(n);
> + tcg_gen_movi_i32(cpu_pc, dest);
> + tcg_gen_exit_tb(tb, n);
> + } else {
> + tcg_gen_movi_i32(cpu_pc, dest);
> + gen_helper_debug(cpu_env);
> + tcg_gen_exit_tb(NULL, 0);
> + }
> +}
Should set ctx->bstate = DISAS_NORETURN here, and not to BS_BRANCH in the
callers.
> + if (avr_feature(ctx->env, AVR_FEATURE_ADIW_SBIW) == false) {
> + gen_helper_unsupported(cpu_env);
> +
> + ctx->bstate = BS_EXCP;
> + return true;
> + }
It would be good to define a helper function
static bool have_feature(DisasContext *ctx, int feature)
{
if (!avr_feature(ctx->env, feature)) {
gen_helper_unsupported(cpu_env);
ctx->bstate = DISAS_NORETURN;
return false;
}
return true;
}
so that this pattern becomes
if (!have_feature(ctx, AVR_FEATURE_FOO)) {
return true;
}
// Lots of code
return true;
or
if (have_feature(ctx, AVR_FEATURE_FOO)) {
// Just a few lines
}
return true;
> +/*
> + * Returns from subroutine. The return address is loaded from the STACK.
> + * The Stack Pointer uses a preincrement scheme during RET.
> + */
> +static bool trans_RET(DisasContext *ctx, arg_RET *a)
> +{
> + gen_pop_ret(ctx, cpu_pc);
> +
> + tcg_gen_exit_tb(NULL, 0);
> +
> + ctx->bstate = BS_BRANCH;
> + return true;
> +}
With very few exceptions, the lone use of tcg_gen_exit_tb is a bug, because you
have failed to take ctx->singlestep into account.
In this case, this is one of the indirect branch places which you should be
using DISAS_LOOKUP.
> +static bool trans_RETI(DisasContext *ctx, arg_RETI *a)
> +{
> + gen_pop_ret(ctx, cpu_pc);
> +
> + tcg_gen_movi_tl(cpu_If, 1);
> +
> + tcg_gen_exit_tb(NULL, 0);
> +
> + ctx->bstate = BS_BRANCH;
> + return true;
> +}
Here you need to use DISAS_EXIT, because instructions that enable interrupts
also need to exit to the main loop so that we re-evaluate whether interrupts
need to be delivered.
> + if (ctx.singlestep) {
> + if (ctx.bstate == BS_STOP || ctx.bstate == BS_NONE) {
> + tcg_gen_movi_tl(cpu_pc, ctx.npc);
> + }
> + gen_helper_debug(cpu_env);
> + tcg_gen_exit_tb(NULL, 0);
> + } else {
> + switch (ctx.bstate) {
> + case BS_STOP:
> + case BS_NONE:
> + gen_goto_tb(&ctx, 0, ctx.npc);
> + break;
> + case BS_EXCP:
> + case BS_BRANCH:
> + tcg_gen_exit_tb(NULL, 0);
> + break;
> + default:
> + break;
> + }
> + }
Better written as
switch (ctx.bstate) {
case DISAS_NORETURN:
break;
case DISAS_NEXT:
case DISAS_CHAIN:
/* Note gen_goto_tb checks singlestep. */
gen_goto_tb(&ctx, 0, ctx.npc);
break;
case DISAS_LOOKUP:
if (!ctx.singlestep) {
tcg_gen_lookup_and_goto_ptr();
break;
}
/* fall through */
case DISAS_EXIT:
if (ctx.singlestep) {
gen_helper_debug(cpu_env);
} else {
tcg_gen_exit_tb(NULL, 0);
}
break;
default:
g_assert_not_reached();
}
r~
