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 <[email protected]> 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 <[email protected]> > 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), [email protected] >> 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 <[email protected]> >> > > > > > --- >> > > > > > 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 >> >
