[PATCH 0/1] tricore: fixed faulty conditions for extr and imask

[PATCH 0/1] tricore: fixed faulty conditions for extr and imask

[David Brenken]

From: Andreas Konopik <andreas.konopik@efs-auto.de>

Hello together,

we have fixed a few conditions leading to incorrect intermediate code
generation. RCPW_IMASK, RRPW_EXTR, RRPW_EXTR_U and RRPW_IMASK invoke
undefined behavior for "pos + width > 32", which is also checked in
tcg_gen_extract_tl(). RRRW_EXTR_U invokes undefined behavior for
"width == 0", hence we removed that condition.

Andreas Konopik (1):
  tricore: fixed faulty conditions for extr and imask

 target/tricore/translate.c | 37 ++++++++++---------------------------
 1 file changed, 10 insertions(+), 27 deletions(-)

-- 
2.30.0

[PATCH 1/1] tricore: fixed faulty conditions for extr and imask

[David Brenken]

From: Andreas Konopik <andreas.konopik@efs-auto.de>

Signed-off-by: Andreas Konopik <andreas.konopik@efs-auto.de>
Signed-off-by: Georg Hofstetter <georg.hofstetter@efs-auto.de>
Signed-off-by: David Brenken <david.brenken@efs-auto.de>
---
 target/tricore/translate.c | 37 ++++++++++---------------------------
 1 file changed, 10 insertions(+), 27 deletions(-)

diff --git a/target/tricore/translate.c b/target/tricore/translate.c
index 7752630ac1..f451e085f8 100644
--- a/target/tricore/translate.c
+++ b/target/tricore/translate.c
@@ -5777,8 +5777,8 @@ static void decode_rcpw_insert(DisasContext *ctx)
     switch (op2) {
     case OPC2_32_RCPW_IMASK:
         CHECK_REG_PAIR(r2);
-        /* if pos + width > 31 undefined result */
-        if (pos + width <= 31) {
+        /* if pos + width > 32 undefined result */
+        if (pos + width <= 32) {
             tcg_gen_movi_tl(cpu_gpr_d[r2+1], ((1u << width) - 1) << pos);
             tcg_gen_movi_tl(cpu_gpr_d[r2], (const4 << pos));
         }
@@ -6999,29 +6999,16 @@ static void decode_rrpw_extract_insert(DisasContext *ctx)
 
     switch (op2) {
     case OPC2_32_RRPW_EXTR:
-        if (pos + width <= 31) {
-            /* optimize special cases */
-            if ((pos == 0) && (width == 8)) {
-                tcg_gen_ext8s_tl(cpu_gpr_d[r3], cpu_gpr_d[r1]);
-            } else if ((pos == 0) && (width == 16)) {
-                tcg_gen_ext16s_tl(cpu_gpr_d[r3], cpu_gpr_d[r1]);
-            } else {
-                tcg_gen_shli_tl(cpu_gpr_d[r3], cpu_gpr_d[r1], 32 - pos - width);
-                tcg_gen_sari_tl(cpu_gpr_d[r3], cpu_gpr_d[r3], 32 - width);
-            }
-        }
+        /* if pos + width > 32 undefined result */
+        tcg_gen_sextract_tl(cpu_gpr_d[r3], cpu_gpr_d[r1], pos, width);
         break;
     case OPC2_32_RRPW_EXTR_U:
-        if (width == 0) {
-            tcg_gen_movi_tl(cpu_gpr_d[r3], 0);
-        } else {
-            tcg_gen_shri_tl(cpu_gpr_d[r3], cpu_gpr_d[r1], pos);
-            tcg_gen_andi_tl(cpu_gpr_d[r3], cpu_gpr_d[r3], ~0u >> (32-width));
-        }
+        /* if pos + width > 32 undefined result */
+        tcg_gen_extract_tl(cpu_gpr_d[r3], cpu_gpr_d[r1], pos, width);
         break;
     case OPC2_32_RRPW_IMASK:
         CHECK_REG_PAIR(r3);
-        if (pos + width <= 31) {
+        if (pos + width <= 32) {
             tcg_gen_movi_tl(cpu_gpr_d[r3+1], ((1u << width) - 1) << pos);
             tcg_gen_shli_tl(cpu_gpr_d[r3], cpu_gpr_d[r2], pos);
         }
@@ -8320,13 +8307,9 @@ static void decode_rrrw_extract_insert(DisasContext *ctx)
         tcg_gen_sari_tl(cpu_gpr_d[r4], cpu_gpr_d[r4], 32 - width);
         break;
     case OPC2_32_RRRW_EXTR_U:
-        if (width == 0) {
-            tcg_gen_movi_tl(cpu_gpr_d[r4], 0);
-        } else {
-            tcg_gen_andi_tl(temp, cpu_gpr_d[r3], 0x1f);
-            tcg_gen_shr_tl(cpu_gpr_d[r4], cpu_gpr_d[r1], temp);
-            tcg_gen_andi_tl(cpu_gpr_d[r4], cpu_gpr_d[r4], ~0u >> (32-width));
-        }
+        tcg_gen_andi_tl(temp, cpu_gpr_d[r3], 0x1f);
+        tcg_gen_shr_tl(cpu_gpr_d[r4], cpu_gpr_d[r1], temp);
+        tcg_gen_andi_tl(cpu_gpr_d[r4], cpu_gpr_d[r4], ~0u >> (32-width));
         break;
     case OPC2_32_RRRW_IMASK:
         temp2 = tcg_temp_new();
-- 
2.30.0

Re: [PATCH 0/1] tricore: fixed faulty conditions for extr and imask

[Richard Henderson]

On 2/10/21 12:26 AM, David Brenken wrote:
> From: Andreas Konopik <andreas.konopik@efs-auto.de>
> 
> Hello together,
> 
> we have fixed a few conditions leading to incorrect intermediate code
> generation. RCPW_IMASK, RRPW_EXTR, RRPW_EXTR_U and RRPW_IMASK invoke
> undefined behavior for "pos + width > 32", which is also checked in
> tcg_gen_extract_tl(). RRRW_EXTR_U invokes undefined behavior for
> "width == 0", hence we removed that condition.

This is incorrect, because "undefined behavior" should not include a qemu abort.

You could raise a guest exception, you could treat the faulty instruction as a
nop, you could truncate the inputs to avoid the abort, you could write
0xdeadbeef to the destination.

Or you could fix the couple of faulty conditions and leave the rest of the code
as-is.


r~

RE: [PATCH 0/1] tricore: fixed faulty conditions for extr and imask

[Konopik, Andreas (EFS-GH2)]

Hi Richard,

thank you for your feedback.

> From: Richard Henderson <richard.henderson@linaro.org>
> Sent: Wednesday, February 10, 2021 20:02
> To: David Brenken <david.brenken@efs-auto.org> 
> On 2/10/21 12:26 AM, David Brenken wrote:
> > From: Andreas Konopik <andreas.konopik@efs-auto.de>
> >
> > Hello together,
> >
> > we have fixed a few conditions leading to incorrect intermediate code
> > generation. RCPW_IMASK, RRPW_EXTR, RRPW_EXTR_U and RRPW_IMASK
> invoke
> > undefined behavior for "pos + width > 32", which is also checked in
> > tcg_gen_extract_tl(). RRRW_EXTR_U invokes undefined behavior for
> > "width == 0", hence we removed that condition.
> 
> This is incorrect, because "undefined behavior" should not include a qemu
> abort.

I didn't notice that these checks terminated program execution.

> You could raise a guest exception, you could treat the faulty instruction as a
> nop, you could truncate the inputs to avoid the abort, you could write
> 0xdeadbeef to the destination.
> 
> Or you could fix the couple of faulty conditions and leave the rest of the code
> as-is.

We will submit the patch following your advice as soon as possible.

Kind regards,

Andreas

INTERNAL

Re: [PATCH 0/1] tricore: fixed faulty conditions for extr and imask

[no-reply]

Patchew URL: https://patchew.org/QEMU/20210210082650.5516-1-david.brenken@efs-auto.org/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20210210082650.5516-1-david.brenken@efs-auto.org
Subject: [PATCH 0/1] tricore: fixed faulty conditions for extr and imask

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
05ead58 tricore: fixed faulty conditions for extr and imask

=== OUTPUT BEGIN ===
ERROR: spaces required around that '-' (ctx:VxV)
#78: FILE: target/tricore/translate.c:8312:
+        tcg_gen_andi_tl(cpu_gpr_d[r4], cpu_gpr_d[r4], ~0u >> (32-width));
                                                                 ^

total: 1 errors, 0 warnings, 60 lines checked

Commit 05ead58660db (tricore: fixed faulty conditions for extr and imask) has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20210210082650.5516-1-david.brenken@efs-auto.org/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com