On Tue, 4 Oct 2022 at 20:27, Richard Henderson <richard.hender...@linaro.org> wrote: > > On 10/4/22 09:23, Peter Maydell wrote: > >> @@ -347,16 +354,22 @@ static void gen_exception_internal(int excp) > >> > >> static void gen_exception_internal_insn(DisasContext *s, int excp) > >> { > >> + target_ulong pc_save = s->pc_save; > >> + > >> gen_a64_update_pc(s, 0); > >> gen_exception_internal(excp); > >> s->base.is_jmp = DISAS_NORETURN; > >> + s->pc_save = pc_save; > > > > What is trashing s->pc_save that we have to work around like this, > > here and in the other similar changes ? > > gen_a64_update_pc trashes pc_save. > > Off of the top of my head, I can't remember what conditionally uses > exceptions (single > step?). But the usage pattern that is interesting is > > brcond(x, y, L1) > update_pc(disp1); > exit-or-exception. > L1: > update_pc(disp2); > exit-or-exception. > > where at L1 we should have the same pc_save value as we did at the brcond. > Saving and > restoring around (at least some of) the DISAS_NORETURN points achieves that.
(I figured it would be easiest to continue this conversation in this thread rather than breaking it up to reply to the v6 equivalent patch.) I guess it does, but it feels like a weird place to be doing that. If what we want is "at the label L1 we know the pc_save value needs to be some specific thing", then shouldn't we achieve that by setting pc_save to a specific value at that point? e.g. wrapping gen_set_label() with a function that does "set the label here, guest PC value is going to be this". Which should generally be the save_pc value at the point where you emit the brcond() to this label, right? Maybe you could even have a little struct that wraps up "TCGLabel* and the pc_save value associated with a branch to it". But anyway, I think the less confusing way to handle this is that the changes to pc_save happen at the label, because that's where the runtime flow-of-control is already being dealt with. Also, I think that you need to do something for the case in translate.c where we call tcg_remove_ops_after() -- that now needs to fix pc_save back up to the value it had when we called tcg_last_op(). (There is also one of those in target/i386, I haven't checked whether the i386 pcrel handling series took care of that.) thanks -- PMM