Xen devel - Feb 2011 - Build errors with latest xen-unstable from staging

FYI - Pulled the latest xen-unstable from staging to sync some patches and got
these trivial errors while compiling -

xl_cmdimpl.c: In function ‘print_domain_vcpuinfo’:
xl_cmdimpl.c:3351: warning: ‘firstset’ may be used uninitialized in this
function
xl_cmdimpl.c:3351: note: ‘firstset’ was declared here
xl_cmdimpl.c:3350: warning: ‘bitmask’ may be used uninitialized in this function
xl_cmdimpl.c:3350: note: ‘bitmask’ was declared here
xl_cmdimpl.c:3350: warning: ‘pmap’ may be used uninitialized in this function
xl_cmdimpl.c:3350: note: ‘pmap’ was declared here

GCC version - 4.2.4.  Initializing the three variables it complained about fixed
the issue.  If this trivial change should require a signed off line, here it is
- Signed-off-by: Kamala Narasimhan <kamala.narasimhan@citrix.com>

diff -r 7ada6faef565 tools/libxl/xl_cmdimpl.c
--- a/tools/libxl/xl_cmdimpl.c	Sun Feb 06 17:26:31 2011 +0000
+++ b/tools/libxl/xl_cmdimpl.c	Sun Feb 06 13:53:50 2011 -0500
@@ -3347,8 +3347,8 @@ static void print_bitmap(uint8_t *map, i
 static void print_bitmap(uint8_t *map, int maplen, FILE *stream)
 {
     int i;
-    uint8_t pmap, bitmask;
-    int firstset, state = 0;
+    uint8_t pmap = 0, bitmask = 0;
+    int firstset = 0, state = 0;

     for (i = 0; i < maplen; i++) {
         if (i % 8 == 0) {

Kamala

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On Sun, 2011-02-06 at 19:01 +0000, Kamala Narasimhan
wrote:
> FYI - Pulled the latest xen-unstable from staging to sync some patches and
got
> these trivial errors while compiling -
> 
> xl_cmdimpl.c: In function ‘print_domain_vcpuinfo’:
> xl_cmdimpl.c:3351: warning: ‘firstset’ may be used uninitialized in this
function
> xl_cmdimpl.c:3351: note: ‘firstset’ was declared here
> xl_cmdimpl.c:3350: warning: ‘bitmask’ may be used uninitialized in this
function
> xl_cmdimpl.c:3350: note: ‘bitmask’ was declared here
> xl_cmdimpl.c:3350: warning: ‘pmap’ may be used uninitialized in this
function
> xl_cmdimpl.c:3350: note: ‘pmap’ was declared here
> 
> GCC version - 4.2.4.  Initializing the three variables it complained about
fixed
> the issue.
They are actually initialised before use, during the first pass through
the for loop when i==0 and state==0, but I can see how gcc would be
unable to figure that out (in fact I''m not sure about firstset myself).

In the Linux kernel they have a macro to annotate such instances:
        /*
         * A trick to suppress uninitialized variable warning without generating
any
         * code
         */
        #define uninitialized_var(x) x = x
        
Do we want something similar?
>   If this trivial change should require a signed off line, here it is
> - Signed-off-by: Kamala Narasimhan <kamala.narasimhan@citrix.com>
Always just assume a change does.

Ian.
> 
> diff -r 7ada6faef565 tools/libxl/xl_cmdimpl.c
> --- a/tools/libxl/xl_cmdimpl.c	Sun Feb 06 17:26:31 2011 +0000
> +++ b/tools/libxl/xl_cmdimpl.c	Sun Feb 06 13:53:50 2011 -0500
> @@ -3347,8 +3347,8 @@ static void print_bitmap(uint8_t *map, i
>  static void print_bitmap(uint8_t *map, int maplen, FILE *stream)
>  {
>      int i;
> -    uint8_t pmap, bitmask;
> -    int firstset, state = 0;
> +    uint8_t pmap = 0, bitmask = 0;
> +    int firstset = 0, state = 0;
> 
>      for (i = 0; i < maplen; i++) {
>          if (i % 8 == 0) {
> 
> Kamala
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Ian Campbell wrote:
> On Sun, 2011-02-06 at 19:01 +0000, Kamala Narasimhan wrote:
> In the Linux kernel they have a macro to annotate such instances:
>         /*
>          * A trick to suppress uninitialized variable warning without
generating any
>          * code
>          */
>         #define uninitialized_var(x) x = x
>         
> Do we want something similar?
> 
But why obfuscate a warning that is there for a good reason in most cases?
Aren''t we better off initializing the variable or use a compiler option
to
suppress it if we must?

Kamala



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Ian Campbell wrote:
> On Sun, 2011-02-06 at 19:01 +0000, Kamala Narasimhan wrote:
>> FYI - Pulled the latest xen-unstable from staging to sync some patches
and got
>> these trivial errors while compiling -
>>
>> xl_cmdimpl.c: In function ‘print_domain_vcpuinfo’:
>> xl_cmdimpl.c:3351: warning: ‘firstset’ may be used uninitialized in
this function
>> xl_cmdimpl.c:3351: note: ‘firstset’ was declared here
>> xl_cmdimpl.c:3350: warning: ‘bitmask’ may be used uninitialized in this
function
>> xl_cmdimpl.c:3350: note: ‘bitmask’ was declared here
>> xl_cmdimpl.c:3350: warning: ‘pmap’ may be used uninitialized in this
function
>> xl_cmdimpl.c:3350: note: ‘pmap’ was declared here
>>
>> GCC version - 4.2.4.  Initializing the three variables it complained
about fixed
>> the issue.
> 
> They are actually initialised before use, during the first pass through
> the for loop when i==0 and state==0, but I can see how gcc would be
> unable to figure that out (in fact I''m not sure about firstset
myself).
I agree with Ian, looking through the code again I can confirm that they 
are _not_ used uninitialized. Even firstset is set when the state 
switches from 0 to 1 and is only used later in state 1 and 3, so it is safe.
It seems like the compiler is not smart enough to recognize this, 
especially the modulo operation could be a barrier for the analysis, I 
think.
According to the manpage those warning are only emitted on -O, which I 
left out for my testing, so I didn''t spot this warning. Sorry.

To appease the compiler I would ack the fix, though from an academic 
point of view it is not necessary.
> In the Linux kernel they have a macro to annotate such instances:
>         /*
>          * A trick to suppress uninitialized variable warning without
generating any
>          * code
>          */
>         #define uninitialized_var(x) x = x
>         
> Do we want something similar?
I don''t think so. x = x looks more like a hack, if we can easily 
initialize the variables, we should do so.

Regards,
Andre.
> 
>>   If this trivial change should require a signed off line, here it is
>> - Signed-off-by: Kamala Narasimhan <kamala.narasimhan@citrix.com>
> 
> Always just assume a change does.
> 
> Ian.
>> diff -r 7ada6faef565 tools/libxl/xl_cmdimpl.c
>> --- a/tools/libxl/xl_cmdimpl.c	Sun Feb 06 17:26:31 2011 +0000
>> +++ b/tools/libxl/xl_cmdimpl.c	Sun Feb 06 13:53:50 2011 -0500
>> @@ -3347,8 +3347,8 @@ static void print_bitmap(uint8_t *map, i
>>  static void print_bitmap(uint8_t *map, int maplen, FILE *stream)
>>  {
>>      int i;
>> -    uint8_t pmap, bitmask;
>> -    int firstset, state = 0;
>> +    uint8_t pmap = 0, bitmask = 0;
>> +    int firstset = 0, state = 0;
>>
>>      for (i = 0; i < maplen; i++) {
>>          if (i % 8 == 0) {
>>
>> Kamala
>>
-- 
Andre Przywara
AMD-Operating System Research Center (OSRC), Dresden, Germany



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Andre Przywara writes ("Re: [Xen-devel] Build errors with latest
xen-unstable from staging"):
> > On Sun, 2011-02-06 at 19:01 +0000, Kamala Narasimhan wrote:
> >> GCC version - 4.2.4.  Initializing the three variables it
complained about fixed
> >> the issue.
Thanks for the patch.
> To appease the compiler I would ack the fix, though from an academic 
> point of view it is not necessary.
Yes, thanks, I have applied it.
> I don''t think so. x = x looks more like a hack, if we can easily 
> initialize the variables, we should do so.
Quite so.
> >>   If this trivial change should require a signed off line, here it
is
> >> - Signed-off-by: Kamala Narasimhan
<kamala.narasimhan@citrix.com>
> > 
> > Always just assume a change does.
Indeed.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel