[Qemu-devel] [PATCH 0/2] target/arm: Coverity fixups

[Qemu-devel] [PATCH 0/2] target/arm: Coverity fixups

[Richard Henderson]

The first patch covers four related false positives.  However, 
we check the same condition twice and we certainly don't need
to that.  Plus the assert might just help documentation-wise.

The second patch covers dead code.  I believe that Coverity is
right and that there are no paths that have !is_q at this point.
Add the assert to check that is true.

The RISU tests that I ran over these insns came out clean.


r~


Richard Henderson (2):
  target/arm: Tidy conditions in handle_vec_simd_shri
  target/arm: Tidy condition in disas_simd_two_reg_misc

 target/arm/translate-a64.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

-- 
2.14.3

[Qemu-devel] [PATCH 1/2] target/arm: Tidy conditions in handle_vec_simd_shri

[Richard Henderson]

The (size > 3 && !is_q) condition is identical to the preceeding test
of bit 3 in immh; eliminate it.  For the benefit of Coverity, assert
that size is within the bounds we expect.

Fixes: Coverity CID1385846
Fixes: Coverity CID1385849
Fixes: Coverity CID1385852
Fixes: Coverity CID1385857
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/translate-a64.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
index bff4e13bf6..97950dce1a 100644
--- a/target/arm/translate-a64.c
+++ b/target/arm/translate-a64.c
@@ -9019,11 +9019,7 @@ static void handle_vec_simd_shri(DisasContext *s, bool is_q, bool is_u,
         unallocated_encoding(s);
         return;
     }
-
-    if (size > 3 && !is_q) {
-        unallocated_encoding(s);
-        return;
-    }
+    tcg_debug_assert(size <= 3);
 
     if (!fp_access_check(s)) {
         return;
-- 
2.14.3

Re: [Qemu-devel] [Qemu-arm] [PATCH 1/2] target/arm: Tidy conditions in handle_vec_simd_shri

[Alex Bennée]

Richard Henderson <richard.henderson@linaro.org> writes:

> The (size > 3 && !is_q) condition is identical to the preceeding test
> of bit 3 in immh; eliminate it.  For the benefit of Coverity, assert
> that size is within the bounds we expect.
>
> Fixes: Coverity CID1385846
> Fixes: Coverity CID1385849
> Fixes: Coverity CID1385852
> Fixes: Coverity CID1385857
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

> ---
>  target/arm/translate-a64.c | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
>
> diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
> index bff4e13bf6..97950dce1a 100644
> --- a/target/arm/translate-a64.c
> +++ b/target/arm/translate-a64.c
> @@ -9019,11 +9019,7 @@ static void handle_vec_simd_shri(DisasContext *s, bool is_q, bool is_u,
>          unallocated_encoding(s);
>          return;
>      }
> -
> -    if (size > 3 && !is_q) {
> -        unallocated_encoding(s);
> -        return;
> -    }
> +    tcg_debug_assert(size <= 3);
>
>      if (!fp_access_check(s)) {
>          return;


--
Alex Bennée

[Qemu-devel] [PATCH 2/2] target/arm: Tidy condition in disas_simd_two_reg_misc

[Richard Henderson]

Path analysis shows that size == 3 && !is_q has been eliminated.

Fixes: Coverity CID1385853
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/translate-a64.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
index 97950dce1a..6d49f30b4a 100644
--- a/target/arm/translate-a64.c
+++ b/target/arm/translate-a64.c
@@ -11473,7 +11473,11 @@ static void disas_simd_two_reg_misc(DisasContext *s, uint32_t insn)
         /* All 64-bit element operations can be shared with scalar 2misc */
         int pass;
 
-        for (pass = 0; pass < (is_q ? 2 : 1); pass++) {
+        /* Coverity claims (size == 3 && !is_q) has been eliminated
+         * from all paths leading to here.
+         */
+        tcg_debug_assert(is_q);
+        for (pass = 0; pass < 2; pass++) {
             TCGv_i64 tcg_op = tcg_temp_new_i64();
             TCGv_i64 tcg_res = tcg_temp_new_i64();
 
-- 
2.14.3

Re: [Qemu-devel] [Qemu-arm] [PATCH 2/2] target/arm: Tidy condition in disas_simd_two_reg_misc

[Alex Bennée]

Richard Henderson <richard.henderson@linaro.org> writes:

> Path analysis shows that size == 3 && !is_q has been eliminated.
>
> Fixes: Coverity CID1385853
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

> ---
>  target/arm/translate-a64.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
> index 97950dce1a..6d49f30b4a 100644
> --- a/target/arm/translate-a64.c
> +++ b/target/arm/translate-a64.c
> @@ -11473,7 +11473,11 @@ static void disas_simd_two_reg_misc(DisasContext *s, uint32_t insn)
>          /* All 64-bit element operations can be shared with scalar 2misc */
>          int pass;
>
> -        for (pass = 0; pass < (is_q ? 2 : 1); pass++) {
> +        /* Coverity claims (size == 3 && !is_q) has been eliminated
> +         * from all paths leading to here.
> +         */
> +        tcg_debug_assert(is_q);
> +        for (pass = 0; pass < 2; pass++) {
>              TCGv_i64 tcg_op = tcg_temp_new_i64();
>              TCGv_i64 tcg_res = tcg_temp_new_i64();


--
Alex Bennée

Re: [Qemu-devel] [PATCH 0/2] target/arm: Coverity fixups

[Peter Maydell]

On 1 May 2018 at 19:04, Richard Henderson <richard.henderson@linaro.org> wrote:
> The first patch covers four related false positives.  However,
> we check the same condition twice and we certainly don't need
> to that.  Plus the assert might just help documentation-wise.
>
> The second patch covers dead code.  I believe that Coverity is
> right and that there are no paths that have !is_q at this point.
> Add the assert to check that is true.
>
> The RISU tests that I ran over these insns came out clean.
>

Applied to target-arm.next, thanks.

-- PMM