Re: [PATCH] disas/riscv: Further correction to LUI disassembly

[Andrew Jones]

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), ajones@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 <rbagley@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