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

[Andrew Jones <ajones@ventanamicro.com>]

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

Thanks,
drew