[PATCH 01/06] Fix compilation warning for sound/soc/codecs/wm8400.c

[PATCH 01/06] Fix compilation warning for sound/soc/codecs/wm8400.c

[Subrata Modak]

Following fix is inspired by David Howells fix few days back:
http://lkml.org/lkml/2009/7/9/109,

Signed-off-by: Subrata Modak<subrata@linux.vnet.ibm.com>,
---

--- a/sound/soc/codecs/wm8400.c	2009-06-15 07:52:31.000000000 +0530
+++ b/sound/soc/codecs/wm8400.c	2009-07-15 05:35:00.000000000 +0530
@@ -1015,7 +1015,7 @@ static int wm8400_set_dai_pll(struct snd
 {
 	struct snd_soc_codec *codec = codec_dai->codec;
 	struct wm8400_priv *wm8400 = codec->private_data;
-	struct fll_factors factors;
+	struct fll_factors uninitialized_var(factors);
 	int ret;
 	u16 reg;
 

---
Regards--
Subrata

Re: [PATCH 01/06] Fix compilation warning for sound/soc/codecs/wm8400.c

[Jaswinder Singh Rajput]

Hello Subrata,

On Wed, 2009-07-15 at 07:48 +0530, Subrata Modak wrote:
> Following fix is inspired by David Howells fix few days back:
> http://lkml.org/lkml/2009/7/9/109,
> 

Please check further of this thread :

http://marc.info/?l=linux-kernel&m=124756210124518&w=2


> Signed-off-by: Subrata Modak<subrata@linux.vnet.ibm.com>,
> ---
> 
> --- a/sound/soc/codecs/wm8400.c	2009-06-15 07:52:31.000000000 +0530
> +++ b/sound/soc/codecs/wm8400.c	2009-07-15 05:35:00.000000000 +0530
> @@ -1015,7 +1015,7 @@ static int wm8400_set_dai_pll(struct snd
>  {
>  	struct snd_soc_codec *codec = codec_dai->codec;
>  	struct wm8400_priv *wm8400 = codec->private_data;
> -	struct fll_factors factors;
> +	struct fll_factors uninitialized_var(factors);
>  	int ret;
>  	u16 reg;
>  
> 

This kind of warnings born due to not handling the variables properly.
Please try to understand the root cause of this warning, just shutting
this warning is of no use.

Please revise your patches, If you face any problems let me know.

Thanks,
--
JSR

Re: [PATCH 01/06] Fix compilation warning for sound/soc/codecs/wm8400.c

[Mark Brown]

On Wed, Jul 15, 2009 at 10:13:38AM +0530, Jaswinder Singh Rajput wrote:
> Hello Subrata,

> This kind of warnings born due to not handling the variables properly.
> Please try to understand the root cause of this warning, just shutting
> this warning is of no use.

...as I said last time you submitted rougly the same patch.  Please also
remember that sometimes these problems are false positives caused by
issues with the compiler flow analysis (as appears to be the case here).

Re:[PATCH 01/06] Fix compilation warning for sound/soc/codecs/wm8400.c

[Subrata Modak]

Hey,

>On Wed, 2009-07-15 at 10:13 +0530, Jaswinder Singh Rajput wrote:
>Hello Subrata,
> 
> On Wed, 2009-07-15 at 07:48 +0530, Subrata Modak wrote:
> > Following fix is inspired by David Howells fix few days back:
> > http://lkml.org/lkml/2009/7/9/109,
> > 
> 
> Please check further of this thread :
> 
> http://marc.info/?l=linux-kernel&m=124756210124518&w=2

Does the above really apply here ?

> 
> 
> > Signed-off-by: Subrata Modak<subrata@linux.vnet.ibm.com>,
> > ---
> > 
> > --- a/sound/soc/codecs/wm8400.c	2009-06-15 07:52:31.000000000 +0530
> > +++ b/sound/soc/codecs/wm8400.c	2009-07-15 05:35:00.000000000 +0530
> > @@ -1015,7 +1015,7 @@ static int wm8400_set_dai_pll(struct snd
> >  {
> >  	struct snd_soc_codec *codec = codec_dai->codec;
> >  	struct wm8400_priv *wm8400 = codec->private_data;
> > -	struct fll_factors factors;
> > +	struct fll_factors uninitialized_var(factors);
> >  	int ret;
> >  	u16 reg;
> >  
> > 
> 
> This kind of warnings born due to not handling the variables properly.
> Please try to understand the root cause of this warning, just shutting
> this warning is of no use.
> 
> Please revise your patches, If you face any problems let me know.
> 

How about the following brutal shutdown ?
---

--- a/sound/soc/codecs/wm8400.c	2009-07-16 16:21:37.000000000 +0530
+++ b/sound/soc/codecs/wm8400.c	2009-07-16 18:28:48.000000000 +0530
@@ -1015,7 +1015,7 @@ static int wm8400_set_dai_pll(struct snd
 {
 	struct snd_soc_codec *codec = codec_dai->codec;
 	struct wm8400_priv *wm8400 = codec->private_data;
-	struct fll_factors factors;
+	struct fll_factors factors = {};
 	int ret;
 	u16 reg;
 
---

As i found that:

1024 
1025         if (freq_out != 0) {
1026                 ret = fll_factors(wm8400, &factors, freq_in, freq_out);
1027                 if (ret != 0)
1028                         return ret;
1029         }
1030 

'factors' will get initialized here, as, 'freq_out' will probably never have
a '0' value. So, 'fll_factors()' will actually overwrite values in 'factors'
even after the initial brutal assignment:
	"struct fll_factors factors = {}"

Regards--
Subrata

Re: [PATCH 01/06] Fix compilation warning for sound/soc/codecs/wm8400.c

[Mark Brown]

On Thu, Jul 16, 2009 at 06:40:01PM +0530, Subrata Modak wrote:

> How about the following brutal shutdown ?

No, this is the patch you originally submitted.  You are missing the
point here - your patches are all just papering over the warning and
won't help at all if there is an actual problem.

> 'factors' will get initialized here, as, 'freq_out' will probably never have
> a '0' value. So, 'fll_factors()' will actually overwrite values in 'factors'
> even after the initial brutal assignment:
> 	"struct fll_factors factors = {}"

This is pretty much the point of the warning when it's valid - it's
trying to catch situations where there is a code path where the variable
is used without being initialised.  If you just blindly initialise the
variable as you are doing then the warning goes away but any problematic
code remains so the situation is worse.

In this case I suspect that whatever compiler you are using (none of
those I've tried with seem to be doing this) isn't able to figure out
that if we skip initialising the variable then we use exactly the same
condition to return from the function before we try to use the variable.
For something like this the warning can normally be worked around by
changing the control flow so that the compiler is able to figure out
that things are safe.

Re: [PATCH 01/06] Fix compilation warning for sound/soc/codecs/wm8400.c

[Takashi Iwai]

At Thu, 16 Jul 2009 14:23:08 +0100,
Mark Brown wrote:
> 
> On Thu, Jul 16, 2009 at 06:40:01PM +0530, Subrata Modak wrote:
> 
> > How about the following brutal shutdown ?
> 
> No, this is the patch you originally submitted.  You are missing the
> point here - your patches are all just papering over the warning and
> won't help at all if there is an actual problem.
> 
> > 'factors' will get initialized here, as, 'freq_out' will probably never have
> > a '0' value. So, 'fll_factors()' will actually overwrite values in 'factors'
> > even after the initial brutal assignment:
> > 	"struct fll_factors factors = {}"
> 
> This is pretty much the point of the warning when it's valid - it's
> trying to catch situations where there is a code path where the variable
> is used without being initialised.  If you just blindly initialise the
> variable as you are doing then the warning goes away but any problematic
> code remains so the situation is worse.
> 
> In this case I suspect that whatever compiler you are using (none of
> those I've tried with seem to be doing this) isn't able to figure out
> that if we skip initialising the variable then we use exactly the same
> condition to return from the function before we try to use the variable.

I get compile warnings on gcc 4.4.0, too.


> For something like this the warning can normally be worked around by
> changing the control flow so that the compiler is able to figure out
> that things are safe.

Agreed.


Takashi