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

[Richard Bagley]

post-nack, one further comment:

One could argue that this change also aligns QEMU with supporting tools (as Andrew observed), and it makes sense to merge this change into QEMU until those tools update to supporting signed decimal numbers with immediates.

As it is, both GNU assembler and the LLVM integrated assembler (or llvm-mc) throws an error with examples such as 
auipc s0, -17

On the other hand, I have only seen this problem with the output of the COLLECT plug-in, not (as yet) with QEMU execution proper.
If the problem is confined to COLLECT, perhaps the argument for aligning with other tools is not as strong.

In the meantime, I have adjusted my change locally to include AUIPC, and written a substantive, and I hope, clear commit description.
If you would like me to resubmit a patch with this updated change, please let me know.

On Thu, Mar 7, 2024 at 4:08 PM Richard Bagley <rbagley@ventanamicro.com> wrote:

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