* [PATCH 1/4] target/s390x: Fix CLC corrupting cc_src
2023-10-31 5:32 [PATCH 0/4] target/s390x: CC fixes Ilya Leoshkevich
@ 2023-10-31 5:32 ` Ilya Leoshkevich
2023-10-31 22:49 ` Richard Henderson
2023-10-31 5:32 ` [PATCH 2/4] tests/tcg/s390x: Test CLC with inaccessible second operand Ilya Leoshkevich
` (3 subsequent siblings)
4 siblings, 1 reply; 12+ messages in thread
From: Ilya Leoshkevich @ 2023-10-31 5:32 UTC (permalink / raw)
To: Richard Henderson, David Hildenbrand
Cc: Thomas Huth, qemu-s390x, qemu-devel, Michael Tokarev,
Ilya Leoshkevich, qemu-stable
CLC updates cc_src before accessing the second operand; if the latter
is inaccessible, the former ends up containing a bogus value.
Fix by reading cc_src into a temporary first.
Fixes: 4f7403d52b1c ("target-s390: Convert CLC")
Closes: https://gitlab.com/qemu-project/qemu/-/issues/1865
Cc: qemu-stable@nongnu.org
Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
target/s390x/tcg/translate.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/target/s390x/tcg/translate.c b/target/s390x/tcg/translate.c
index 4bae1509f50..a0d6a2a35dd 100644
--- a/target/s390x/tcg/translate.c
+++ b/target/s390x/tcg/translate.c
@@ -2007,6 +2007,7 @@ static DisasJumpType op_cksm(DisasContext *s, DisasOps *o)
static DisasJumpType op_clc(DisasContext *s, DisasOps *o)
{
int l = get_field(s, l1);
+ TCGv_i64 src;
TCGv_i32 vl;
MemOp mop;
@@ -2016,9 +2017,11 @@ static DisasJumpType op_clc(DisasContext *s, DisasOps *o)
case 4:
case 8:
mop = ctz32(l + 1) | MO_TE;
- tcg_gen_qemu_ld_tl(cc_src, o->addr1, get_mem_index(s), mop);
+ /* Do not update cc_src yet: loading cc_dst may cause an exception. */
+ src = tcg_temp_new_i64();
+ tcg_gen_qemu_ld_tl(src, o->addr1, get_mem_index(s), mop);
tcg_gen_qemu_ld_tl(cc_dst, o->in2, get_mem_index(s), mop);
- gen_op_update2_cc_i64(s, CC_OP_LTUGTU_64, cc_src, cc_dst);
+ gen_op_update2_cc_i64(s, CC_OP_LTUGTU_64, src, cc_dst);
return DISAS_NEXT;
default:
vl = tcg_constant_i32(l);
--
2.41.0
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH 1/4] target/s390x: Fix CLC corrupting cc_src
2023-10-31 5:32 ` [PATCH 1/4] target/s390x: Fix CLC corrupting cc_src Ilya Leoshkevich
@ 2023-10-31 22:49 ` Richard Henderson
0 siblings, 0 replies; 12+ messages in thread
From: Richard Henderson @ 2023-10-31 22:49 UTC (permalink / raw)
To: Ilya Leoshkevich, David Hildenbrand
Cc: Thomas Huth, qemu-s390x, qemu-devel, Michael Tokarev, qemu-stable
On 10/30/23 22:32, Ilya Leoshkevich wrote:
> CLC updates cc_src before accessing the second operand; if the latter
> is inaccessible, the former ends up containing a bogus value.
>
> Fix by reading cc_src into a temporary first.
>
> Fixes: 4f7403d52b1c ("target-s390: Convert CLC")
> Closes: https://gitlab.com/qemu-project/qemu/-/issues/1865
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
> target/s390x/tcg/translate.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
r~
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 2/4] tests/tcg/s390x: Test CLC with inaccessible second operand
2023-10-31 5:32 [PATCH 0/4] target/s390x: CC fixes Ilya Leoshkevich
2023-10-31 5:32 ` [PATCH 1/4] target/s390x: Fix CLC corrupting cc_src Ilya Leoshkevich
@ 2023-10-31 5:32 ` Ilya Leoshkevich
2023-10-31 22:53 ` Richard Henderson
2023-10-31 5:32 ` [PATCH 3/4] target/s390x: Fix LAALG not updating cc_src Ilya Leoshkevich
` (2 subsequent siblings)
4 siblings, 1 reply; 12+ messages in thread
From: Ilya Leoshkevich @ 2023-10-31 5:32 UTC (permalink / raw)
To: Richard Henderson, David Hildenbrand
Cc: Thomas Huth, qemu-s390x, qemu-devel, Michael Tokarev,
Ilya Leoshkevich
Add a small test to prevent regressions.
Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
tests/tcg/s390x/Makefile.target | 1 +
tests/tcg/s390x/clc.c | 48 +++++++++++++++++++++++++++++++++
2 files changed, 49 insertions(+)
create mode 100644 tests/tcg/s390x/clc.c
diff --git a/tests/tcg/s390x/Makefile.target b/tests/tcg/s390x/Makefile.target
index 826f0a18e43..ccd4f4e68de 100644
--- a/tests/tcg/s390x/Makefile.target
+++ b/tests/tcg/s390x/Makefile.target
@@ -41,6 +41,7 @@ TESTS+=larl
TESTS+=mdeb
TESTS+=cgebra
TESTS+=clgebr
+TESTS+=clc
cdsg: CFLAGS+=-pthread
cdsg: LDFLAGS+=-pthread
diff --git a/tests/tcg/s390x/clc.c b/tests/tcg/s390x/clc.c
new file mode 100644
index 00000000000..e14189bd75e
--- /dev/null
+++ b/tests/tcg/s390x/clc.c
@@ -0,0 +1,48 @@
+/*
+ * Test the CLC instruction.
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+#include <assert.h>
+#include <signal.h>
+#include <stdlib.h>
+#include <string.h>
+#include <unistd.h>
+
+static void handle_sigsegv(int sig, siginfo_t *info, void *ucontext)
+{
+ mcontext_t *mcontext = &((ucontext_t *)ucontext)->uc_mcontext;
+ if (mcontext->gregs[0] != 600) {
+ write(STDERR_FILENO, "bad r0\n", 7);
+ _exit(EXIT_FAILURE);
+ }
+ if (((mcontext->psw.mask >> 44) & 3) != 1) {
+ write(STDERR_FILENO, "bad cc\n", 7);
+ _exit(EXIT_FAILURE);
+ }
+ _exit(EXIT_SUCCESS);
+}
+
+int main(void)
+{
+ register unsigned long r0 asm("r0");
+ unsigned long mem = 42, rhs = 500;
+ struct sigaction act;
+ int err;
+
+ memset(&act, 0, sizeof(act));
+ act.sa_sigaction = handle_sigsegv;
+ act.sa_flags = SA_SIGINFO;
+ err = sigaction(SIGSEGV, &act, NULL);
+ assert(err == 0);
+
+ r0 = 100;
+ asm("algr %[r0],%[rhs]\n"
+ "clc 0(8,%[mem]),0(0)\n" /* The 2nd operand will cause a SEGV. */
+ : [r0] "+r" (r0)
+ : [mem] "r" (&mem)
+ , [rhs] "r" (rhs)
+ : "cc", "memory");
+
+ return EXIT_FAILURE;
+}
--
2.41.0
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH 2/4] tests/tcg/s390x: Test CLC with inaccessible second operand
2023-10-31 5:32 ` [PATCH 2/4] tests/tcg/s390x: Test CLC with inaccessible second operand Ilya Leoshkevich
@ 2023-10-31 22:53 ` Richard Henderson
2023-11-06 9:20 ` Ilya Leoshkevich
0 siblings, 1 reply; 12+ messages in thread
From: Richard Henderson @ 2023-10-31 22:53 UTC (permalink / raw)
To: Ilya Leoshkevich, David Hildenbrand
Cc: Thomas Huth, qemu-s390x, qemu-devel, Michael Tokarev
On 10/30/23 22:32, Ilya Leoshkevich wrote:
> +int main(void)
> +{
> + register unsigned long r0 asm("r0");
> + unsigned long mem = 42, rhs = 500;
> + struct sigaction act;
> + int err;
> +
> + memset(&act, 0, sizeof(act));
> + act.sa_sigaction = handle_sigsegv;
> + act.sa_flags = SA_SIGINFO;
> + err = sigaction(SIGSEGV, &act, NULL);
> + assert(err == 0);
> +
> + r0 = 100;
> + asm("algr %[r0],%[rhs]\n"
> + "clc 0(8,%[mem]),0(0)\n" /* The 2nd operand will cause a SEGV. */
> + : [r0] "+r" (r0)
> + : [mem] "r" (&mem)
> + , [rhs] "r" (rhs)
> + : "cc", "memory");
> +
You could just as easily set cc based on CHI or something to avoid hard-coding r0, or even
clobbering an output register at all.
But I guess there's little point bike shedding this too much...
r~
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH 2/4] tests/tcg/s390x: Test CLC with inaccessible second operand
2023-10-31 22:53 ` Richard Henderson
@ 2023-11-06 9:20 ` Ilya Leoshkevich
0 siblings, 0 replies; 12+ messages in thread
From: Ilya Leoshkevich @ 2023-11-06 9:20 UTC (permalink / raw)
To: Richard Henderson, David Hildenbrand
Cc: Thomas Huth, qemu-s390x, qemu-devel, Michael Tokarev
On Tue, 2023-10-31 at 15:53 -0700, Richard Henderson wrote:
> On 10/30/23 22:32, Ilya Leoshkevich wrote:
> > +int main(void)
> > +{
> > + register unsigned long r0 asm("r0");
> > + unsigned long mem = 42, rhs = 500;
> > + struct sigaction act;
> > + int err;
> > +
> > + memset(&act, 0, sizeof(act));
> > + act.sa_sigaction = handle_sigsegv;
> > + act.sa_flags = SA_SIGINFO;
> > + err = sigaction(SIGSEGV, &act, NULL);
> > + assert(err == 0);
> > +
> > + r0 = 100;
> > + asm("algr %[r0],%[rhs]\n"
> > + "clc 0(8,%[mem]),0(0)\n" /* The 2nd operand will cause a
> > SEGV. */
> > + : [r0] "+r" (r0)
> > + : [mem] "r" (&mem)
> > + , [rhs] "r" (rhs)
> > + : "cc", "memory");
> > +
>
> You could just as easily set cc based on CHI or something to avoid
> hard-coding r0, or even
> clobbering an output register at all.
The point of hardcoding r0 is rather to be able to check its value in
handle_sigsegv(). While this was not buggy, I still wanted to make sure
that the updated value is "committed" despite SEGV.
>
> But I guess there's little point bike shedding this too much...
>
> r~
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 3/4] target/s390x: Fix LAALG not updating cc_src
2023-10-31 5:32 [PATCH 0/4] target/s390x: CC fixes Ilya Leoshkevich
2023-10-31 5:32 ` [PATCH 1/4] target/s390x: Fix CLC corrupting cc_src Ilya Leoshkevich
2023-10-31 5:32 ` [PATCH 2/4] tests/tcg/s390x: Test CLC with inaccessible second operand Ilya Leoshkevich
@ 2023-10-31 5:32 ` Ilya Leoshkevich
2023-10-31 22:57 ` Richard Henderson
2023-10-31 5:32 ` [PATCH 4/4] tests/tcg/s390x: Test LAALG with negative cc_src Ilya Leoshkevich
2023-10-31 8:38 ` [PATCH 0/4] target/s390x: CC fixes David Hildenbrand
4 siblings, 1 reply; 12+ messages in thread
From: Ilya Leoshkevich @ 2023-10-31 5:32 UTC (permalink / raw)
To: Richard Henderson, David Hildenbrand
Cc: Thomas Huth, qemu-s390x, qemu-devel, Michael Tokarev,
Ilya Leoshkevich, qemu-stable
LAALG uses op_laa() and cout_addu64(). The latter expects cc_src to be
set, but the former does not do it. This can lead to assertion failures
if something sets cc_src to neither 0 nor 1 before.
Fix by setting cc_src in op_laa().
Fixes: 4dba4d6fef61 ("target/s390x: Use atomic operations for LOAD AND OP")
Cc: qemu-stable@nongnu.org
Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
target/s390x/tcg/translate.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/target/s390x/tcg/translate.c b/target/s390x/tcg/translate.c
index a0d6a2a35dd..db386ea94d8 100644
--- a/target/s390x/tcg/translate.c
+++ b/target/s390x/tcg/translate.c
@@ -2684,7 +2684,8 @@ static DisasJumpType op_laa(DisasContext *s, DisasOps *o)
tcg_gen_atomic_fetch_add_i64(o->in2, o->in2, o->in1, get_mem_index(s),
s->insn->data | MO_ALIGN);
/* However, we need to recompute the addition for setting CC. */
- tcg_gen_add_i64(o->out, o->in1, o->in2);
+ tcg_gen_movi_i64(cc_src, 0);
+ tcg_gen_add2_i64(o->out, cc_src, o->in1, cc_src, o->in2, cc_src);
return DISAS_NEXT;
}
--
2.41.0
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH 3/4] target/s390x: Fix LAALG not updating cc_src
2023-10-31 5:32 ` [PATCH 3/4] target/s390x: Fix LAALG not updating cc_src Ilya Leoshkevich
@ 2023-10-31 22:57 ` Richard Henderson
0 siblings, 0 replies; 12+ messages in thread
From: Richard Henderson @ 2023-10-31 22:57 UTC (permalink / raw)
To: Ilya Leoshkevich, David Hildenbrand
Cc: Thomas Huth, qemu-s390x, qemu-devel, Michael Tokarev, qemu-stable
On 10/30/23 22:32, Ilya Leoshkevich wrote:
> LAALG uses op_laa() and cout_addu64(). The latter expects cc_src to be
> set, but the former does not do it. This can lead to assertion failures
> if something sets cc_src to neither 0 nor 1 before.
>
> Fix by setting cc_src in op_laa().
>
> Fixes: 4dba4d6fef61 ("target/s390x: Use atomic operations for LOAD AND OP")
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
> target/s390x/tcg/translate.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/target/s390x/tcg/translate.c b/target/s390x/tcg/translate.c
> index a0d6a2a35dd..db386ea94d8 100644
> --- a/target/s390x/tcg/translate.c
> +++ b/target/s390x/tcg/translate.c
> @@ -2684,7 +2684,8 @@ static DisasJumpType op_laa(DisasContext *s, DisasOps *o)
> tcg_gen_atomic_fetch_add_i64(o->in2, o->in2, o->in1, get_mem_index(s),
> s->insn->data | MO_ALIGN);
> /* However, we need to recompute the addition for setting CC. */
> - tcg_gen_add_i64(o->out, o->in1, o->in2);
> + tcg_gen_movi_i64(cc_src, 0);
> + tcg_gen_add2_i64(o->out, cc_src, o->in1, cc_src, o->in2, cc_src);
> return DISAS_NEXT;
> }
>
This is wrong, or at least not ideal -- op_laa is not *only* used with cout_addu64.
I think this would be better with a second version of op_laa.
r~
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 4/4] tests/tcg/s390x: Test LAALG with negative cc_src
2023-10-31 5:32 [PATCH 0/4] target/s390x: CC fixes Ilya Leoshkevich
` (2 preceding siblings ...)
2023-10-31 5:32 ` [PATCH 3/4] target/s390x: Fix LAALG not updating cc_src Ilya Leoshkevich
@ 2023-10-31 5:32 ` Ilya Leoshkevich
2023-10-31 8:38 ` [PATCH 0/4] target/s390x: CC fixes David Hildenbrand
4 siblings, 0 replies; 12+ messages in thread
From: Ilya Leoshkevich @ 2023-10-31 5:32 UTC (permalink / raw)
To: Richard Henderson, David Hildenbrand
Cc: Thomas Huth, qemu-s390x, qemu-devel, Michael Tokarev,
Ilya Leoshkevich
Add a small test to prevent regressions.
Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
tests/tcg/s390x/Makefile.target | 1 +
tests/tcg/s390x/laalg.c | 27 +++++++++++++++++++++++++++
2 files changed, 28 insertions(+)
create mode 100644 tests/tcg/s390x/laalg.c
diff --git a/tests/tcg/s390x/Makefile.target b/tests/tcg/s390x/Makefile.target
index ccd4f4e68de..a476547b659 100644
--- a/tests/tcg/s390x/Makefile.target
+++ b/tests/tcg/s390x/Makefile.target
@@ -42,6 +42,7 @@ TESTS+=mdeb
TESTS+=cgebra
TESTS+=clgebr
TESTS+=clc
+TESTS+=laalg
cdsg: CFLAGS+=-pthread
cdsg: LDFLAGS+=-pthread
diff --git a/tests/tcg/s390x/laalg.c b/tests/tcg/s390x/laalg.c
new file mode 100644
index 00000000000..797d168bb16
--- /dev/null
+++ b/tests/tcg/s390x/laalg.c
@@ -0,0 +1,27 @@
+/*
+ * Test the LAALG instruction.
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+#include <assert.h>
+#include <stdlib.h>
+
+int main(void)
+{
+ unsigned long cc = 0, op1, op2 = 40, op3 = 2;
+
+ asm("slgfi %[cc],1\n" /* Set cc_src = -1. */
+ "laalg %[op1],%[op3],%[op2]\n"
+ "ipm %[cc]"
+ : [cc] "+r" (cc)
+ , [op1] "=r" (op1)
+ , [op2] "+T" (op2)
+ : [op3] "r" (op3)
+ : "cc");
+
+ assert(cc == 0xffffffff10ffffff);
+ assert(op1 == 40);
+ assert(op2 == 42);
+
+ return EXIT_SUCCESS;
+}
--
2.41.0
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH 0/4] target/s390x: CC fixes
2023-10-31 5:32 [PATCH 0/4] target/s390x: CC fixes Ilya Leoshkevich
` (3 preceding siblings ...)
2023-10-31 5:32 ` [PATCH 4/4] tests/tcg/s390x: Test LAALG with negative cc_src Ilya Leoshkevich
@ 2023-10-31 8:38 ` David Hildenbrand
2023-11-03 16:44 ` Ilya Leoshkevich
4 siblings, 1 reply; 12+ messages in thread
From: David Hildenbrand @ 2023-10-31 8:38 UTC (permalink / raw)
To: Ilya Leoshkevich, Richard Henderson
Cc: Thomas Huth, qemu-s390x, qemu-devel, Michael Tokarev
On 31.10.23 06:32, Ilya Leoshkevich wrote:
> Hi,
>
> This series fixes two issues with updating CC. David was suggesting a
> bigger rewrite [1], but I did not dare do this (yet). Instead, these
I started coding that up but was distracted by other things; last time I
looked at that, I concluded that the way we are calculating the carry in
not suitable when we're doing two additions (like ADD LOGICAL WITH CARRY).
In addition, the way cout_addu32/cout_addu64 relies on cc_src/cc_dst is
wrong in some occurrences.
> are targeted fixes: patch 1 helps with installing Fedora, and patch 3
> addresses something I noticed when reviewing the code. Patches 2 and 4
> are tests.
I assume you are only scratching the surface with your fixes and we
might be better off just doing CC_OP_ADDU / CC_OP_SUBU like CC_OP_ADD_32
/ CC_OP_ADD_64 / ... to fix all issues involved.
>
> Best regards,
> Ilya
>
> [1] https://gitlab.com/qemu-project/qemu/-/issues/1865#note_1545060658
>
> Ilya Leoshkevich (4):
> target/s390x: Fix CLC corrupting cc_src
> tests/tcg/s390x: Test CLC with inaccessible second operand
> target/s390x: Fix LAALG not updating cc_src
> tests/tcg/s390x: Test LAALG with negative cc_src
>
> target/s390x/tcg/translate.c | 10 ++++---
> tests/tcg/s390x/Makefile.target | 2 ++
> tests/tcg/s390x/clc.c | 48 +++++++++++++++++++++++++++++++++
> tests/tcg/s390x/laalg.c | 27 +++++++++++++++++++
> 4 files changed, 84 insertions(+), 3 deletions(-)
> create mode 100644 tests/tcg/s390x/clc.c
> create mode 100644 tests/tcg/s390x/laalg.c
>
--
Cheers,
David / dhildenb
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH 0/4] target/s390x: CC fixes
2023-10-31 8:38 ` [PATCH 0/4] target/s390x: CC fixes David Hildenbrand
@ 2023-11-03 16:44 ` Ilya Leoshkevich
2023-11-03 17:02 ` David Hildenbrand
0 siblings, 1 reply; 12+ messages in thread
From: Ilya Leoshkevich @ 2023-11-03 16:44 UTC (permalink / raw)
To: David Hildenbrand, Richard Henderson
Cc: Thomas Huth, qemu-s390x, qemu-devel, Michael Tokarev
On Tue, 2023-10-31 at 09:38 +0100, David Hildenbrand wrote:
> On 31.10.23 06:32, Ilya Leoshkevich wrote:
> > Hi,
> >
> > This series fixes two issues with updating CC. David was suggesting
> > a
> > bigger rewrite [1], but I did not dare do this (yet). Instead,
> > these
>
> I started coding that up but was distracted by other things; last
> time I
> looked at that, I concluded that the way we are calculating the carry
> in
> not suitable when we're doing two additions (like ADD LOGICAL WITH
> CARRY).
Do you per chance remember any details? IIUC the code in question is:
static DisasJumpType op_addc64(DisasContext *s, DisasOps *o)
{
compute_carry(s);
TCGv_i64 zero = tcg_constant_i64(0);
tcg_gen_add2_i64(o->out, cc_src, o->in1, zero, cc_src, zero);
tcg_gen_add2_i64(o->out, cc_src, o->out, cc_src, o->in2, zero);
return DISAS_NEXT;
}
This looks correct to me, because the 128-bit result of the first
addition is passed fully to the second addition, and the upper half is
always either 0 or 1.
I played with chaining ADD-LOGICAL-WITH-CARRYs with various inputs, and
could not produce any wrong calculations in the emulation.
Best regards,
Ilya
[...]
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH 0/4] target/s390x: CC fixes
2023-11-03 16:44 ` Ilya Leoshkevich
@ 2023-11-03 17:02 ` David Hildenbrand
0 siblings, 0 replies; 12+ messages in thread
From: David Hildenbrand @ 2023-11-03 17:02 UTC (permalink / raw)
To: Ilya Leoshkevich, Richard Henderson
Cc: Thomas Huth, qemu-s390x, qemu-devel, Michael Tokarev
On 03.11.23 17:44, Ilya Leoshkevich wrote:
> On Tue, 2023-10-31 at 09:38 +0100, David Hildenbrand wrote:
>> On 31.10.23 06:32, Ilya Leoshkevich wrote:
>>> Hi,
>>>
>>> This series fixes two issues with updating CC. David was suggesting
>>> a
>>> bigger rewrite [1], but I did not dare do this (yet). Instead,
>>> these
>>
>> I started coding that up but was distracted by other things; last
>> time I
>> looked at that, I concluded that the way we are calculating the carry
>> in
>> not suitable when we're doing two additions (like ADD LOGICAL WITH
>> CARRY).
>
> Do you per chance remember any details? IIUC the code in question is:
Unfortunately, I don't. I thought there would be a case where we could
overflow twice, and result in a carry value of 2. Or some other weird
corner case where the result would not be expressive.
Maybe I was daydreaming, let me see if I can re-discover what I found
(should have taken notes but was just briefly looking at this).
If not, your fixes might be just good enough.
--
Cheers,
David / dhildenb
^ permalink raw reply [flat|nested] 12+ messages in thread