[PATCH 0/2] Prevent uninitialized warnings

[PATCH 0/2] Prevent uninitialized warnings

[mrezanin]

From: Miroslav Rezanina <mrezanin@redhat.com>

Using -Wmaybe-uninitialized when optimalization is enabled can cause 
several warnings during build. This will break build in case -Werror
is used.

This series fixes two cases of this warnings that can happen during
build of QEMU.

Miroslav Rezanina (2):
  test-logging: Fix -Werror=maybe-uninitialized warning
  aspeed/i2c: Prevent uninitialized warning

 hw/i2c/aspeed_i2c.c  | 2 +-
 tests/test-logging.c | 6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)

-- 
1.8.3.1

[PATCH 1/2] test-logging: Fix -Werror=maybe-uninitialized warning

[mrezanin]

From: Miroslav Rezanina <mrezanin@redhat.com>

Checking for uninitialized variables raises warning for file path
variables in test_logfile_write and test_logfile_lock functions.

To suppress this warning, initialize varibles to NULL. This is safe
change as result of g_build_filename is stored to them before any usage.

Signed-off-by: Miroslav Rezanina <mrezanin@redhat.com>
---
 tests/test-logging.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tests/test-logging.c b/tests/test-logging.c
index 1e646f0..6387e49 100644
--- a/tests/test-logging.c
+++ b/tests/test-logging.c
@@ -114,8 +114,8 @@ static void test_logfile_write(gconstpointer data)
     QemuLogFile *logfile2;
     gchar const *dir = data;
     Error *err = NULL;
