diff mbox series

target/tricore: Use unsigned types for bitops in helper_eq_b()

Message ID 20240723151042.1396610-1-peter.maydell@linaro.org (mailing list archive)
State New, archived
Headers show
Series target/tricore: Use unsigned types for bitops in helper_eq_b() | expand

Commit Message

Peter Maydell July 23, 2024, 3:10 p.m. UTC
Coverity points out that in helper_eq_b() we have an int32_t 'msk'
and we end up shifting into its sign bit. This is OK for QEMU because
we use -fwrapv to give this well defined semantics, but when you look
at what this function is doing it's doing bit operations, so we
should be using an unsigned variable anyway. This also matches the
return type of the function.

Make 'ret' and 'msk' uint32_t.

Resolves: Coverity CID 1547758
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/tricore/op_helper.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Philippe Mathieu-Daudé July 23, 2024, 3:37 p.m. UTC | #1
On 23/7/24 17:10, Peter Maydell wrote:
> Coverity points out that in helper_eq_b() we have an int32_t 'msk'
> and we end up shifting into its sign bit. This is OK for QEMU because
> we use -fwrapv to give this well defined semantics, but when you look
> at what this function is doing it's doing bit operations, so we
> should be using an unsigned variable anyway. This also matches the
> return type of the function.
> 
> Make 'ret' and 'msk' uint32_t.
> 
> Resolves: Coverity CID 1547758
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   target/tricore/op_helper.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/target/tricore/op_helper.c b/target/tricore/op_helper.c
> index ba9c4444b39..a0d5a0da1df 100644
> --- a/target/tricore/op_helper.c
> +++ b/target/tricore/op_helper.c
> @@ -1505,8 +1505,8 @@ uint32_t helper_sub_h(CPUTriCoreState *env, target_ulong r1, target_ulong r2)
>   
>   uint32_t helper_eq_b(target_ulong r1, target_ulong r2)
>   {
> -    int32_t ret;
> -    int32_t i, msk;
> +    uint32_t ret, msk;
> +    int32_t i;

We could even reduce 'i' scope to the for().

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Richard Henderson July 23, 2024, 11:28 p.m. UTC | #2
On 7/24/24 01:10, Peter Maydell wrote:
> Coverity points out that in helper_eq_b() we have an int32_t 'msk'
> and we end up shifting into its sign bit. This is OK for QEMU because
> we use -fwrapv to give this well defined semantics, but when you look
> at what this function is doing it's doing bit operations, so we
> should be using an unsigned variable anyway. This also matches the
> return type of the function.
> 
> Make 'ret' and 'msk' uint32_t.
> 
> Resolves: Coverity CID 1547758
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   target/tricore/op_helper.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~
Peter Maydell July 29, 2024, 3:57 p.m. UTC | #3
On Tue, 23 Jul 2024 at 16:10, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> Coverity points out that in helper_eq_b() we have an int32_t 'msk'
> and we end up shifting into its sign bit. This is OK for QEMU because
> we use -fwrapv to give this well defined semantics, but when you look
> at what this function is doing it's doing bit operations, so we
> should be using an unsigned variable anyway. This also matches the
> return type of the function.
>
> Make 'ret' and 'msk' uint32_t.
>
> Resolves: Coverity CID 1547758
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---

I'll take this via my target-arm queue since I'm doing a
pullreq anyway.

thanks
-- PMM
diff mbox series

Patch

diff --git a/target/tricore/op_helper.c b/target/tricore/op_helper.c
index ba9c4444b39..a0d5a0da1df 100644
--- a/target/tricore/op_helper.c
+++ b/target/tricore/op_helper.c
@@ -1505,8 +1505,8 @@  uint32_t helper_sub_h(CPUTriCoreState *env, target_ulong r1, target_ulong r2)
 
 uint32_t helper_eq_b(target_ulong r1, target_ulong r2)
 {
-    int32_t ret;
-    int32_t i, msk;
+    uint32_t ret, msk;
+    int32_t i;
 
     ret = 0;
     msk = 0xff;