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

[Peter Maydell]

On Tue, 30 Jul 2019 at 03:11, Richard Henderson
<address@hidden> 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