diff mbox series

[1/5] target/riscv/csr.c: fix deadcode in rmw_xireg()

Message ID 20250121184847.2109128-2-dbarboza@ventanamicro.com
State New
Headers show
Series target/riscv: Coverity fixes | expand

Commit Message

Daniel Henrique Barboza Jan. 21, 2025, 6:48 p.m. UTC 
Coverity found a DEADCODE issue in rmw_xireg() claiming that we can't
reach 'RISCV_EXCP_VIRT_INSTRUCTION_FAULT' at the 'done' label:

done:
    if (ret) {
        return (env->virt_enabled && virt) ?
               RISCV_EXCP_VIRT_INSTRUCTION_FAULT : RISCV_EXCP_ILLEGAL_INST;
    }
    return RISCV_EXCP_NONE;

This happens because the 'virt' flag, which is only used by 'done', is
set to 'false' and it will always remain 'false' in any condition where
we'll jump to 'done':

    switch (csrno) {
    (...)
    case CSR_VSIREG:
        isel = env->vsiselect;
        virt = true;
        break;
    default:
        goto done;
    };

'virt = true' will never reach 'done' because we have a if/else-if/else
block right before the label that will always return:

    if (xiselect_aia_range(isel)) {
        return ...
    } else if (...) {
        return ...
    } else {
        return RISCV_EXCP_ILLEGAL_INST;
    }

All this means that we can preserve the current logic by reducing the
'done' label to:

done:
    if (ret) {
        return RISCV_EXCP_ILLEGAL_INST;
    }
    return RISCV_EXCP_NONE;

The flag 'virt' is now unused. Remove it.

Fix the 'goto done' identation while we're at it.

Resolves: Coverity CID 1590359
Fixes: dc0280723d ("target/riscv: Decouple AIA processing from xiselect and xireg")
Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 target/riscv/csr.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

Comments

Alistair Francis Jan. 29, 2025, 12:53 a.m. UTC | #1 
On Wed, Jan 22, 2025 at 4:52 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> Coverity found a DEADCODE issue in rmw_xireg() claiming that we can't
> reach 'RISCV_EXCP_VIRT_INSTRUCTION_FAULT' at the 'done' label:
>
> done:
>     if (ret) {
>         return (env->virt_enabled && virt) ?
>                RISCV_EXCP_VIRT_INSTRUCTION_FAULT : RISCV_EXCP_ILLEGAL_INST;
>     }
>     return RISCV_EXCP_NONE;
>
> This happens because the 'virt' flag, which is only used by 'done', is
> set to 'false' and it will always remain 'false' in any condition where
> we'll jump to 'done':
>
>     switch (csrno) {
>     (...)
>     case CSR_VSIREG:
>         isel = env->vsiselect;
>         virt = true;
>         break;
>     default:
>         goto done;
>     };
>
> 'virt = true' will never reach 'done' because we have a if/else-if/else
> block right before the label that will always return:
>
>     if (xiselect_aia_range(isel)) {
>         return ...
>     } else if (...) {
>         return ...
>     } else {
>         return RISCV_EXCP_ILLEGAL_INST;
>     }
>
> All this means that we can preserve the current logic by reducing the
> 'done' label to:
>
> done:
>     if (ret) {
>         return RISCV_EXCP_ILLEGAL_INST;
>     }
>     return RISCV_EXCP_NONE;
>
> The flag 'virt' is now unused. Remove it.
>
> Fix the 'goto done' identation while we're at it.
>
> Resolves: Coverity CID 1590359
> Fixes: dc0280723d ("target/riscv: Decouple AIA processing from xiselect and xireg")
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  target/riscv/csr.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index afb7544f07..ab209d0cda 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -2658,7 +2658,6 @@ static RISCVException rmw_xireg(CPURISCVState *env, int csrno,
>                                  target_ulong *val, target_ulong new_val,
>                                  target_ulong wr_mask)
>  {
> -    bool virt = false;
>      int ret = -EINVAL;
>      target_ulong isel;
>
> @@ -2680,10 +2679,9 @@ static RISCVException rmw_xireg(CPURISCVState *env, int csrno,
>          break;
>      case CSR_VSIREG:
>          isel = env->vsiselect;
> -        virt = true;
>          break;
>      default:
> -         goto done;
> +        goto done;
>      };
>
>      /*
> @@ -2705,8 +2703,7 @@ static RISCVException rmw_xireg(CPURISCVState *env, int csrno,
>
>  done:
>      if (ret) {
> -        return (env->virt_enabled && virt) ?
> -               RISCV_EXCP_VIRT_INSTRUCTION_FAULT : RISCV_EXCP_ILLEGAL_INST;
> +        return RISCV_EXCP_ILLEGAL_INST;
>      }
>      return RISCV_EXCP_NONE;
>  }
> --
> 2.47.1
>
>
diff mbox series

Patch

diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index afb7544f07..ab209d0cda 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -2658,7 +2658,6 @@  static RISCVException rmw_xireg(CPURISCVState *env, int csrno,
                                 target_ulong *val, target_ulong new_val,
                                 target_ulong wr_mask)
 {
-    bool virt = false;
     int ret = -EINVAL;
     target_ulong isel;
 
@@ -2680,10 +2679,9 @@  static RISCVException rmw_xireg(CPURISCVState *env, int csrno,
         break;
     case CSR_VSIREG:
         isel = env->vsiselect;
-        virt = true;
         break;
     default:
-         goto done;
+        goto done;
     };
 
     /*
@@ -2705,8 +2703,7 @@  static RISCVException rmw_xireg(CPURISCVState *env, int csrno,
 
 done:
     if (ret) {
-        return (env->virt_enabled && virt) ?
-               RISCV_EXCP_VIRT_INSTRUCTION_FAULT : RISCV_EXCP_ILLEGAL_INST;
+        return RISCV_EXCP_ILLEGAL_INST;
     }
     return RISCV_EXCP_NONE;
 }