| Message ID | 20181130215221.20554-14-richard.henderson@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | tcg: Assorted cleanups | expand |
Richard Henderson <richard.henderson@linaro.org> writes: > This does require an extra two checks within the slow paths > to replace the assert that we're moving. > > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > tcg/aarch64/tcg-target.inc.c | 35 ++++++++++++++++++++--------------- > 1 file changed, 20 insertions(+), 15 deletions(-) > > diff --git a/tcg/aarch64/tcg-target.inc.c b/tcg/aarch64/tcg-target.inc.c > index 16f08c59c4..77f0ca4d5e 100644 > --- a/tcg/aarch64/tcg-target.inc.c > +++ b/tcg/aarch64/tcg-target.inc.c > @@ -78,20 +78,26 @@ static const int tcg_target_call_oarg_regs[1] = { > #define TCG_REG_GUEST_BASE TCG_REG_X28 > #endif > > -static inline void reloc_pc26(tcg_insn_unit *code_ptr, tcg_insn_unit *target) > +static inline bool reloc_pc26(tcg_insn_unit *code_ptr, tcg_insn_unit *target) > { > ptrdiff_t offset = target - code_ptr; > - tcg_debug_assert(offset == sextract64(offset, 0, 26)); > - /* read instruction, mask away previous PC_REL26 parameter contents, > - set the proper offset, then write back the instruction. */ > - *code_ptr = deposit32(*code_ptr, 0, 26, offset); > + if (offset == sextract64(offset, 0, 26)) { > + /* read instruction, mask away previous PC_REL26 parameter contents, > + set the proper offset, then write back the instruction. */ > + *code_ptr = deposit32(*code_ptr, 0, 26, offset); > + return true; > + } > + return false; > } > > -static inline void reloc_pc19(tcg_insn_unit *code_ptr, tcg_insn_unit *target) > +static inline bool reloc_pc19(tcg_insn_unit *code_ptr, tcg_insn_unit *target) > { > ptrdiff_t offset = target - code_ptr; > - tcg_debug_assert(offset == sextract64(offset, 0, 19)); > - *code_ptr = deposit32(*code_ptr, 5, 19, offset); > + if (offset == sextract64(offset, 0, 19)) { > + *code_ptr = deposit32(*code_ptr, 5, 19, offset); > + return true; > + } > + return false; > } > > static inline bool patch_reloc(tcg_insn_unit *code_ptr, int type, > @@ -101,15 +107,12 @@ static inline bool patch_reloc(tcg_insn_unit *code_ptr, int type, > switch (type) { > case R_AARCH64_JUMP26: > case R_AARCH64_CALL26: > - reloc_pc26(code_ptr, (tcg_insn_unit *)value); > - break; > + return reloc_pc26(code_ptr, (tcg_insn_unit *)value); > case R_AARCH64_CONDBR19: > - reloc_pc19(code_ptr, (tcg_insn_unit *)value); > - break; > + return reloc_pc19(code_ptr, (tcg_insn_unit *)value); > default: > tcg_abort(); > } > - return true; nit: the default leg could return false for the same effect Otherwise: Reviewed-by: Alex Bennée <alex.bennee@linaro.org> -- Alex Bennée
On 12/3/18 4:43 AM, Alex Bennée wrote: >> case R_AARCH64_CONDBR19: >> - reloc_pc19(code_ptr, (tcg_insn_unit *)value); >> - break; >> + return reloc_pc19(code_ptr, (tcg_insn_unit *)value); >> default: >> tcg_abort(); >> } >> - return true; > > nit: the default leg could return false for the same effect Would it be clearer changed to g_assert_not_reached()? Because I'm not intending "unknown relocation" to have the same effect as "out of range relocation". r~
Richard Henderson <richard.henderson@linaro.org> writes: > On 12/3/18 4:43 AM, Alex Bennée wrote: >>> case R_AARCH64_CONDBR19: >>> - reloc_pc19(code_ptr, (tcg_insn_unit *)value); >>> - break; >>> + return reloc_pc19(code_ptr, (tcg_insn_unit *)value); >>> default: >>> tcg_abort(); >>> } >>> - return true; >> >> nit: the default leg could return false for the same effect > > Would it be clearer changed to g_assert_not_reached()? > Because I'm not intending "unknown relocation" to have > the same effect as "out of range relocation". g_assert_not_reached would probably be better then. Is there any particular reason tcg has tcg_abort(), is it just for the slightly different report string? > > > r~ -- Alex Bennée
On 12/3/18 8:15 AM, Alex Bennée wrote: > > Richard Henderson <richard.henderson@linaro.org> writes: > >> On 12/3/18 4:43 AM, Alex Bennée wrote: >>>> case R_AARCH64_CONDBR19: >>>> - reloc_pc19(code_ptr, (tcg_insn_unit *)value); >>>> - break; >>>> + return reloc_pc19(code_ptr, (tcg_insn_unit *)value); >>>> default: >>>> tcg_abort(); >>>> } >>>> - return true; >>> >>> nit: the default leg could return false for the same effect >> >> Would it be clearer changed to g_assert_not_reached()? >> Because I'm not intending "unknown relocation" to have >> the same effect as "out of range relocation". > > g_assert_not_reached would probably be better then. > > Is there any particular reason tcg has tcg_abort(), is it just for the > slightly different report string? It's just old, pre-dating a more concerted use of glib. r~
diff --git a/tcg/aarch64/tcg-target.inc.c b/tcg/aarch64/tcg-target.inc.c index 16f08c59c4..77f0ca4d5e 100644 --- a/tcg/aarch64/tcg-target.inc.c +++ b/tcg/aarch64/tcg-target.inc.c @@ -78,20 +78,26 @@ static const int tcg_target_call_oarg_regs[1] = { #define TCG_REG_GUEST_BASE TCG_REG_X28 #endif -static inline void reloc_pc26(tcg_insn_unit *code_ptr, tcg_insn_unit *target) +static inline bool reloc_pc26(tcg_insn_unit *code_ptr, tcg_insn_unit *target) { ptrdiff_t offset = target - code_ptr; - tcg_debug_assert(offset == sextract64(offset, 0, 26)); - /* read instruction, mask away previous PC_REL26 parameter contents, - set the proper offset, then write back the instruction. */ - *code_ptr = deposit32(*code_ptr, 0, 26, offset); + if (offset == sextract64(offset, 0, 26)) { + /* read instruction, mask away previous PC_REL26 parameter contents, + set the proper offset, then write back the instruction. */ + *code_ptr = deposit32(*code_ptr, 0, 26, offset); + return true; + } + return false; } -static inline void reloc_pc19(tcg_insn_unit *code_ptr, tcg_insn_unit *target) +static inline bool reloc_pc19(tcg_insn_unit *code_ptr, tcg_insn_unit *target) { ptrdiff_t offset = target - code_ptr; - tcg_debug_assert(offset == sextract64(offset, 0, 19)); - *code_ptr = deposit32(*code_ptr, 5, 19, offset); + if (offset == sextract64(offset, 0, 19)) { + *code_ptr = deposit32(*code_ptr, 5, 19, offset); + return true; + } + return false; } static inline bool patch_reloc(tcg_insn_unit *code_ptr, int type, @@ -101,15 +107,12 @@ static inline bool patch_reloc(tcg_insn_unit *code_ptr, int type, switch (type) { case R_AARCH64_JUMP26: case R_AARCH64_CALL26: - reloc_pc26(code_ptr, (tcg_insn_unit *)value); - break; + return reloc_pc26(code_ptr, (tcg_insn_unit *)value); case R_AARCH64_CONDBR19: - reloc_pc19(code_ptr, (tcg_insn_unit *)value); - break; + return reloc_pc19(code_ptr, (tcg_insn_unit *)value); default: tcg_abort(); } - return true; } #define TCG_CT_CONST_AIMM 0x100 @@ -1387,7 +1390,8 @@ static void tcg_out_qemu_ld_slow_path(TCGContext *s, TCGLabelQemuLdst *lb) TCGMemOp opc = get_memop(oi); TCGMemOp size = opc & MO_SIZE; - reloc_pc19(lb->label_ptr[0], s->code_ptr); + bool ok = reloc_pc19(lb->label_ptr[0], s->code_ptr); + tcg_debug_assert(ok); tcg_out_mov(s, TCG_TYPE_PTR, TCG_REG_X0, TCG_AREG0); tcg_out_mov(s, TARGET_LONG_BITS == 64, TCG_REG_X1, lb->addrlo_reg); @@ -1409,7 +1413,8 @@ static void tcg_out_qemu_st_slow_path(TCGContext *s, TCGLabelQemuLdst *lb) TCGMemOp opc = get_memop(oi); TCGMemOp size = opc & MO_SIZE; - reloc_pc19(lb->label_ptr[0], s->code_ptr); + bool ok = reloc_pc19(lb->label_ptr[0], s->code_ptr); + tcg_debug_assert(ok); tcg_out_mov(s, TCG_TYPE_PTR, TCG_REG_X0, TCG_AREG0); tcg_out_mov(s, TARGET_LONG_BITS == 64, TCG_REG_X1, lb->addrlo_reg);
This does require an extra two checks within the slow paths to replace the assert that we're moving. Signed-off-by: Richard Henderson <richard.henderson@linaro.org> --- tcg/aarch64/tcg-target.inc.c | 35 ++++++++++++++++++++--------------- 1 file changed, 20 insertions(+), 15 deletions(-) -- 2.17.2