[04/67] target/arm: Remove offset argument to gen_exception_internal_insn

Message ID 20190726175032.6769-5-richard.henderson@linaro.org
State New
Headers show
Series
  • target/arm: Convert aa32 base isa to decodetree
Related show

Commit Message

Richard Henderson July 26, 2019, 5:49 p.m.
The actual argument is 0 for all callers.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

---
 target/arm/translate.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

-- 
2.17.1

Comments

Peter Maydell July 29, 2019, 1:52 p.m. | #1
On Fri, 26 Jul 2019 at 18:50, Richard Henderson
<richard.henderson@linaro.org> wrote:
>

> The actual argument is 0 for all callers.

>

> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

> ---

>  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
Richard Henderson July 30, 2019, 2:11 a.m. | #2
On 7/29/19 6:52 AM, Peter Maydell wrote:
> 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.


I don't like "offsets", because they don't work as expected between different
modes.  Would you prefer the pc in full be passed in?  Would you prefer that
the previous patches also pass in a pc, instead of implicitly using
base.pc_next (you had rationale vs patch 2 for why it was ok as-is).

Shall we shuffle these patches later, after the Great Renaming of Things Named
PC, as discussed wrt patch 6 (pc_read and friends), so that the "offset"
parameter immediately becomes the Right Sort of PC, rather than some
intermediary confusion?


r~
Peter Maydell Aug. 6, 2019, 9:55 a.m. | #3
On Tue, 30 Jul 2019 at 03:11, Richard Henderson
<richard.henderson@linaro.org> wrote:
>

> On 7/29/19 6:52 AM, Peter Maydell wrote:

> > 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.

>

> I don't like "offsets", because they don't work as expected between different

> modes.  Would you prefer the pc in full be passed in?  Would you prefer that

> the previous patches also pass in a pc, instead of implicitly using

> base.pc_next (you had rationale vs patch 2 for why it was ok as-is).

>

> Shall we shuffle these patches later, after the Great Renaming of Things Named

> PC, as discussed wrt patch 6 (pc_read and friends), so that the "offset"

> parameter immediately becomes the Right Sort of PC, rather than some

> intermediary confusion?


I think what we're really trying to distinguish here is two
orthogonal sets of possibilities:
 * take an exception, with the PC pointing to the following insn
 * take an exception, with the PC pointing to the current insn
and also
 * take an "internal" exception
 * take a guest-visible exception

(of which we happen to only want 2 of the 4 possible flavours at
the moment). I don't particularly mind what API we use as long
as the naming/parameter setup cleanly separates out the two
orthogonal concerns such that you could have all four without
having to rename anything. Possibilities:
 * have gen_exception{_internal,}_insn and
   gen_exception{_internal,}_next_insn
 * have the functions take a bool for "exception on this insn
   or on next insn?" (not ideal because 'bool' parameters are
   a bit opaque in meaning at the callsites)
 * pass in the specific PC to use

PS: the "_insn" part of the function name isn't sacrosanct:
it sort of makes sense I think but if better names occur that
don't include it that's fine.

thanks
-- PMM

Patch

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