|
From: | Richard Henderson |
Subject: | Re: [PATCH v2 4/5] target/riscv: Add support for PC-relative translation |
Date: | Thu, 30 Mar 2023 10:07:44 -0700 |
User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.9.0 |
On 3/29/23 18:09, liweiwei wrote:
@@ -51,26 +59,43 @@ static bool trans_jal(DisasContext *ctx, arg_jal *a) static bool trans_jalr(DisasContext *ctx, arg_jalr *a) { TCGLabel *misaligned = NULL; + TCGv succ_pc = tcg_temp_new();succ_pc can by null for !CF_PCREL...I think this is OK since it's only used for CF_PCREL.
It allocates an unused temp. Not a bug per se, but an easily fixable mistake.
... or initialized like } else { succ_pc = tcg_constant_tl(ctx->pc_succ_insn); }
If you do this, you can avoid the test/set/seti later.
if (misaligned) { gen_set_label(misaligned); - gen_exception_inst_addr_mis(ctx); + gen_exception_inst_addr_mis(ctx, target_pc); }This is what I expected from patch 3: cpu_pc is unchanged, with the new (incorrect) address passed to inst_addr_mis for assigning to badaddr. Bug being fixed here, thus should really be a separate patch.It's OK to update cpu_pc before gen_exception_inst_addr_mis() since it will restore the current pc by gen_set_pc_imm() after update cpu_pc into badaddr.
True, but I think it's confusing to set cpu_pc for it's mere use in copying to badaddr, and rely on generate_exception to reset cpu_pc to the correct value.
However, after PC-relative translation is enabled, we cannot use gen_set_pc to directly update cpu_pc in above case, since gen_set_pc() will break the pc_save, and make gen_set_pc_imm() in gen_exception_inst_addr_mis() failed. So we introduce a temp target_pc instead of cpu_pc to compute the destination pc and use it to do misaligned check.
Exactly.Which is why I think it is better to simply pass gen_exception_inst_addr_mis the value to use with badaddr in a normal temp (or constant). And do this always, not simply in the one case where it is absolutely required to not clobber cpu_pc.
r~
[Prev in Thread] | Current Thread | [Next in Thread] |