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

[Andrew Jones]

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.

Thanks,
drew

> 
> > 
> > Thanks,
> > drew
> > 
> > 
> > > +                append(buf, tmp, buflen);
> > > +            } else if (*fmt == 'o') {
> > > +                snprintf(tmp, sizeof(tmp), "%d", dec->imm >> 12);
> > > +                append(buf, tmp, buflen);
> > >                  while (strlen(buf) < tab * 2) {
> > >                      append(buf, " ", buflen);
> > >                  }
> > > --
> > > 2.34.1
> > > 
> > >