-    g_autofree gchar *file_path;
-    g_autofree gchar *file_path1;
+    g_autofree gchar *file_path = NULL;
+    g_autofree gchar *file_path1 = NULL;
     FILE *orig_fd;
 
     /*
@@ -157,7 +157,7 @@ static void test_logfile_lock(gconstpointer data)
     FILE *logfile;
     gchar const *dir = data;
     Error *err = NULL;
-    g_autofree gchar *file_path;
+    g_autofree gchar *file_path = NULL;
 
     file_path = g_build_filename(dir, "qemu_test_logfile_lock0.log", NULL);
 
-- 
1.8.3.1

[PATCH 2/2] aspeed/i2c: Prevent uninitialized warning

[mrezanin]

From: Miroslav Rezanina <mrezanin@redhat.com>

Compiler reports uninitialized warning for cmd_flags variable.

Adding NULL initialization to prevent this warning.

Signed-off-by: Miroslav Rezanina <mrezanin@redhat.com>
---
 hw/i2c/aspeed_i2c.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
index 2da04a4..445182a 100644
--- a/hw/i2c/aspeed_i2c.c
+++ b/hw/i2c/aspeed_i2c.c
@@ -400,7 +400,7 @@ static bool aspeed_i2c_check_sram(AspeedI2CBus *bus)
 
 static void aspeed_i2c_bus_cmd_dump(AspeedI2CBus *bus)
 {
-    g_autofree char *cmd_flags;
+    g_autofree char *cmd_flags = NULL;
     uint32_t count;
 
     if (bus->cmd & (I2CD_RX_BUFF_ENABLE | I2CD_RX_BUFF_ENABLE)) {
-- 
1.8.3.1

Re: [PATCH 1/2] test-logging: Fix -Werror=maybe-uninitialized warning

[Thomas Huth]

On 21/01/2020 10.28, mrezanin@redhat.com wrote:
> From: Miroslav Rezanina <mrezanin@redhat.com>
> 
> Checking for uninitialized variables raises warning for file path
> variables in test_logfile_write and test_logfile_lock functions.
> 
> To suppress this warning, initialize varibles to NULL. This is safe
> change as result of g_build_filename is stored to them before any usage.
> 
> Signed-off-by: Miroslav Rezanina <mrezanin@redhat.com>
> ---
>  tests/test-logging.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/tests/test-logging.c b/tests/test-logging.c
> index 1e646f0..6387e49 100644
> --- a/tests/test-logging.c
> +++ b/tests/test-logging.c
> @@ -114,8 +114,8 @@ static void test_logfile_write(gconstpointer data)
>      QemuLogFile *logfile2;
>      gchar const *dir = data;
>      Error *err = NULL;
> -    g_autofree gchar *file_path;
> -    g_autofree gchar *file_path1;
> +    g_autofree gchar *file_path = NULL;
> +    g_autofree gchar *file_path1 = NULL;
>      FILE *orig_fd;
>  
>      /*
> @@ -157,7 +157,7 @@ static void test_logfile_lock(gconstpointer data)
>      FILE *logfile;
>      gchar const *dir = data;
>      Error *err = NULL;
> -    g_autofree gchar *file_path;
> +    g_autofree gchar *file_path = NULL;
>  
>      file_path = g_build_filename(dir, "qemu_test_logfile_lock0.log", NULL);

Right. The glib documentation clearly states that "the variable must be
initialized", see:

https://developer.gnome.org/glib/stable/glib-Miscellaneous-Macros.html#g-autofree

So this is the right thing to do here!

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH 2/2] aspeed/i2c: Prevent uninitialized warning

[Thomas Huth]

On 21/01/2020 10.28, mrezanin@redhat.com wrote:
> From: Miroslav Rezanina <mrezanin@redhat.com>
> 
> Compiler reports uninitialized warning for cmd_flags variable.
> 
> Adding NULL initialization to prevent this warning.
> 
> Signed-off-by: Miroslav Rezanina <mrezanin@redhat.com>
> ---
>  hw/i2c/aspeed_i2c.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
> index 2da04a4..445182a 100644
> --- a/hw/i2c/aspeed_i2c.c
> +++ b/hw/i2c/aspeed_i2c.c
> @@ -400,7 +400,7 @@ static bool aspeed_i2c_check_sram(AspeedI2CBus *bus)
>  
>  static void aspeed_i2c_bus_cmd_dump(AspeedI2CBus *bus)
>  {
> -    g_autofree char *cmd_flags;
> +    g_autofree char *cmd_flags = NULL;
>      uint32_t count;
>  
>      if (bus->cmd & (I2CD_RX_BUFF_ENABLE | I2CD_RX_BUFF_ENABLE)) {

Reviewed-by: Thomas Huth <thuth@redhat.com>

... maybe someone with enough Perl-foo (i.e. not me ;-)) should add a
check to our check_patch.pl script so that it complains when new code is
introduced that uses g_autofree without initializing the variable...

Re: [PATCH 2/2] aspeed/i2c: Prevent uninitialized warning

[Cédric Le Goater]

On 1/21/20 11:02 AM, Thomas Huth wrote:
> On 21/01/2020 10.28, mrezanin@redhat.com wrote:
>> From: Miroslav Rezanina <mrezanin@redhat.com>
>>
>> Compiler reports uninitialized warning for cmd_flags variable.
>>
>> Adding NULL initialization to prevent this warning.
>>
>> Signed-off-by: Miroslav Rezanina <mrezanin@redhat.com>
>> ---
>>  hw/i2c/aspeed_i2c.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
>> index 2da04a4..445182a 100644
>> --- a/hw/i2c/aspeed_i2c.c
>> +++ b/hw/i2c/aspeed_i2c.c
>> @@ -400,7 +400,7 @@ static bool aspeed_i2c_check_sram(AspeedI2CBus *bus)
>>  
>>  static void aspeed_i2c_bus_cmd_dump(AspeedI2CBus *bus)
>>  {
>> -    g_autofree char *cmd_flags;
>> +    g_autofree char *cmd_flags = NULL;
>>      uint32_t count;
>>  
>>      if (bus->cmd & (I2CD_RX_BUFF_ENABLE | I2CD_RX_BUFF_ENABLE)) {
> 
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> 
> ... maybe someone with enough Perl-foo (i.e. not me ;-)) should add a
> check to our check_patch.pl script so that it complains when new code is
> introduced that uses g_autofree without initializing the variable...

weird. The cmd_flags variable is assigned just after and used
in a trace. 

C.

Re: [PATCH 2/2] aspeed/i2c: Prevent uninitialized warning

[Miroslav Rezanina]

----- Original Message -----
> From: "Cédric Le Goater" <clg@kaod.org>
> To: "Thomas Huth" <thuth@redhat.com>, mrezanin@redhat.com, qemu-devel@nongnu.org
> Cc: "peter maydell" <peter.maydell@linaro.org>, "Andrew Jeffery" <andrew@aj.id.au>, "Joel Stanley" <joel@jms.id.au>,
> qemu-trivial@nongnu.org
> Sent: Tuesday, January 21, 2020 11:44:14 AM
> Subject: Re: [PATCH 2/2] aspeed/i2c: Prevent uninitialized warning
> 
> On 1/21/20 11:02 AM, Thomas Huth wrote:
> > On 21/01/2020 10.28, mrezanin@redhat.com wrote:
> >> From: Miroslav Rezanina <mrezanin@redhat.com>
> >>
> >> Compiler reports uninitialized warning for cmd_flags variable.
> >>
> >> Adding NULL initialization to prevent this warning.
> >>
> >> Signed-off-by: Miroslav Rezanina <mrezanin@redhat.com>
> >> ---
> >>  hw/i2c/aspeed_i2c.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
> >> index 2da04a4..445182a 100644
> >> --- a/hw/i2c/aspeed_i2c.c
> >> +++ b/hw/i2c/aspeed_i2c.c
> >> @@ -400,7 +400,7 @@ static bool aspeed_i2c_check_sram(AspeedI2CBus *bus)
> >>  
> >>  static void aspeed_i2c_bus_cmd_dump(AspeedI2CBus *bus)
> >>  {
> >> -    g_autofree char *cmd_flags;
> >> +    g_autofree char *cmd_flags = NULL;
> >>      uint32_t count;
> >>  
> >>      if (bus->cmd & (I2CD_RX_BUFF_ENABLE | I2CD_RX_BUFF_ENABLE)) {
> > 
> > Reviewed-by: Thomas Huth <thuth@redhat.com>
> > 
> > ... maybe someone with enough Perl-foo (i.e. not me ;-)) should add a
> > check to our check_patch.pl script so that it complains when new code is
> > introduced that uses g_autofree without initializing the variable...
> 
> weird. The cmd_flags variable is assigned just after and used
> in a trace.
> 

As g_autofree is used, variable has to be initialized otherwise will compiler
complain even in the case we write to variable immediately after.

Mirek

> C.
> 
> 

-- 
Miroslav Rezanina
Software Engineer - Virtualization Team Maintainer

Re: [PATCH 2/2] aspeed/i2c: Prevent uninitialized warning

[Thomas Huth]

On 21/01/2020 11.44, Cédric Le Goater wrote:
> On 1/21/20 11:02 AM, Thomas Huth wrote:
>> On 21/01/2020 10.28, mrezanin@redhat.com wrote:
>>> From: Miroslav Rezanina <mrezanin@redhat.com>
>>>
>>> Compiler reports uninitialized warning for cmd_flags variable.
>>>
>>> Adding NULL initialization to prevent this warning.
>>>
>>> Signed-off-by: Miroslav Rezanina <mrezanin@redhat.com>
>>> ---
>>>  hw/i2c/aspeed_i2c.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
>>> index 2da04a4..445182a 100644
>>> --- a/hw/i2c/aspeed_i2c.c
>>> +++ b/hw/i2c/aspeed_i2c.c
>>> @@ -400,7 +400,7 @@ static bool aspeed_i2c_check_sram(AspeedI2CBus *bus)
>>>  
>>>  static void aspeed_i2c_bus_cmd_dump(AspeedI2CBus *bus)
>>>  {
>>> -    g_autofree char *cmd_flags;
>>> +    g_autofree char *cmd_flags = NULL;
>>>      uint32_t count;
>>>  
>>>      if (bus->cmd & (I2CD_RX_BUFF_ENABLE | I2CD_RX_BUFF_ENABLE)) {
>>
>> Reviewed-by: Thomas Huth <thuth@redhat.com>
>>
>> ... maybe someone with enough Perl-foo (i.e. not me ;-)) should add a
>> check to our check_patch.pl script so that it complains when new code is
>> introduced that uses g_autofree without initializing the variable...
> 
> weird. The cmd_flags variable is assigned just after and used
> in a trace. 

I don't know, but my guess is that you could compile with tracing
disabled - in that case gcc might maybe optimize the assignment away,
too... ?

 Thomas

Re: [PATCH 1/2] test-logging: Fix -Werror=maybe-uninitialized warning

[Robert Foley]

Good catch.
Reviewed-by: Robert Foley <robert.foley@linaro.org>

On Tue, 21 Jan 2020 at 04:58, Thomas Huth <thuth@redhat.com> wrote:
>
> On 21/01/2020 10.28, mrezanin@redhat.com wrote:
> > From: Miroslav Rezanina <mrezanin@redhat.com>
> >
> > Checking for uninitialized variables raises warning for file path
> > variables in test_logfile_write and test_logfile_lock functions.
> >
> > To suppress this warning, initialize varibles to NULL. This is safe
> > change as result of g_build_filename is stored to them before any usage.
> >
> > Signed-off-by: Miroslav Rezanina <mrezanin@redhat.com>
> > ---
> >  tests/test-logging.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/tests/test-logging.c b/tests/test-logging.c
> > index 1e646f0..6387e49 100644
> > --- a/tests/test-logging.c
> > +++ b/tests/test-logging.c
> > @@ -114,8 +114,8 @@ static void test_logfile_write(gconstpointer data)
> >      QemuLogFile *logfile2;
> >      gchar const *dir = data;
> >      Error *err = NULL;
> > -    g_autofree gchar *file_path;
> > -    g_autofree gchar *file_path1;
> > +    g_autofree gchar *file_path = NULL;
> > +    g_autofree gchar *file_path1 = NULL;
> >      FILE *orig_fd;
> >
> >      /*
> > @@ -157,7 +157,7 @@ static void test_logfile_lock(gconstpointer data)
> >      FILE *logfile;
> >      gchar const *dir = data;
> >      Error *err = NULL;
> > -    g_autofree gchar *file_path;
> > +    g_autofree gchar *file_path = NULL;
> >
> >      file_path = g_build_filename(dir, "qemu_test_logfile_lock0.log", NULL);
>
> Right. The glib documentation clearly states that "the variable must be
> initialized", see:
>
> https://developer.gnome.org/glib/stable/glib-Miscellaneous-Macros.html#g-autofree
>
> So this is the right thing to do here!
>
> Reviewed-by: Thomas Huth <thuth@redhat.com>
>

Re: [PATCH 2/2] aspeed/i2c: Prevent uninitialized warning

[Laurent Vivier]

Le 21/01/2020 à 10:28, mrezanin@redhat.com a écrit :
> From: Miroslav Rezanina <mrezanin@redhat.com>
> 
> Compiler reports uninitialized warning for cmd_flags variable.
> 
> Adding NULL initialization to prevent this warning.
> 
> Signed-off-by: Miroslav Rezanina <mrezanin@redhat.com>
> ---
>  hw/i2c/aspeed_i2c.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
> index 2da04a4..445182a 100644
> --- a/hw/i2c/aspeed_i2c.c
> +++ b/hw/i2c/aspeed_i2c.c
> @@ -400,7 +400,7 @@ static bool aspeed_i2c_check_sram(AspeedI2CBus *bus)
>  
>  static void aspeed_i2c_bus_cmd_dump(AspeedI2CBus *bus)
>  {
> -    g_autofree char *cmd_flags;
> +    g_autofree char *cmd_flags = NULL;
>      uint32_t count;
>  
>      if (bus->cmd & (I2CD_RX_BUFF_ENABLE | I2CD_RX_BUFF_ENABLE)) {
> 

Applied to my trivial-patches branch.

Thanks,
Laurent