[PATCH 1/1] target/arm: Adding a check for the result of calling the CPU information check function

[PATCH 1/1] target/arm: Adding a check for the result of calling the CPU information check function

[Sergey Mironov]

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(+)

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

Re: [PATCH 1/1] target/arm: Adding a check for the result of calling the CPU information check function

[Alex Bennée]

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.

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH 1/1] target/arm: Adding a check for the result of calling the CPU information check function

[Peter Maydell]

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

Re: [PATCH 1/1] target/arm: Adding a check for the result of calling the CPU information check function

[Миронов Сергей Владимирович]

[-- Attachment #1: Type: text/plain, Size: 2074 bytes --]

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

________________________________
От: Peter Maydell <peter.maydell@linaro.org>
Отправлено: 16 октября 2023 г. 19:23
Кому: Миронов Сергей Владимирович
Копия: qemu-devel@nongnu.org; qemu-arm@nongnu.org
Тема: Re: [PATCH 1/1] target/arm: Adding a check for the result of calling the CPU information check function

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

[-- Attachment #2: Type: text/html, Size: 4261 bytes --]