[v1] fix uninitialized variable warning

[PATCH 1/6] target/xtensa: fix uninitialized variable warning

Posted by Chen Qun 5 years, 3 months ago

The compiler cannot determine whether the return values of the xtensa_operand_is_register(isa, opc, opnd)
 and xtensa_operand_is_visible(isa, opc, opnd) functions are the same.
So,it assumes that 'rf' is not assigned, but it's used.

The compiler showed warning:
target/xtensa/translate.c: In function ‘disas_xtensa_insn’:
target/xtensa/translate.c:985:43: warning: ‘rf’ may be used uninitialized in this function [-Wmaybe-uninitialized]
  985 |                     arg[vopnd].num_bits = xtensa_regfile_num_bits(isa, rf);
      |                                           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Add a default value for 'rf' to prevented the warning.

Reported-by: Euler Robot <euler.robot@huawei.com>
Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
---
Cc: Max Filippov <jcmvbkbc@gmail.com>
---
 target/xtensa/translate.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/xtensa/translate.c b/target/xtensa/translate.c
index 944a157747..eea851bbe7 100644
--- a/target/xtensa/translate.c
+++ b/target/xtensa/translate.c
@@ -953,7 +953,7 @@ static void disas_xtensa_insn(CPUXtensaState *env, DisasContext *dc)
 
         for (opnd = vopnd = 0; opnd < opnds; ++opnd) {
             void **register_file = NULL;
-            xtensa_regfile rf;
+            xtensa_regfile rf = -1;
 
             if (xtensa_operand_is_register(isa, opc, opnd)) {
                 rf = xtensa_operand_regfile(isa, opc, opnd);
-- 
2.27.0

Re: [PATCH 1/6] target/xtensa: fix uninitialized variable warning

Posted by Philippe Mathieu-Daudé 5 years, 3 months ago

On 11/3/20 2:52 AM, Chen Qun wrote:
> The compiler cannot determine whether the return values of the xtensa_operand_is_register(isa, opc, opnd)
>  and xtensa_operand_is_visible(isa, opc, opnd) functions are the same.
> So,it assumes that 'rf' is not assigned, but it's used.
> 
> The compiler showed warning:
> target/xtensa/translate.c: In function ‘disas_xtensa_insn’:
> target/xtensa/translate.c:985:43: warning: ‘rf’ may be used uninitialized in this function [-Wmaybe-uninitialized]
>   985 |                     arg[vopnd].num_bits = xtensa_regfile_num_bits(isa, rf);
>       |                                           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> Add a default value for 'rf' to prevented the warning.
> 
> Reported-by: Euler Robot <euler.robot@huawei.com>
> Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
> ---
> Cc: Max Filippov <jcmvbkbc@gmail.com>
> ---
>  target/xtensa/translate.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/target/xtensa/translate.c b/target/xtensa/translate.c
> index 944a157747..eea851bbe7 100644
> --- a/target/xtensa/translate.c
> +++ b/target/xtensa/translate.c
> @@ -953,7 +953,7 @@ static void disas_xtensa_insn(CPUXtensaState *env, DisasContext *dc)
>  
>          for (opnd = vopnd = 0; opnd < opnds; ++opnd) {
>              void **register_file = NULL;
> -            xtensa_regfile rf;
> +            xtensa_regfile rf = -1;

NAck (code smells).

Deferring to Max, but possible fix:

-- >8 --
@@ -953,10 +953,9 @@ static void disas_xtensa_insn(CPUXtensaState *env,
DisasContext *dc)

         for (opnd = vopnd = 0; opnd < opnds; ++opnd) {
             void **register_file = NULL;
-            xtensa_regfile rf;
+            xtensa_regfile rf = xtensa_operand_regfile(isa, opc, opnd);

             if (xtensa_operand_is_register(isa, opc, opnd)) {
-                rf = xtensa_operand_regfile(isa, opc, opnd);
                 register_file = dc->config->regfile[rf];

                 if (rf == dc->config->a_regfile) {
---

Re: [PATCH 1/6] target/xtensa: fix uninitialized variable warning

Posted by Max Filippov 5 years, 3 months ago

On Mon, Nov 2, 2020 at 5:52 PM Chen Qun <kuhn.chenqun@huawei.com> wrote:
>
> The compiler cannot determine whether the return values of the xtensa_operand_is_register(isa, opc, opnd)
>  and xtensa_operand_is_visible(isa, opc, opnd) functions are the same.

It doesn't have to because 1) they definitely are not the same, but
2) it doesn't matter.

> So,it assumes that 'rf' is not assigned, but it's used.

The assumption is wrong. rf is used under the 'if (register_file)'
condition and register_file is initialized to NULL and then set
to something non-NULL based on the value of rf here:

958             if (xtensa_operand_is_register(isa, opc, opnd)) {
959                 rf = xtensa_operand_regfile(isa, opc, opnd);
960                 register_file = dc->config->regfile[rf];

> The compiler showed warning:
> target/xtensa/translate.c: In function ‘disas_xtensa_insn’:
> target/xtensa/translate.c:985:43: warning: ‘rf’ may be used uninitialized in this function [-Wmaybe-uninitialized]
>   985 |                     arg[vopnd].num_bits = xtensa_regfile_num_bits(isa, rf);
>       |                                           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> Add a default value for 'rf' to prevented the warning.

I don't see it doing default build with gcc 8.3. But then I don't see
-Wmaybe-uninitialized in the compiler command line either.

> Reported-by: Euler Robot <euler.robot@huawei.com>
> Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
> ---
> Cc: Max Filippov <jcmvbkbc@gmail.com>
> ---
>  target/xtensa/translate.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/target/xtensa/translate.c b/target/xtensa/translate.c
> index 944a157747..eea851bbe7 100644
> --- a/target/xtensa/translate.c
> +++ b/target/xtensa/translate.c
> @@ -953,7 +953,7 @@ static void disas_xtensa_insn(CPUXtensaState *env, DisasContext *dc)
>
>          for (opnd = vopnd = 0; opnd < opnds; ++opnd) {
>              void **register_file = NULL;
> -            xtensa_regfile rf;
> +            xtensa_regfile rf = -1;

Please use XTENSA_UNDEFINED instead if you still think this
is worth changing.

-- 
Thanks.
-- Max

RE: [PATCH 1/6] target/xtensa: fix uninitialized variable warning

Posted by Chenqun (kuhn) 5 years, 3 months ago

> -----Original Message-----
> From: Max Filippov [mailto:jcmvbkbc@gmail.com]
> Sent: Tuesday, November 3, 2020 5:22 PM
> To: Chenqun (kuhn) <kuhn.chenqun@huawei.com>
> Cc: qemu-devel <qemu-devel@nongnu.org>; QEMU Trivial
> <qemu-trivial@nongnu.org>; Zhanghailiang
> <zhang.zhanghailiang@huawei.com>; ganqixin <ganqixin@huawei.com>; Euler
> Robot <euler.robot@huawei.com>
> Subject: Re: [PATCH 1/6] target/xtensa: fix uninitialized variable warning
> 
> On Mon, Nov 2, 2020 at 5:52 PM Chen Qun <kuhn.chenqun@huawei.com>
> wrote:
> >
> > The compiler cannot determine whether the return values of the
> > xtensa_operand_is_register(isa, opc, opnd)  and
> xtensa_operand_is_visible(isa, opc, opnd) functions are the same.
> 
> It doesn't have to because 1) they definitely are not the same, but
> 2) it doesn't matter.
> 
> > So,it assumes that 'rf' is not assigned, but it's used.
> 
> The assumption is wrong. rf is used under the 'if (register_file)'
> condition and register_file is initialized to NULL and then set to something
> non-NULL based on the value of rf here:
> 
Hi Max,
  Yeah, your analysis is correct. This rf is used only when register_file is non-NULL. When this condition is met, the rf must have been assigned a value.
The GCC 9.3 compilation I use contains the Wmaybe-uninitialized parameter by default. It cannot recognize this complex logic judgment.
This warning may be frequently encountered by developers who compile this part of code.

> 958             if (xtensa_operand_is_register(isa, opc, opnd)) {
> 959                 rf = xtensa_operand_regfile(isa, opc, opnd);
> 960                 register_file = dc->config->regfile[rf];
> 
> > The compiler showed warning:
> > target/xtensa/translate.c: In function ‘disas_xtensa_insn’:
> > target/xtensa/translate.c:985:43: warning: ‘rf’ may be used uninitialized in
> this function [-Wmaybe-uninitialized]
> >   985 |                     arg[vopnd].num_bits =
> xtensa_regfile_num_bits(isa, rf);
> >       |
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >
> > Add a default value for 'rf' to prevented the warning.
> 
> I don't see it doing default build with gcc 8.3. But then I don't see
> -Wmaybe-uninitialized in the compiler command line either.
> 
Maybe it's available after GCC9, or some CFLAG configuration.

The -Wmaybe-uninitialized parameter has this description：
"These warnings are only possible in optimizing compilation, because otherwise GCC does not keep track of the state of variables."
From：https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#Warning-Options

I have tried to configure only "-O2 -fexceptions" for the CFLAG on GCC9, and this warning will occur.

> > Reported-by: Euler Robot <euler.robot@huawei.com>
> > Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
> > ---
> > Cc: Max Filippov <jcmvbkbc@gmail.com>
> > ---
> >  target/xtensa/translate.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/target/xtensa/translate.c b/target/xtensa/translate.c
> > index 944a157747..eea851bbe7 100644
> > --- a/target/xtensa/translate.c
> > +++ b/target/xtensa/translate.c
> > @@ -953,7 +953,7 @@ static void disas_xtensa_insn(CPUXtensaState *env,
> > DisasContext *dc)
> >
> >          for (opnd = vopnd = 0; opnd < opnds; ++opnd) {
> >              void **register_file = NULL;
> > -            xtensa_regfile rf;
> > +            xtensa_regfile rf = -1;
> 
> Please use XTENSA_UNDEFINED instead if you still think this is worth changing.
> 
I don't think it's wrong, it's just a bit of trouble for the compiler :)

Thanks,
Chen Qun