Re: [PATCH 1/2] sim: riscv: fix a divw division by -1 bug

[Andrew Burgess <aburgess@redhat.com>]

Bernd Edlinger <bernd.edlinger@hotmail.de> writes:

> There is a bug in divw for riscv64 target
> with dividing by -1, the result was always 0,
> when it should in fact be sign-extended -dividend.
> It did not affect the rv32 target, because
> these instructions are rv64 only.
> Since 1 << 31 is an integer overflow this avoids
> an undefined behaviour bug at the same time.
>
> This caused test failures in the gcc testsuite like:
>
> FAIL: gcc.c-torture/execute/arith-rand-ll.c   -O0  execution test
> FAIL: gcc.c-torture/execute/arith-rand-ll.c   -O1  execution test
> FAIL: gcc.c-torture/execute/arith-rand-ll.c   -O2  execution test
> FAIL: gcc.c-torture/execute/arith-rand-ll.c   -O3  execution test
> FAIL: gcc.c-torture/execute/arith-rand.c   -O0  execution test
> FAIL: gcc.c-torture/execute/arith-rand.c   -O1  execution test
> FAIL: gcc.c-torture/execute/arith-rand.c   -O2  execution test
> FAIL: gcc.c-torture/execute/arith-rand.c   -O3  execution test
> ...
> ---
>  sim/riscv/sim-main.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/sim/riscv/sim-main.c b/sim/riscv/sim-main.c
> index c8adb65139b..515ff835223 100644
> --- a/sim/riscv/sim-main.c
> +++ b/sim/riscv/sim-main.c
> @@ -724,7 +724,7 @@ execute_m (SIM_CPU *cpu, unsigned_word iw, const struct riscv_opcode *op)
>  		  rd_name, rs1_name, rs2_name, rd_name, rs1_name, rs2_name);
>        RISCV_ASSERT_RV64 (cpu, "insn: %s", op->name);
>        if (EXTEND32 (riscv_cpu->regs[rs2]) == -1)
> -	tmp = 1 << 31;
> +	tmp = EXTEND32 (-(uint32_t) riscv_cpu->regs[rs1]);

I think this part of the if tree is no longer needed, the following...

>        else if (EXTEND32 (riscv_cpu->regs[rs2]))
>  	tmp = EXTEND32 (riscv_cpu->regs[rs1]) / EXTEND32 (riscv_cpu->regs[rs2]);

... should give the correct answer, right?

Additionally, we should be adding tests for these bugs as we fix them.

Below is what I wrote in order to check this patch, feel free to include
this with a v2 of this patch, or do your own thing if you prefer.

Thanks,
Andrew

---

diff --git a/sim/testsuite/riscv/divw.s b/sim/testsuite/riscv/divw.s
new file mode 100644
index 00000000000..ed130358607
--- /dev/null
+++ b/sim/testsuite/riscv/divw.s
@@ -0,0 +1,31 @@
+# Basic divw tests.
+# mach: riscv64
+# sim(riscv64): --model RV64GC
+# ld(riscv64): -m elf64lriscv
+# as(riscv64): -march=rv64gc
+
+.include "testutils.inc"
+
+	.macro test_divw  dividend, divisor, answer
+	# Setup inputs.
+	li	a1, \dividend
+	li	a2, \divisor
+
+	# Do the divide.
+	divw	a3, a1, a2
+
+	# Check answer is correct.
+	li	a4, \answer
+	bne	a3, a4, test_failed
+	.endm
+
+
+	start
+
+	test_divw	1, -1, -1
+	test_divw	6, 2, 3
+
+	pass
+
+test_failed:
+	fail