Xen devel - Oct 2008 - [PATCH] cpufreq.c: shut up compiler about cpufreq_dom

Some versions of GCC are too stupid to figure out that cpufreq_dom is
only used if !!domexist and always set in that case, and complain that
it may be used uninitialised.

(In general it is IMO better to avoid these kind of flag
variables; I would prefer structures like
       for (...) { cpufreq_dom = dom;  if (...) goto cpufreq_dom_found; }
       cpufreq_dom = 0;
     cpufreq_dom_found:
but on the other hand I don''t like purely stylistic changes.)

Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>

diff -r ae100f264f6a xen/drivers/cpufreq/cpufreq.c
--- a/xen/drivers/cpufreq/cpufreq.c	Wed Oct 29 13:09:37 2008 +0000
+++ b/xen/drivers/cpufreq/cpufreq.c	Wed Oct 29 16:41:14 2008 +0000
@@ -80,7 +80,7 @@ int cpufreq_add_cpu(unsigned int cpu)
     unsigned int dom, domexist = 0;
     unsigned int j;
     struct list_head *pos;
-    struct cpufreq_dom *cpufreq_dom;
+    struct cpufreq_dom *cpufreq_dom = 0;
     struct cpufreq_policy new_policy;
     struct cpufreq_policy *policy;
     struct processor_performance *perf = &processor_pminfo[cpu]->perf;

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

I wrote:
> -    struct cpufreq_dom *cpufreq_dom;
> +    struct cpufreq_dom *cpufreq_dom = 0;
Thanks for applying that but it turns out that GCC only produces the
first such error !  Hopefully this will fix our automated builds ...

Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>

Ian.

diff -r 6a8fea290af5 xen/drivers/cpufreq/cpufreq.c
--- a/xen/drivers/cpufreq/cpufreq.c	Wed Oct 29 16:58:05 2008 +0000
+++ b/xen/drivers/cpufreq/cpufreq.c	Wed Oct 29 17:50:16 2008 +0000
@@ -181,7 +181,7 @@ int cpufreq_del_cpu(unsigned int cpu)
 {
     unsigned int dom, domexist = 0;
     struct list_head *pos;
-    struct cpufreq_dom *cpufreq_dom;
+    struct cpufreq_dom *cpufreq_dom = NULL;
     struct cpufreq_policy *policy;
     struct processor_performance *perf = &processor_pminfo[cpu]->perf;
 

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

Ian Jackson wrote:
> Some versions of GCC are too stupid to figure out that cpufreq_dom is
> only used if !!domexist and always set in that case, and complain that
> it may be used uninitialised.
> 
> (In general it is IMO better to avoid these kind of flag
> variables; I would prefer structures like
>        for (...) { cpufreq_dom = dom;  if (...) goto
>        cpufreq_dom_found; } cpufreq_dom = 0;
>      cpufreq_dom_found:
> but on the other hand I don''t like purely stylistic changes.)
Ian,

What''s the advantage of the above coding style? seems it saved a flag
but add 1 more jump.

Thanks,
Jinsong
> 
> Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
> 
> diff -r ae100f264f6a xen/drivers/cpufreq/cpufreq.c
> --- a/xen/drivers/cpufreq/cpufreq.c	Wed Oct 29 13:09:37 2008 +0000
> +++ b/xen/drivers/cpufreq/cpufreq.c	Wed Oct 29 16:41:14 2008 +0000
> @@ -80,7 +80,7 @@ int cpufreq_add_cpu(unsigned int cpu)
>      unsigned int dom, domexist = 0;
>      unsigned int j;
>      struct list_head *pos;
> -    struct cpufreq_dom *cpufreq_dom;
> +    struct cpufreq_dom *cpufreq_dom = 0;
>      struct cpufreq_policy new_policy;
>      struct cpufreq_policy *policy;
>      struct processor_performance *perf >
&processor_pminfo[cpu]->perf;
> 
> _______________________________________________
> 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

Liu, Jinsong writes ("RE: [Xen-devel] [PATCH] cpufreq.c: shut
up
> What''s the advantage of the above coding style? seems it saved a
> flag but add 1 more jump.
It''s clearer, and the compiler can see what''s going on.

There isn''t another jump `as written'', since `break''
is a jump too.
Whether there is another jump after the compiler is done with it
depends on a lot of factors, but generally I would expect the compiler
to do a better job when information about the program''s possible
behaviours is represented directly in the control flow than when it is
stored in a flag variable.

For example, often the allocation of a new structure can be done
directly at the point where we fall out of the loop, eg:
> >        for (...) { cpufreq_dom = dom; if (...) goto cpufreq_dom_found;
}
           cpufreq_dom = xmalloc(...);  cpufreq_dom->contents = value;
...
> >      cpufreq_dom_found:
Ian.

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

Ian Jackson wrote:
> Liu, Jinsong writes ("RE: [Xen-devel] [PATCH] cpufreq.c: shut up
>> What''s the advantage of the above coding style? seems it saved
a
>> flag but add 1 more jump.
> 
> It''s clearer, and the compiler can see what''s going on.
> 
> There isn''t another jump `as written'', since
`break'' is a jump too.
> Whether there is another jump after the compiler is done with it
> depends on a lot of factors, but generally I would expect the compiler
> to do a better job when information about the program''s possible
> behaviours is represented directly in the control flow than when it is
> stored in a flag variable.
> 
> For example, often the allocation of a new structure can be done
> directly at the point where we fall out of the loop, eg:
>>>        for (...) { cpufreq_dom = dom; if (...) goto
>>> cpufreq_dom_found; } 
>            cpufreq_dom = xmalloc(...);  cpufreq_dom->contents >
value; ...
>>>      cpufreq_dom_found:
> 
> Ian.
Thanks a lot!

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