Re: [PATCH v2 04/15] target/ppc: Move DISAS_NORETURN setting into gen_exception*

[Richard Henderson <richard.henderson@linaro.org>]

On 4/29/21 10:07 AM, Luis Fernando Fujita Pires wrote:
>>> -static inline void gen_stop_exception(DisasContext *ctx)
>>> +static inline void gen_end_tb_exception(DisasContext *ctx, uint32_t
>>> +excp)
>>>    {
>>> -    gen_update_nip(ctx, ctx->base.pc_next);
>>> -    ctx->exception = POWERPC_EXCP_STOP;
>>> +    /* No need to update nip for SYNC/BRANCH, as execution flow will change
>> */
>>> +    if ((excp != POWERPC_EXCP_SYNC) &&
>>> +        (excp != POWERPC_EXCP_BRANCH))
>>> +    {
>>> +        gen_update_nip(ctx, ctx->base.pc_next);
>>> +    }
>>> +    ctx->exception = excp;
>>> +    ctx->base.is_jmp = DISAS_NORETURN;
>>>    }
>>
>> Hmm.  You didn't actually raise the exception, so you can't set
>> DISAS_NORETURN that way.  It looks like you should be using
>> gen_exception_nip().
> 
> This is reproducing the behavior that was implemented before the DISAS_NORETURN changes, that caused check-tcg to fail with an assertion otherwise.
> 
> IIUC, POWERPC_EXCP_{STOP,SYNC,BRANCH} are not really exceptions and, in these cases, ctx->exception is being used just to cause  ppc_tr_translate_insn() to end the translation block. If so, we should not be using ctx->exception for that, but I believe fixing that to not use ctx->exception belongs in a separate stand-alone patch.

Hum.  Well, you can set is_jmp = DISAS_TOO_MANY to force exit from the 
translation loop.

I believe a proper fix would be to turn all of these "fake" exceptions into 
DISAS_FOO constants, to be assigned to is_jmp. There are a bunch of 
DISAS_TARGET_N enumerators which should be renamed like in target/arm/translate.h:

#define DISAS_WFI       DISAS_TARGET_2
#define DISAS_SWI       DISAS_TARGET_3

etc.  Then most of the code that is special casing these constants should get 
moved to ppc_tr_tb_stop.


r~