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

[Alistair Francis]

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
>
>