Re: [Qemu-devel] [PATCH 04/67] target/arm: Remove offset argument to gen_exception_internal_insn

[Peter Maydell]

On Fri, 26 Jul 2019 at 18:50, Richard Henderson
<address@hidden> wrote:
>
> The actual argument is 0 for all callers.
>
> Signed-off-by: Richard Henderson <address@hidden>
> ---
>  target/arm/translate.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/target/arm/translate.c b/target/arm/translate.c
> index 19b126d4f3..0848fb933a 100644
> --- a/target/arm/translate.c
> +++ b/target/arm/translate.c
> @@ -1239,10 +1239,10 @@ static inline void gen_smc(DisasContext *s)
>      s->base.is_jmp = DISAS_SMC;
>  }
>
> -static void gen_exception_internal_insn(DisasContext *s, int offset, int 
> excp)
> +static void gen_exception_internal_insn(DisasContext *s, int excp)
>  {
>      gen_set_condexec(s);
> -    gen_set_pc_im(s, s->pc - offset);
> +    gen_set_pc_im(s, s->pc);
>      gen_exception_internal(excp);
>      s->base.is_jmp = DISAS_NORETURN;
>  }
> @@ -1294,7 +1294,7 @@ static inline void gen_hlt(DisasContext *s, int imm)
>          s->current_el != 0 &&
>  #endif
>          (imm == (s->thumb ? 0x3c : 0xf000))) {
> -        gen_exception_internal_insn(s, 0, EXCP_SEMIHOST);
> +        gen_exception_internal_insn(s, EXCP_SEMIHOST);
>          return;
>      }
>
> @@ -11984,7 +11984,7 @@ static bool arm_tr_breakpoint_check(DisasContextBase 
> *dcbase, CPUState *cpu,
>          /* End the TB early; it's likely not going to be executed */
>          dc->base.is_jmp = DISAS_TOO_MANY;
>      } else {
> -        gen_exception_internal_insn(dc, 0, EXCP_DEBUG);
> +        gen_exception_internal_insn(dc, EXCP_DEBUG);
>          /* The address covered by the breakpoint must be
>             included in [tb->pc, tb->pc + tb->size) in order
>             to for it to be properly cleared -- thus we
> --
> 2.17.1


I'm not so convinced about this one -- gen_exception_insn()
and gen_exception_internal_insn() shouldn't have the
same pattern of function prototype but different semantics
like this, it's confusing. It happens that both the cases
of wanting to generate an "internal" exception happen to want
it to be taken with the PC being for the following insn,
not the current one, but that seems more coincidence to
me than anything else.

thanks
-- PMM