Re: [PATCH] target/riscv: Fix a uninitialized variable warning

[Daniel Henrique Barboza]

On 10/19/25 9:22 PM, Akihiko Odaki wrote:

On 2025/10/19 19:01, Daniel Henrique Barboza wrote:

On 10/19/25 5:19 AM, Akihiko Odaki wrote:

riscv_cpu_validate_v() left its variable, min_vlen, uninitialized if
no vector extension is available, causing a compiler warning. Avoid
the warning by calling g_assert_not_reached() in the case.

For the compiler point of view the variable will be left uninitialized.
In reality we'll always set it to at least '32' in validate_v(). This
is how the function is being called:

     if (cpu->cfg.ext_zve32x) {
         riscv_cpu_validate_v(env, &cpu->cfg, &local_err);
         if (local_err != NULL) {
             error_propagate(errp, local_err);
             return;
         }
     }

This means that inside the function we guarantee that min_vlen will be
at least set to 32 because cfg->ext_zve32x will always be true:

     if (riscv_has_ext(env, RVV)) {
         min_vlen = 128;
     } else if (cfg->ext_zve64x) {
         min_vlen = 64;
     } else if (cfg->ext_zve32x) {
         min_vlen = 32;
     }


To make the compiler happy and the code a bit clearer I suggest initializing
min_vlen = 32 and folding the "if (cpu->cfg.ext_zve32x)" check inside
validate_v() for an early exit. Something like this:


@@ -417,15 +417,19 @@ static void riscv_cpu_validate_misa_priv(CPURISCVState 
*env, Error **errp)
  static void riscv_cpu_validate_v(CPURISCVState *env, RISCVCPUConfig *cfg,
                                   Error **errp)
  {
-    uint32_t min_vlen;
-    uint32_t vlen = cfg->vlenb << 3;
+    uint32_t min_vlen, vlen;
+
+    if (cfg->ext_zve32x) {
+        return;
+    }
+
+    min_vlen = 32;
+    vlen = cfg->vlenb << 3;

      if (riscv_has_ext(env, RVV)) {
          min_vlen = 128;
      } else if (cfg->ext_zve64x) {
          min_vlen = 64;
-    } else if (cfg->ext_zve32x) {
-        min_vlen = 32;
      }

What about:

     if (riscv_has_ext(env, RVV)) {
         min_vlen = 128;
     } else if (cfg->ext_zve64x) {
         min_vlen = 64;
     } else if (cfg->ext_zve32x) {
         min_vlen = 32;
     } else {
         return;
     }

Always initializing min_vlen with 32 looks a bit misleading to me. min_vlen is 
inherently dependent on the RVV and zve* flags; initializing the value after 
checking the flags show that more clearly.

LGTM

And I find separating the cfg->ext_zve64x and cfg->ext_zve32x checks a bit 
awkward as they are semantically not that different. In terms of semantics, I see the 
code like as follows:

if (RVV) {
   initialize the extension parameters for RVV
} else if (zve64x) {
   initialize the extension parameters for zve64x
} else if (zve32x) {
   initialize the extension parameters for zve32x
} else {
   no vector extension is present so skip validation
}

      if (vlen > RV_VLEN_MAX || vlen < min_vlen) {
@@ -676,12 +680,10 @@ void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, 
Error **errp)
          return;
      }

-    if (cpu->cfg.ext_zve32x) {
-        riscv_cpu_validate_v(env, &cpu->cfg, &local_err);
-        if (local_err != NULL) {
-            error_propagate(errp, local_err);
-            return;
-        }
+    riscv_cpu_validate_v(env, &cpu->cfg, &local_err);
+    if (local_err != NULL) {
+        error_propagate(errp, local_err);
+        return;

Removing this duplicate cpu->cfg.ext_zve32x looks good.

      }


Note: I wonder why we're allowing settings of VLEN and so on when we do
not have RVV set. Seems like a bug ...

I think this is because the ordering of property setting is not deterministic. 
It is possible to set the vlen property before setting the v, zve64x or zve32x 
properties.

Hmmm good point. The logic predates the current structure we have ATM.


Thanks,


Daniel

Regards,
Akihiko Odaki