NACK
We have established that the change is a workaround for a bug in
the assembler.
I withdraw the merge request.
Thank you for this careful review.
On Fri, Aug 11, 2023 at 4:55 AM Andrew Jones <ajo...@ventanamicro.com>
wrote:
> On Fri, Aug 11, 2023 at 10:25:52AM +0200, Andrew Jones wrote:
> > On Thu, Aug 10, 2023 at 06:27:50PM +0200, Andrew Jones wrote:
> > > On Thu, Aug 10, 2023 at 09:12:42AM -0700, Palmer Dabbelt wrote:
> > > > On Thu, 10 Aug 2023 08:31:46 PDT (-0700), ajo...@ventanamicro.com
> wrote:
> > > > > On Mon, Jul 31, 2023 at 11:33:20AM -0700, Richard Bagley wrote:
> > > > > > The recent commit 36df75a0a9 corrected one aspect of LUI
> disassembly
> > > > > > by recovering the immediate argument from the result of LUI with
> a
> > > > > > shift right by 12. However, the shift right will left-fill with
> the
> > > > > > sign. By applying a mask we recover an unsigned representation
> of the
> > > > > > 20-bit field (which includes a sign bit).
> > > > > >
> > > > > > Example:
> > > > > > 0xfffff000 >> 12 = 0xffffffff
> > > > > > 0xfffff000 >> 12 & 0xfffff = 0x000fffff
> > > > > >
> > > > > > Fixes: 36df75a0a9 ("riscv/disas: Fix disas output of upper
> immediates")
> > > > > > Signed-off-by: Richard Bagley <rbag...@ventanamicro.com>
> > > > > > ---
> > > > > > disas/riscv.c | 9 ++++++---
> > > > > > 1 file changed, 6 insertions(+), 3 deletions(-)
> > > > > >
> > > > > > diff --git a/disas/riscv.c b/disas/riscv.c
> > > > > > index 4023e3fc65..690eb4a1ac 100644
> > > > > > --- a/disas/riscv.c
> > > > > > +++ b/disas/riscv.c
> > > > > > @@ -4723,9 +4723,12 @@ static void format_inst(char *buf, size_t
> buflen, size_t tab, rv_decode *dec)
> > > > > > break;
> > > > > > case 'U':
> > > > > > fmt++;
> > > > > > - snprintf(tmp, sizeof(tmp), "%d", dec->imm >> 12);
> > > > > > - append(buf, tmp, buflen);
> > > > > > - if (*fmt == 'o') {
> > > > > > + if (*fmt == 'i') {
> > > > > > + snprintf(tmp, sizeof(tmp), "%d", dec->imm >> 12
> & 0xfffff);
> > > > >
> > > > > Why are we correcting LUI's output, but still outputting
> sign-extended
> > > > > values for AUIPC?
> > > > >
> > > > > We can't assemble 'auipc a1, 0xffffffff' or 'auipc a1, -1' without
> getting
> > > > >
> > > > > Error: lui expression not in range 0..1048575
> > > > >
> > > > > (and additionally for 0xffffffff)
> > > > >
> > > > > Error: value of 00000ffffffff000 too large for field of 4 bytes
> at 0000000000000000
> > > > >
> > > > > either.
> > > > >
> > > > > (I see that the assembler's error messages state 'lui', but I was
> trying
> > > > > 'auipc'.)
> > > > >
> > > > > I'm using as from gnu binutils 2.40.0.20230214.
> > > > >
> > > > > (And, FWIW, I agree with Richard Henderson that these instructions
> should
> > > > > accept negative values.)
> > > >
> > > > I'm kind of lost here, and you saying binutils rejects this syntax?
> If
> > > > that's the case it's probably just an oversight, can you file a bug
> in
> > > > binutils land so folks can see?
> > >
> > > Will do.
> > >
> >
> > https://sourceware.org/bugzilla/show_bug.cgi?id=30746
> >
>
> But, to try to bring this thread back to the patch under review. While the
> binutils BZ may address our preferred way of providing immediates to the
> assembler, this patch is trying to make QEMU's output consistent with
> objdump. Since objdump always outputs long immediate values as hex, then
> it doesn't need to care about negative signs. QEMU seems to prefer
> decimal, though, and so does llvm-objdump, which outputs values for these
> instructions in the range 0..1048575. So, I guess this patch is making
> QEMU consistent with llvm-objdump.
>
> Back to making suggestions for this patch...
>
> 1. The commit message should probably say something along the lines of
> what I just wrote in the preceding paragraph to better explain the
> motivation.
>
> 2. Unless I'm missing something, then this patch should also address
> AUIPC.
>
> Thanks,
> drew
>
