| Message ID | 20231012085710.880440-1-mironov@fintech.ru (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/1] target/arm: Adding a check for the result of calling the CPU information check function | expand |
Sergey Mironov <mironov@fintech.ru> writes: > 6 out of 7 calls to get_arm_cp_reginfo() are checked Yes but we should be careful with asserts (vs if (ri) legs) because I don't think get_arm_cp_reginfo() guarantees it will always be successful. > > Signed-off-by: Sergey Mironov <mironov@fintech.ru> > --- > target/arm/helper.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/target/arm/helper.c b/target/arm/helper.c > index 74fbb6e1d7..cffbbaf571 100644 > --- a/target/arm/helper.c > +++ b/target/arm/helper.c > @@ -198,6 +198,7 @@ static void add_cpreg_to_list(gpointer key, gpointer opaque) > uint32_t regidx = (uintptr_t)key; > const ARMCPRegInfo *ri = get_arm_cp_reginfo(cpu->cp_regs, regidx); > > + assert(ri != NULL); /* must always succeed as we are iterating the keys of cp_regs */ assert(ri); is enough for a !NULL check. > if (!(ri->type & (ARM_CP_NO_RAW | ARM_CP_ALIAS))) { > cpu->cpreg_indexes[cpu->cpreg_array_len] = cpreg_to_kvm_id(regidx); > /* The value array need not be initialized at this point */ That said we already have an assert that would fire in init_cpregs_list(): assert(cpu->cpreg_array_len == arraylen); so I'm not sure what this is adding to ensuring the contract is kept.
On Thu, 12 Oct 2023 at 09:57, Sergey Mironov <mironov@fintech.ru> wrote: > > 6 out of 7 calls to get_arm_cp_reginfo() are checked This sounds like it's talking about a Coverity warning, though it doesn't say so. Is that the motivation here ? If so, it would be good to say so in the commit message. If not, the commit message should explain why we're making the change. That particular Coverity warning is quite prone to false positives, since it's only a heuristic. Sometimes it's useful to add an assert(), if it helps both Coverity and human readers, but not always. assert()s are also most useful if there's a comment that explains why we can assume the thing they're assuming, as Alex suggests. > Signed-off-by: Sergey Mironov <mironov@fintech.ru> > --- > target/arm/helper.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/target/arm/helper.c b/target/arm/helper.c > index 74fbb6e1d7..cffbbaf571 100644 > --- a/target/arm/helper.c > +++ b/target/arm/helper.c > @@ -198,6 +198,7 @@ static void add_cpreg_to_list(gpointer key, gpointer opaque) > uint32_t regidx = (uintptr_t)key; > const ARMCPRegInfo *ri = get_arm_cp_reginfo(cpu->cp_regs, regidx); > > + assert(ri != NULL); > if (!(ri->type & (ARM_CP_NO_RAW | ARM_CP_ALIAS))) { > cpu->cpreg_indexes[cpu->cpreg_array_len] = cpreg_to_kvm_id(regidx); > /* The value array need not be initialized at this point */ > -- > 2.31.1 thanks -- PMM
Yes, the warning was initially received in the static analyzer SVACE, the same type as Coverity. In this case, return value of a function 'get_arm_cp_reginfo' is referenced at helper.c without checking for ALL, but it is usually checked for this function (8/9).
diff --git a/target/arm/helper.c b/target/arm/helper.c index 74fbb6e1d7..cffbbaf571 100644 --- a/target/arm/helper.c +++ b/target/arm/helper.c @@ -198,6 +198,7 @@ static void add_cpreg_to_list(gpointer key, gpointer opaque) uint32_t regidx = (uintptr_t)key; const ARMCPRegInfo *ri = get_arm_cp_reginfo(cpu->cp_regs, regidx); + assert(ri != NULL); if (!(ri->type & (ARM_CP_NO_RAW | ARM_CP_ALIAS))) { cpu->cpreg_indexes[cpu->cpreg_array_len] = cpreg_to_kvm_id(regidx); /* The value array need not be initialized at this point */
6 out of 7 calls to get_arm_cp_reginfo() are checked Signed-off-by: Sergey Mironov <mironov@fintech.ru> --- target/arm/helper.c | 1 + 1 file changed, 1 insertion(+)