On Thu, 3 Mar 2022 at 15:43, Richard Henderson
<richard.hender...@linaro.org> wrote:
>
> On 3/3/22 05:04, Peter Maydell wrote:
> >> if (USE_GUEST_BASE) {
> >> tcg_out_qemu_ld_direct(s, memop, ext, data_reg,
> >> - TCG_REG_GUEST_BASE, otype, addr_reg);
> >> + TCG_REG_GUEST_BASE, option, addr_reg);
> >> } else {
> >> tcg_out_qemu_ld_direct(s, memop, ext, data_reg,
> >> - addr_reg, TCG_TYPE_I64, TCG_REG_XZR);
> >> + addr_reg, option, TCG_REG_XZR);
> >
> > This doesn't look right. 'option' specifies how we extend the offset
> > register, but here that is XZR, which is 0 no matter how we choose
> > to extend it, whereas we aren't going to be extending the base
> > register 'addr_reg' which is what we do need to either zero or
> > sign extend. Unfortunately we can't just flip addr_reg and XZR
> > around, because XZR isn't valid as the base reg.
> >
> > Is this a pre-existing bug? If addr_reg needs zero extending
> > we won't be doing that.
>
> It's just confusing, because stuff is hidden in macros:
>
> #define USE_GUEST_BASE (guest_base != 0 || TARGET_LONG_BITS == 32)
>
> We *always* use TCG_REG_GUEST_BASE when we require an extension, so the else
> case you
> point out will always have option == 3 /* LSL #0 */.
>
> Previously I had a named constant I could use here, but I didn't create names
> for the full
> 'option' field being filled, so I thought it clearer to just pass along the
> variable.
> Would it be clearer as
>
> 3 /* LSL #0 */
>
> or with some LDST_OPTION_LSL0?
I think that using something that says it's LSL 0 (either comment as done
elsewhere in the patch, or maybe better with some symbolic constant)
would help, yes. Plus an assert or a comment that we know we don't
need to extend addr_reg in this half of the if().
thanks
-- PMM
