On Mon, 2016-07-25 at 06:04 +0530, Richard Henderson wrote:
> I noticed a related problem recently, while working on the cmpxchg patch set.
>
> In my opinion, we should (1) merge GETRA and GETPC so there's no confusion
> between the two, (2) push all adjustment down to the final moment before use,
> perhaps in cpu_restore_state.
>
> Thus a null value would be properly retained until checked, and one can
> easily
> call the memory helper functions without confusion.
Ok, after a bit more scrubbing of the code I think I understand what you
mean.
Now assuming we fix that, there is still a problem if the target code, such
as the PPC code, calls a helper that might cause a fault without first
updating the PC in the env, right ? IE. On powerpc for example, that means
that any instruction using a helper that might potentially do loads or stores
needs to first call gen_update_nip().
(The same way we seem to do before generating other exceptions such as traps,
well provided we do it correctly everywhere, I need to double check).
Either that, or change the helpers to capture the PC using GETPC/GETRA from
the first level of helper function (so as to ensure the return address is
correct).
Am I right ?
IE. Even if we fix the 0 vs. -2 problem, I still need this patch:
--- a/target-ppc/translate.c
+++ b/target-ppc/translate.c
@@ -6916,6 +6916,7 @@ static void gen_lve##name(DisasContext *ctx)
\
if (size > 1) { \
tcg_gen_andi_tl(EA, EA, ~(size - 1)); \
} \
+ gen_update_nip(ctx, ctx->nip - 4); \
rs = gen_avr_ptr(rS(ctx->opcode)); \
gen_helper_lve##name(cpu_env, rs, EA); \
tcg_temp_free(EA); \
@@ -6937,6 +6938,7 @@ static void gen_stve##name(DisasContext *ctx)
\
if (size > 1) { \
tcg_gen_andi_tl(EA, EA, ~(size - 1)); \
} \
+ gen_update_nip(ctx, ctx->nip - 4); \
rs = gen_avr_ptr(rS(ctx->opcode)); \
gen_helper_stve##name(cpu_env, rs, EA); \
tcg_temp_free(EA); \
(And possibly others I haven't yet audited)
Cheers,
Ben.
