diff mbox series

target/m68k: avoid shift into sign bit in dump_address_map()

Message ID 20240723154207.1483665-1-peter.maydell@linaro.org (mailing list archive)
State New, archived
Headers show
Series target/m68k: avoid shift into sign bit in dump_address_map() | expand

Commit Message

Peter Maydell July 23, 2024, 3:42 p.m. UTC
Coverity complains (CID 1547592) that in dump_address_map() we take a
value stored in a signed integer variable 'i' and shift it by enough
to shift into the sign bit when we construct the value 'logical'.
This isn't a bug for QEMU because we use -fwrapv semantics, but
we can make Coverity happy by using an unsigned type for the loop
variables i, j, k in this function.

While we're changing the declaration of the variables, put them
in the for() loops so their scope is the minimum required (a style
now permitted by our coding style guide).

Resolves: Coverity CID 1547592
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
I could have just marked this as a false-positive, but it
just about seemed worth making the change overall.
---
 target/m68k/helper.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

Comments

Philippe Mathieu-Daudé July 23, 2024, 3:45 p.m. UTC | #1
On 23/7/24 17:42, Peter Maydell wrote:
> Coverity complains (CID 1547592) that in dump_address_map() we take a
> value stored in a signed integer variable 'i' and shift it by enough
> to shift into the sign bit when we construct the value 'logical'.
> This isn't a bug for QEMU because we use -fwrapv semantics, but
> we can make Coverity happy by using an unsigned type for the loop
> variables i, j, k in this function.
> 
> While we're changing the declaration of the variables, put them
> in the for() loops so their scope is the minimum required (a style
> now permitted by our coding style guide).
> 
> Resolves: Coverity CID 1547592
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> I could have just marked this as a false-positive, but it
> just about seemed worth making the change overall.
> ---
>   target/m68k/helper.c | 7 +++----
>   1 file changed, 3 insertions(+), 4 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Peter Maydell July 29, 2024, 3:59 p.m. UTC | #2
On Tue, 23 Jul 2024 at 16:42, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> Coverity complains (CID 1547592) that in dump_address_map() we take a
> value stored in a signed integer variable 'i' and shift it by enough
> to shift into the sign bit when we construct the value 'logical'.
> This isn't a bug for QEMU because we use -fwrapv semantics, but
> we can make Coverity happy by using an unsigned type for the loop
> variables i, j, k in this function.
>
> While we're changing the declaration of the variables, put them
> in the for() loops so their scope is the minimum required (a style
> now permitted by our coding style guide).
>
> Resolves: Coverity CID 1547592
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> I could have just marked this as a false-positive, but it
> just about seemed worth making the change overall.
> ---

I'll take this via target-arm.next unless you prefer
otherwise, since I'm doing a pullreq anyway.

thanks
-- PMM
diff mbox series

Patch

diff --git a/target/m68k/helper.c b/target/m68k/helper.c
index 7967ad13cbf..4c85badd5d3 100644
--- a/target/m68k/helper.c
+++ b/target/m68k/helper.c
@@ -479,7 +479,6 @@  static void print_address_zone(uint32_t logical, uint32_t physical,
 
 static void dump_address_map(CPUM68KState *env, uint32_t root_pointer)
 {
-    int i, j, k;
     int tic_size, tic_shift;
     uint32_t tib_mask;
     uint32_t tia, tib, tic;
@@ -502,19 +501,19 @@  static void dump_address_map(CPUM68KState *env, uint32_t root_pointer)
         tic_shift = 12;
         tib_mask = M68K_4K_PAGE_MASK;
     }
-    for (i = 0; i < M68K_ROOT_POINTER_ENTRIES; i++) {
+    for (unsigned i = 0; i < M68K_ROOT_POINTER_ENTRIES; i++) {
         tia = address_space_ldl(cs->as, M68K_POINTER_BASE(root_pointer) + i * 4,
                                 MEMTXATTRS_UNSPECIFIED, &txres);
         if (txres != MEMTX_OK || !M68K_UDT_VALID(tia)) {
             continue;
         }
-        for (j = 0; j < M68K_ROOT_POINTER_ENTRIES; j++) {
+        for (unsigned j = 0; j < M68K_ROOT_POINTER_ENTRIES; j++) {
             tib = address_space_ldl(cs->as, M68K_POINTER_BASE(tia) + j * 4,
                                     MEMTXATTRS_UNSPECIFIED, &txres);
             if (txres != MEMTX_OK || !M68K_UDT_VALID(tib)) {
                 continue;
             }
-            for (k = 0; k < tic_size; k++) {
+            for (unsigned k = 0; k < tic_size; k++) {
                 tic = address_space_ldl(cs->as, (tib & tib_mask) + k * 4,
                                         MEMTXATTRS_UNSPECIFIED, &txres);
                 if (txres != MEMTX_OK || !M68K_PDT_VALID(tic)) {