| Message ID | 20190726175032.6769-5-richard.henderson@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | target/arm: Convert aa32 base isa to decodetree | expand |
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
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~
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
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
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