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