On Tue, 2016-07-26 at 06:12 +0530, Richard Henderson wrote: > The return address argument to the softmmu template helpers was > confused. In the legacy case, we wanted to indicate that there > is no return address, and so passed in NULL. However, we then > immediately subtracted GETPC_ADJ from NULL, resulting in a non-zero > value, indicating the presence of an (invalid) return address. > > Push the GETPC_ADJ subtraction down to the only point it's required: > immediately before use within cpu_restore_state, after all NULL > pointer > checks have been completed. This makes GETPC and GETRA identical. > > Remove GETRA as the lesser used macro, replacing all uses with GETPC. > > Signed-off-by: Richard Henderson <r...@twiddle.net> > --- > > Ben, this should fix the "-2" problem that you reported. Of course, > as also discussed in that thread, this won't fix the whole issue.
Thanks. I've fixed the rest of the issue here: https://github.com/ozbenh/qemu/commit/5b84a90aa696a7072eaa7a2f4cf6ade7cdd10358 While I will send to the list later today hopefully, along with the rest of my cleanups and fixes that are starting to pile up here: https://github.com/ozbenh/qemu/commits/wip I haven't tackled the FP exceptions yet though but that can wait. Cheers, Ben. > r~ > > --- > cputlb.c | 6 ++---- > include/exec/exec-all.h | 9 +++------ > softmmu_template.h | 32 ++++++-------------------------- > target-arm/helper.c | 6 +++--- > target-mips/op_helper.c | 18 +++++++++--------- > translate-all.c | 1 + > 6 files changed, 24 insertions(+), 48 deletions(-) > > diff --git a/cputlb.c b/cputlb.c > index d068ee5..3c99c34 100644 > --- a/cputlb.c > +++ b/cputlb.c > @@ -543,10 +543,8 @@ static bool victim_tlb_hit(CPUArchState *env, > size_t mmu_idx, size_t index, > #undef MMUSUFFIX > > #define MMUSUFFIX _cmmu > -#undef GETPC_ADJ > -#define GETPC_ADJ 0 > -#undef GETRA > -#define GETRA() ((uintptr_t)0) > +#undef GETPC > +#define GETPC() ((uintptr_t)0) > #define SOFTMMU_CODE_ACCESS > > #define SHIFT 0 > diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h > index acda7b6..6a317db 100644 > --- a/include/exec/exec-all.h > +++ b/include/exec/exec-all.h > @@ -335,13 +335,12 @@ static inline void tb_add_jump(TranslationBlock > *tb, int n, > tb_next->jmp_list_first = (uintptr_t)tb | n; > } > > -/* GETRA is the true target of the return instruction that we'll > execute, > - defined here for simplicity of defining the follow-up macros. */ > +/* GETPC is the true target of the return instruction that we'll > execute. */ > #if defined(CONFIG_TCG_INTERPRETER) > extern uintptr_t tci_tb_ptr; > -# define GETRA() tci_tb_ptr > +# define GETPC() tci_tb_ptr > #else > -# define GETRA() \ > +# define GETPC() \ > ((uintptr_t)__builtin_extract_return_addr(__builtin_return_addre > ss(0))) > #endif > > @@ -354,8 +353,6 @@ extern uintptr_t tci_tb_ptr; > smaller than 4 bytes, so we don't worry about special-casing > this. */ > #define GETPC_ADJ 2 > > -#define GETPC() (GETRA() - GETPC_ADJ) > - > #if !defined(CONFIG_USER_ONLY) > > struct MemoryRegion *iotlb_to_region(CPUState *cpu, > diff --git a/softmmu_template.h b/softmmu_template.h > index 284ab2c..4cbb714 100644 > --- a/softmmu_template.h > +++ b/softmmu_template.h > @@ -150,9 +150,6 @@ WORD_TYPE helper_le_ld_name(CPUArchState *env, > target_ulong addr, > uintptr_t haddr; > DATA_TYPE res; > > - /* Adjust the given return address. */ > - retaddr -= GETPC_ADJ; > - > if (a_bits > 0 && (addr & ((1 << a_bits) - 1)) != 0) { > cpu_unaligned_access(ENV_GET_CPU(env), addr, > READ_ACCESS_TYPE, > mmu_idx, retaddr); > @@ -193,10 +190,8 @@ WORD_TYPE helper_le_ld_name(CPUArchState *env, > target_ulong addr, > do_unaligned_access: > addr1 = addr & ~(DATA_SIZE - 1); > addr2 = addr1 + DATA_SIZE; > - /* Note the adjustment at the beginning of the function. > - Undo that for the recursion. */ > - res1 = helper_le_ld_name(env, addr1, oi, retaddr + > GETPC_ADJ); > - res2 = helper_le_ld_name(env, addr2, oi, retaddr + > GETPC_ADJ); > + res1 = helper_le_ld_name(env, addr1, oi, retaddr); > + res2 = helper_le_ld_name(env, addr2, oi, retaddr); > shift = (addr & (DATA_SIZE - 1)) * 8; > > /* Little-endian combine. */ > @@ -224,9 +219,6 @@ WORD_TYPE helper_be_ld_name(CPUArchState *env, > target_ulong addr, > uintptr_t haddr; > DATA_TYPE res; > > - /* Adjust the given return address. */ > - retaddr -= GETPC_ADJ; > - > if (a_bits > 0 && (addr & ((1 << a_bits) - 1)) != 0) { > cpu_unaligned_access(ENV_GET_CPU(env), addr, > READ_ACCESS_TYPE, > mmu_idx, retaddr); > @@ -267,10 +259,8 @@ WORD_TYPE helper_be_ld_name(CPUArchState *env, > target_ulong addr, > do_unaligned_access: > addr1 = addr & ~(DATA_SIZE - 1); > addr2 = addr1 + DATA_SIZE; > - /* Note the adjustment at the beginning of the function. > - Undo that for the recursion. */ > - res1 = helper_be_ld_name(env, addr1, oi, retaddr + > GETPC_ADJ); > - res2 = helper_be_ld_name(env, addr2, oi, retaddr + > GETPC_ADJ); > + res1 = helper_be_ld_name(env, addr1, oi, retaddr); > + res2 = helper_be_ld_name(env, addr2, oi, retaddr); > shift = (addr & (DATA_SIZE - 1)) * 8; > > /* Big-endian combine. */ > @@ -334,9 +324,6 @@ void helper_le_st_name(CPUArchState *env, > target_ulong addr, DATA_TYPE val, > int a_bits = get_alignment_bits(get_memop(oi)); > uintptr_t haddr; > > - /* Adjust the given return address. */ > - retaddr -= GETPC_ADJ; > - > if (a_bits > 0 && (addr & ((1 << a_bits) - 1)) != 0) { > cpu_unaligned_access(ENV_GET_CPU(env), addr, MMU_DATA_STORE, > mmu_idx, retaddr); > @@ -391,10 +378,8 @@ void helper_le_st_name(CPUArchState *env, > target_ulong addr, DATA_TYPE val, > for (i = 0; i < DATA_SIZE; ++i) { > /* Little-endian extract. */ > uint8_t val8 = val >> (i * 8); > - /* Note the adjustment at the beginning of the function. > - Undo that for the recursion. */ > glue(helper_ret_stb, MMUSUFFIX)(env, addr + i, val8, > - oi, retaddr + > GETPC_ADJ); > + oi, retaddr); > } > return; > } > @@ -417,9 +402,6 @@ void helper_be_st_name(CPUArchState *env, > target_ulong addr, DATA_TYPE val, > int a_bits = get_alignment_bits(get_memop(oi)); > uintptr_t haddr; > > - /* Adjust the given return address. */ > - retaddr -= GETPC_ADJ; > - > if (a_bits > 0 && (addr & ((1 << a_bits) - 1)) != 0) { > cpu_unaligned_access(ENV_GET_CPU(env), addr, MMU_DATA_STORE, > mmu_idx, retaddr); > @@ -474,10 +456,8 @@ void helper_be_st_name(CPUArchState *env, > target_ulong addr, DATA_TYPE val, > for (i = 0; i < DATA_SIZE; ++i) { > /* Big-endian extract. */ > uint8_t val8 = val >> (((DATA_SIZE - 1) * 8) - (i * 8)); > - /* Note the adjustment at the beginning of the function. > - Undo that for the recursion. */ > glue(helper_ret_stb, MMUSUFFIX)(env, addr + i, val8, > - oi, retaddr + > GETPC_ADJ); > + oi, retaddr); > } > return; > } > diff --git a/target-arm/helper.c b/target-arm/helper.c > index bdb842c..915fe0f 100644 > --- a/target-arm/helper.c > +++ b/target-arm/helper.c > @@ -8310,12 +8310,12 @@ void HELPER(dc_zva)(CPUARMState *env, > uint64_t vaddr_in) > * this purpose use the actual register value passed to > us > * so that we get the fault address right. > */ > - helper_ret_stb_mmu(env, vaddr_in, 0, oi, GETRA()); > + helper_ret_stb_mmu(env, vaddr_in, 0, oi, GETPC()); > /* Now we can populate the other TLB entries, if any */ > for (i = 0; i < maxidx; i++) { > uint64_t va = vaddr + TARGET_PAGE_SIZE * i; > if (va != (vaddr_in & TARGET_PAGE_MASK)) { > - helper_ret_stb_mmu(env, va, 0, oi, GETRA()); > + helper_ret_stb_mmu(env, va, 0, oi, GETPC()); > } > } > } > @@ -8332,7 +8332,7 @@ void HELPER(dc_zva)(CPUARMState *env, uint64_t > vaddr_in) > * bounce buffer was in use > */ > for (i = 0; i < blocklen; i++) { > - helper_ret_stb_mmu(env, vaddr + i, 0, oi, GETRA()); > + helper_ret_stb_mmu(env, vaddr + i, 0, oi, GETPC()); > } > } > #else > diff --git a/target-mips/op_helper.c b/target-mips/op_helper.c > index ea2f2ab..7af4c2f 100644 > --- a/target-mips/op_helper.c > +++ b/target-mips/op_helper.c > @@ -4122,10 +4122,10 @@ void helper_msa_ld_ ## TYPE(CPUMIPSState > *env, uint32_t wd, \ > } > > #if !defined(CONFIG_USER_ONLY) > -MSA_LD_DF(DF_BYTE, b, helper_ret_ldub_mmu, oi, GETRA()) > -MSA_LD_DF(DF_HALF, h, helper_ret_lduw_mmu, oi, GETRA()) > -MSA_LD_DF(DF_WORD, w, helper_ret_ldul_mmu, oi, GETRA()) > -MSA_LD_DF(DF_DOUBLE, d, helper_ret_ldq_mmu, oi, GETRA()) > +MSA_LD_DF(DF_BYTE, b, helper_ret_ldub_mmu, oi, GETPC()) > +MSA_LD_DF(DF_HALF, h, helper_ret_lduw_mmu, oi, GETPC()) > +MSA_LD_DF(DF_WORD, w, helper_ret_ldul_mmu, oi, GETPC()) > +MSA_LD_DF(DF_DOUBLE, d, helper_ret_ldq_mmu, oi, GETPC()) > #else > MSA_LD_DF(DF_BYTE, b, cpu_ldub_data) > MSA_LD_DF(DF_HALF, h, cpu_lduw_data) > @@ -4161,17 +4161,17 @@ void helper_msa_st_ ## TYPE(CPUMIPSState > *env, uint32_t wd, \ > int mmu_idx = cpu_mmu_index(env, false); > \ > int > i; \ > MEMOP_IDX(DF) > \ > - ensure_writable_pages(env, addr, mmu_idx, > GETRA()); \ > + ensure_writable_pages(env, addr, mmu_idx, > GETPC()); \ > for (i = 0; i < DF_ELEMENTS(DF); i++) > { \ > ST_INSN(env, addr + (i << DF), pwd->TYPE[i], > ##__VA_ARGS__); \ > } > \ > } > > #if !defined(CONFIG_USER_ONLY) > -MSA_ST_DF(DF_BYTE, b, helper_ret_stb_mmu, oi, GETRA()) > -MSA_ST_DF(DF_HALF, h, helper_ret_stw_mmu, oi, GETRA()) > -MSA_ST_DF(DF_WORD, w, helper_ret_stl_mmu, oi, GETRA()) > -MSA_ST_DF(DF_DOUBLE, d, helper_ret_stq_mmu, oi, GETRA()) > +MSA_ST_DF(DF_BYTE, b, helper_ret_stb_mmu, oi, GETPC()) > +MSA_ST_DF(DF_HALF, h, helper_ret_stw_mmu, oi, GETPC()) > +MSA_ST_DF(DF_WORD, w, helper_ret_stl_mmu, oi, GETPC()) > +MSA_ST_DF(DF_DOUBLE, d, helper_ret_stq_mmu, oi, GETPC()) > #else > MSA_ST_DF(DF_BYTE, b, cpu_stb_data) > MSA_ST_DF(DF_HALF, h, cpu_stw_data) > diff --git a/translate-all.c b/translate-all.c > index 0d47c1c..644d081 100644 > --- a/translate-all.c > +++ b/translate-all.c > @@ -299,6 +299,7 @@ bool cpu_restore_state(CPUState *cpu, uintptr_t > retaddr) > { > TranslationBlock *tb; > > + retaddr -= GETPC_ADJ; > tb = tb_find_pc(retaddr); > if (tb) { > cpu_restore_state_from_tb(cpu, tb, retaddr);
