[PATCH -tip] pcm_native: label out defined but not used

[PATCH -tip] pcm_native: label out defined but not used

[Steven Noonan]

Signed-off-by: Steven Noonan <steven@uplinklabs.net>
---
 sound/core/pcm_native.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
index c487025..3953bf6 100644
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -3252,7 +3252,7 @@ static int snd_pcm_fasync(int fd, struct file * file, int on)
 	runtime = substream->runtime;
 
 	err = fasync_helper(fd, file, on, &runtime->fasync);
-out:
+
 	unlock_kernel();
 	if (err < 0)
 		return err;
-- 
1.6.0.2

[PATCH -tip] sdhci: 'scratch' may be used uninitialized

[Steven Noonan]

Signed-off-by: Steven Noonan <steven@uplinklabs.net>
---
 drivers/mmc/host/sdhci.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index e3a8133..6257677 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -177,7 +177,7 @@ static void sdhci_read_block_pio(struct sdhci_host *host)
 {
 	unsigned long flags;
 	size_t blksize, len, chunk;
-	u32 scratch;
+	u32 uninitialized_var(scratch);
 	u8 *buf;
 
 	DBG("PIO reading\n");
-- 
1.6.0.2

[PATCH -tip] drivers/serial/8250.c: 'i' may be used uninitialized

[Steven Noonan]

Signed-off-by: Steven Noonan <steven@uplinklabs.net>
---
 drivers/serial/8250.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/serial/8250.c b/drivers/serial/8250.c
index 356c2a2..b51e91e 100644
--- a/drivers/serial/8250.c
+++ b/drivers/serial/8250.c
@@ -1599,7 +1599,7 @@ static int serial_link_irq_chain(struct uart_8250_port *up)
 
 static void serial_unlink_irq_chain(struct uart_8250_port *up)
 {
-	struct irq_info *i;
+	struct irq_info *uninitialized_var(i);
 	struct hlist_node *n;
 	struct hlist_head *h;
 
-- 
1.6.0.2

Re: [PATCH -tip] pcm_native: label out defined but not used

[Ingo Molnar]

(Cc:-ed Taskashi for this ALSA cleanup patch.)

* Steven Noonan <steven@uplinklabs.net> wrote:

> Signed-off-by: Steven Noonan <steven@uplinklabs.net>
> ---
>  sound/core/pcm_native.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
> index c487025..3953bf6 100644
> --- a/sound/core/pcm_native.c
> +++ b/sound/core/pcm_native.c
> @@ -3252,7 +3252,7 @@ static int snd_pcm_fasync(int fd, struct file * file, int on)
>  	runtime = substream->runtime;
>  
>  	err = fasync_helper(fd, file, on, &runtime->fasync);
> -out:
> +
>  	unlock_kernel();
>  	if (err < 0)
>  		return err;
> -- 
> 1.6.0.2

Re: [PATCH -tip] sdhci: 'scratch' may be used uninitialized

[Ingo Molnar]

(Cc:-ed Pierre Ossman)

* Steven Noonan <steven@uplinklabs.net> wrote:

> Signed-off-by: Steven Noonan <steven@uplinklabs.net>
> ---
>  drivers/mmc/host/sdhci.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index e3a8133..6257677 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -177,7 +177,7 @@ static void sdhci_read_block_pio(struct sdhci_host *host)
>  {
>  	unsigned long flags;
>  	size_t blksize, len, chunk;
> -	u32 scratch;
> +	u32 uninitialized_var(scratch);
>  	u8 *buf;
>  
>  	DBG("PIO reading\n");
> -- 
> 1.6.0.2

Re: [PATCH -tip] drivers/serial/8250.c: 'i' may be used uninitialized

[Ingo Molnar]

( Alan Cc:-ed. Seems like gcc bogosity - so your solution of using
  uninitialized_var() is the correct way to annotate this. )

* Steven Noonan <steven@uplinklabs.net> wrote:

> Signed-off-by: Steven Noonan <steven@uplinklabs.net>
> ---
>  drivers/serial/8250.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/serial/8250.c b/drivers/serial/8250.c
> index 356c2a2..b51e91e 100644
> --- a/drivers/serial/8250.c
> +++ b/drivers/serial/8250.c
> @@ -1599,7 +1599,7 @@ static int serial_link_irq_chain(struct uart_8250_port *up)
>  
>  static void serial_unlink_irq_chain(struct uart_8250_port *up)
>  {
> -	struct irq_info *i;
> +	struct irq_info *uninitialized_var(i);
>  	struct hlist_node *n;
>  	struct hlist_head *h;
>  
> -- 
> 1.6.0.2

[PATCH -tip] pcm_native: label out defined but not used

[Steven Noonan]

Signed-off-by: Steven Noonan <steven@uplinklabs.net>
---
 sound/core/pcm_native.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
index c487025..3953bf6 100644
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -3252,7 +3252,7 @@ static int snd_pcm_fasync(int fd, struct file * file, int on)
 	runtime = substream->runtime;
 
 	err = fasync_helper(fd, file, on, &runtime->fasync);
-out:
+
 	unlock_kernel();
 	if (err < 0)
 		return err;
-- 
1.6.0.2

Re: [PATCH -tip] pcm_native: label out defined but not used

[Takashi Iwai]

At Wed, 1 Oct 2008 09:59:21 +0200,
Ingo Molnar wrote:
> 
> 
> (Cc:-ed Taskashi for this ALSA cleanup patch.)

This label is used when the debug option is enabled.
So, don't apply this.

thanks,

Takashi

> * Steven Noonan <steven@uplinklabs.net> wrote:
> 
> > Signed-off-by: Steven Noonan <steven@uplinklabs.net>
> > ---
> >  sound/core/pcm_native.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> > 
> > diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
> > index c487025..3953bf6 100644
> > --- a/sound/core/pcm_native.c
> > +++ b/sound/core/pcm_native.c
> > @@ -3252,7 +3252,7 @@ static int snd_pcm_fasync(int fd, struct file * file, int on)
> >  	runtime = substream->runtime;
> >  
> >  	err = fasync_helper(fd, file, on, &runtime->fasync);
> > -out:
> > +
> >  	unlock_kernel();
> >  	if (err < 0)
> >  		return err;
> > -- 
> > 1.6.0.2
> 

[PATCH -tip] sdhci: 'scratch' may be used uninitialized

[Steven Noonan]

Signed-off-by: Steven Noonan <steven@uplinklabs.net>
---
 drivers/mmc/host/sdhci.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index e3a8133..6257677 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -177,7 +177,7 @@ static void sdhci_read_block_pio(struct sdhci_host *host)
 {
 	unsigned long flags;
 	size_t blksize, len, chunk;
-	u32 scratch;
+	u32 uninitialized_var(scratch);
 	u8 *buf;
 
 	DBG("PIO reading\n");
-- 
1.6.0.2

[PATCH -tip] drivers/serial/8250.c: 'i' may be used uninitialized

[Steven Noonan]

Signed-off-by: Steven Noonan <steven@uplinklabs.net>
---
 drivers/serial/8250.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/serial/8250.c b/drivers/serial/8250.c
index 356c2a2..b51e91e 100644
--- a/drivers/serial/8250.c
+++ b/drivers/serial/8250.c
@@ -1599,7 +1599,7 @@ static int serial_link_irq_chain(struct uart_8250_port *up)
 
 static void serial_unlink_irq_chain(struct uart_8250_port *up)
 {
-	struct irq_info *i;
+	struct irq_info *uninitialized_var(i);
 	struct hlist_node *n;
 	struct hlist_head *h;
 
-- 
1.6.0.2

Re: [PATCH -tip] drivers/serial/8250.c: 'i' may be used uninitialized

[Ingo Molnar]

* Steven Noonan <steven@uplinklabs.net> wrote:

> Signed-off-by: Steven Noonan <steven@uplinklabs.net>
> ---
>  drivers/serial/8250.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)

the change is obvious to you, but it's useful to put an analysis into 
the changelog. Something like:

 serial_unlink_irq_chain() does not initialize iterator 'i', and that is 
 correct logically because it is always initialized due to XYZ. Gcc does
 not realize this connection and emits a false warning. Annotate it with 
 uninitialized_var().

and fill in XYZ.

Doing such changelogs is useful to maintainers: they'll see that you 
havent just squashed a warning you noticed, you understood the code and 
determined it via review that the warning is GCC's fault, not the 
kernel's.

with an empty changelog the maintainer will have to do this himself. 
(and can easily put your patch to the tail of a very long TODO list, or 
outright skip your patch.)

	Ingo

Re: [PATCH -tip] pcm_native: label out defined but not used

[Ingo Molnar]

* Takashi Iwai <tiwai@suse.de> wrote:

> At Wed, 1 Oct 2008 09:59:21 +0200,
> Ingo Molnar wrote:
> > 
> > 
> > (Cc:-ed Taskashi for this ALSA cleanup patch.)
> 
> This label is used when the debug option is enabled.

ah, indeed:

        snd_assert(substream != NULL, goto out);

if then i'd suggest to clean it up like this:

        if (snd_assert(substream != NULL))
		goto out;

this is how DEBUG_LOCKS_WARN_ON() is used in kernel/lockdep.c for 
example. Putting code flow side-effects into debug macros looks a bit 
weird.

	Ingo

Re: [PATCH -tip] sdhci: 'scratch' may be used uninitialized

[Steven Noonan]

On Wed, Oct 1, 2008 at 1:14 AM, Steven Noonan <steven@uplinklabs.net> wrote:
> -       u32 scratch;
> +       u32 uninitialized_var(scratch);

A bit of a further explanation:

The variable 'scratch' is always initialized before it's used. The
conditional which is responsible for initialization of 'scratch' will
always evaluate 'true' when the first loop iteration occurs, and thus,
it's properly initialized. GCC doesn't see this, of course, so using
the uninitialized_var() macro seems to work for silencing this case.

- Steven

Re: [PATCH -tip] sdhci: 'scratch' may be used uninitialized

[Ingo Molnar]

* Steven Noonan <steven@uplinklabs.net> wrote:

> On Wed, Oct 1, 2008 at 1:14 AM, Steven Noonan <steven@uplinklabs.net> wrote:
> > -       u32 scratch;
> > +       u32 uninitialized_var(scratch);
> 
> A bit of a further explanation:
> 
> The variable 'scratch' is always initialized before it's used. The
> conditional which is responsible for initialization of 'scratch' will
> always evaluate 'true' when the first loop iteration occurs, and thus,
> it's properly initialized. GCC doesn't see this, of course, so using
> the uninitialized_var() macro seems to work for silencing this case.

another small workflow suggestion: when you add explanation for the 
changelog it's better to just include the updated patch (dont worry 
about the duplication) - that way maintainers dont have to cut&slice the 
patch and the description toghether. [which is not just a matter of 
wasting time, but if you do tons of patch juggling, it becomes a real 
risk factor.]

	Ingo

Re: [PATCH -tip] drivers/serial/8250.c: 'i' may be used uninitialized

[Steven Noonan]

On Wed, Oct 1, 2008 at 1:29 AM, Ingo Molnar <mingo@elte.hu> wrote:
>
> * Steven Noonan <steven@uplinklabs.net> wrote:
>
>> Signed-off-by: Steven Noonan <steven@uplinklabs.net>
>> ---
>>  drivers/serial/8250.c |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> the change is obvious to you, but it's useful to put an analysis into
> the changelog. Something like:
>
>  serial_unlink_irq_chain() does not initialize iterator 'i', and that is
>  correct logically because it is always initialized due to XYZ. Gcc does
>  not realize this connection and emits a false warning. Annotate it with
>  uninitialized_var().
>
> and fill in XYZ.
>
> Doing such changelogs is useful to maintainers: they'll see that you
> havent just squashed a warning you noticed, you understood the code and
> determined it via review that the warning is GCC's fault, not the
> kernel's.
>
> with an empty changelog the maintainer will have to do this himself.
> (and can easily put your patch to the tail of a very long TODO list, or
> outright skip your patch.)
>
>        Ingo
>

I was kind of worried about being exceedingly verbose, but I
understand your point perfectly. I'm going to resend the patch with an
appropriately verbose comment.

As always, I appreciate the criticism. Thank you! :)

- Steven

[PATCH] drivers/serial/8250.c: 'i' may be used uninitialized

[Steven Noonan]

serial_unlink_irq_chain() does not initialize iterator 'i', and that is
correct logically because it is always initialized, either in the
hlist_for_each or in the conditional immediately after (which fires if
hlist_for_each comes up empty-handed). GCC does not realize this
connection and emits a false warning. Annotate it with uninitialized_var().

Signed-off-by: Steven Noonan <steven@uplinklabs.net>
---
 drivers/serial/8250.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/serial/8250.c b/drivers/serial/8250.c
index 356c2a2..4950ee5 100644
--- a/drivers/serial/8250.c
+++ b/drivers/serial/8250.c
@@ -1551,7 +1551,7 @@ static int serial_link_irq_chain(struct uart_8250_port *up)
 {
 	struct hlist_head *h;
 	struct hlist_node *n;
-	struct irq_info *i;
+	struct irq_info *uninitialized_var(i);
 	int ret, irq_flags = up->port.flags & UPF_SHARE_IRQ ? IRQF_SHARED : 0;
 
 	mutex_lock(&hash_mutex);
-- 
1.6.0.2

Re: [PATCH -tip] drivers/serial/8250.c: 'i' may be used uninitialized

[Ingo Molnar]

* Steven Noonan <steven@uplinklabs.net> wrote:

> I was kind of worried about being exceedingly verbose, but I 
> understand your point perfectly. I'm going to resend the patch with an 
> appropriately verbose comment.
> 
> As always, I appreciate the criticism. Thank you! :)

I have yet to meet a too verbose commit log, and i've seen many - so 
there's basically no way you can stretch. Our problem 99% of the time is 
that commit logs are either not verbose at all, or are structured in a 
way that makes it hard to interpret them.

If you think a change is too verbose, you could start using the 
'Impact:' line convention we recently started using in the x86 tree. 
Something like:

 serial, 8250.c: fix warning: 'i' may be used uninitialized

 Impact: cleanup, fix bogus gcc warning

 serial_unlink_irq_chain() does not initialize iterator 'i', and that is
 correct logically because it is always initialized due to XYZ. Gcc does
 not realize this connection and emits a false warning. Annotate it with
 uninitialized_var().

 Signed-off-by: Steven Noonan <steven@uplinklabs.net>

Other good 'Impact:' tags are:

  Impact: fix boot crash
  Impact: style cleanup
  Impact: documentation fix

it's a "see impact at a glance" kind of thing. Maintainers will still 
read the rest and the code as well, but the thought process is much 
smoother: the maintainer can concentrate on "does what the patch does 
meet the expectation spelled out in the changelog", instead of spending 
time on "what does this patch do" thinking.

	Ingo

[PATCH] sdhci: 'scratch' may be used uninitialized

[Steven Noonan]

The variable 'scratch' is always initialized before it's used. The
conditional which is responsible for initialization of 'scratch' will
always evaluate 'true' when the first loop iteration occurs, and thus,
it's properly initialized. GCC doesn't see this, of course, so using
the uninitialized_var() macro seems to work for silencing this case.

Signed-off-by: Steven Noonan <steven@uplinklabs.net>
---
 drivers/mmc/host/sdhci.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index e3a8133..6257677 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -177,7 +177,7 @@ static void sdhci_read_block_pio(struct sdhci_host *host)
 {
 	unsigned long flags;
 	size_t blksize, len, chunk;
-	u32 scratch;
+	u32 uninitialized_var(scratch);
 	u8 *buf;
 
 	DBG("PIO reading\n");
-- 
1.6.0.2

Re: [PATCH -tip] pcm_native: label out defined but not used

[Takashi Iwai]

At Wed, 1 Oct 2008 10:32:24 +0200,
Ingo Molnar wrote:
> 
> 
> * Takashi Iwai <tiwai@suse.de> wrote:
> 
> > At Wed, 1 Oct 2008 09:59:21 +0200,
> > Ingo Molnar wrote:
> > > 
> > > 
> > > (Cc:-ed Taskashi for this ALSA cleanup patch.)
> > 
> > This label is used when the debug option is enabled.
> 
> ah, indeed:
> 
>         snd_assert(substream != NULL, goto out);
> 
> if then i'd suggest to clean it up like this:
> 
>         if (snd_assert(substream != NULL))
> 		goto out;
> 
> this is how DEBUG_LOCKS_WARN_ON() is used in kernel/lockdep.c for 
> example. Putting code flow side-effects into debug macros looks a bit 
> weird.

Yeah, already a similar change was done in the latest ALSA tree :)


thanks,

Takashi

Re: [PATCH -tip] drivers/serial/8250.c: 'i' may be used uninitialized

[Alan Cox]

On Wed, 1 Oct 2008 10:01:50 +0200
Ingo Molnar <mingo@elte.hu> wrote:

> 
> ( Alan Cc:-ed. Seems like gcc bogosity - so your solution of using
>   uninitialized_var() is the correct way to annotate this. )

Sorry but uninitialized_var() is too ugly to be acceptable. Please use a
proper __foo style notation for consistency with the rest of the kernel

(Besides which last time I checked current gcc could figure that one out)

Alan

Re: [PATCH -tip] drivers/serial/8250.c: 'i' may be used uninitialized

[Alan Cox]

On Wed,  1 Oct 2008 01:16:57 -0700
Steven Noonan <steven@uplinklabs.net> wrote:

> Signed-off-by: Steven Noonan <steven@uplinklabs.net>

I'd rather have the warning than that particular ugly in the source
of the serial/tty code. It doesn't fit the pattern of attributes used
elsewhere in the code (__unused __user etc).

Alan

Re: [PATCH -tip] drivers/serial/8250.c: 'i' may be used uninitialized

[Ingo Molnar]

* Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:

> On Wed, 1 Oct 2008 10:01:50 +0200
> Ingo Molnar <mingo@elte.hu> wrote:
> 
> > 
> > ( Alan Cc:-ed. Seems like gcc bogosity - so your solution of using
> >   uninitialized_var() is the correct way to annotate this. )
> 
> Sorry but uninitialized_var() is too ugly to be acceptable. Please use 
> a proper __foo style notation for consistency with the rest of the 
> kernel

it cannot be wrapped like that via __foo style notation (which can be 
used for section tricks), and uninitialized_var() is the accepted 
upstream facility for this, it got introduced 1.5 years ago, via commit 
94909914 ("Add unitialized_var() macro for suppressing gcc warnings").

> (Besides which last time I checked current gcc could figure that one 
> out)

that would indeed moot the patch.

	Ingo

Re: [PATCH -tip] drivers/serial/8250.c: 'i' may be used uninitialized

[Alan Cox]

> it cannot be wrapped like that via __foo style notation (which can be 
> used for section tricks), and uninitialized_var() is the accepted 

Any particular reason that

#define __used		__attribute__((used))

doesn't work ?

> > (Besides which last time I checked current gcc could figure that one 
> > out)
> 
> that would indeed moot the patch.

I'll re-check that before rejecting the patch definitively just to be
sure.

Alan

Re: [PATCH] drivers/serial/8250.c: 'i' may be used uninitialized

[Alan Cox]

On Wed,  1 Oct 2008 01:47:07 -0700
Steven Noonan <steven@uplinklabs.net> wrote:

> serial_unlink_irq_chain() does not initialize iterator 'i', and that is
> correct logically because it is always initialized, either in the
> hlist_for_each or in the conditional immediately after (which fires if
> hlist_for_each comes up empty-handed). GCC does not realize this
> connection and emits a false warning. Annotate it with uninitialized_var().
> 
> Signed-off-by: Steven Noonan <steven@uplinklabs.net>

Ok definitive NAK. gcc 4.3.0 can work this out and doesn't produce a
warning. Thanks for sending the patch though (and to the gcc folks for
rendering it unnecessary)

Alan

Re: [PATCH -tip] drivers/serial/8250.c: 'i' may be used uninitialized

[Ingo Molnar]

* Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:

> > it cannot be wrapped like that via __foo style notation (which can be 
> > used for section tricks), and uninitialized_var() is the accepted 
> 
> Any particular reason that
> 
> #define __used		__attribute__((used))
> 
> doesn't work ?

ah, if that works then it's a very nice idea!

	Ingo

Re: [PATCH] drivers/serial/8250.c: 'i' may be used uninitialized

[Ingo Molnar]

* Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:

> On Wed,  1 Oct 2008 01:47:07 -0700
> Steven Noonan <steven@uplinklabs.net> wrote:
> 
> > serial_unlink_irq_chain() does not initialize iterator 'i', and that is
> > correct logically because it is always initialized, either in the
> > hlist_for_each or in the conditional immediately after (which fires if
> > hlist_for_each comes up empty-handed). GCC does not realize this
> > connection and emits a false warning. Annotate it with uninitialized_var().
> > 
> > Signed-off-by: Steven Noonan <steven@uplinklabs.net>
> 
> Ok definitive NAK. gcc 4.3.0 can work this out and doesn't produce a 
> warning. Thanks for sending the patch though (and to the gcc folks for 
> rendering it unnecessary)

thanks for sorting it out! A CONFIG_CC_DISABLE_WARNING_ANNOTATIONS=y 
might be useful as well, which could be used periodically (by gcc folks) 
to check which warnings are hidden by annotations?

	Ingo

Re: [PATCH] sdhci: 'scratch' may be used uninitialized

[Pierre Ossman]

[-- Attachment #1: Type: text/plain, Size: 754 bytes --]

On Wed,  1 Oct 2008 01:50:25 -0700
Steven Noonan <steven@uplinklabs.net> wrote:

> The variable 'scratch' is always initialized before it's used. The
> conditional which is responsible for initialization of 'scratch' will
> always evaluate 'true' when the first loop iteration occurs, and thus,
> it's properly initialized. GCC doesn't see this, of course, so using
> the uninitialized_var() macro seems to work for silencing this case.
> 
> Signed-off-by: Steven Noonan <steven@uplinklabs.net>

Queued, thanks.

Rgds
-- 
     -- Pierre Ossman

  WARNING: This correspondence is being monitored by the
  Swedish government. Make sure your server uses encryption
  for SMTP traffic and consider using PGP for end-to-end
  encryption.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] sdhci: 'scratch' may be used uninitialized

[Adrian Bunk]

On Wed, Oct 01, 2008 at 01:50:25AM -0700, Steven Noonan wrote:
> The variable 'scratch' is always initialized before it's used. The
> conditional which is responsible for initialization of 'scratch' will
> always evaluate 'true' when the first loop iteration occurs, and thus,
> it's properly initialized. GCC doesn't see this, of course, so using
> the uninitialized_var() macro seems to work for silencing this case.
> 
> Signed-off-by: Steven Noonan <steven@uplinklabs.net>
> ---
>  drivers/mmc/host/sdhci.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index e3a8133..6257677 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -177,7 +177,7 @@ static void sdhci_read_block_pio(struct sdhci_host *host)
>  {
>  	unsigned long flags;
>  	size_t blksize, len, chunk;
> -	u32 scratch;
> +	u32 uninitialized_var(scratch);
>...

With which gcc version?

I'm not getting this warning with gcc 4.3, and IMHO it doesn't make 
sense to clutter the source code with such workarounds for older gcc 
versions (we officially support 6 years old compilers, and warning-free 
compilations with all of them are not reasonably possible).

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: [PATCH] sdhci: 'scratch' may be used uninitialized

[Steven Noonan]

On Sun, Oct 5, 2008 at 7:28 AM, Adrian Bunk <bunk@kernel.org> wrote:
> On Wed, Oct 01, 2008 at 01:50:25AM -0700, Steven Noonan wrote:
>> The variable 'scratch' is always initialized before it's used. The
>> conditional which is responsible for initialization of 'scratch' will
>> always evaluate 'true' when the first loop iteration occurs, and thus,
>> it's properly initialized. GCC doesn't see this, of course, so using
>> the uninitialized_var() macro seems to work for silencing this case.
>>
>> Signed-off-by: Steven Noonan <steven@uplinklabs.net>
>> ---
>>  drivers/mmc/host/sdhci.c |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>> index e3a8133..6257677 100644
>> --- a/drivers/mmc/host/sdhci.c
>> +++ b/drivers/mmc/host/sdhci.c
>> @@ -177,7 +177,7 @@ static void sdhci_read_block_pio(struct sdhci_host *host)
>>  {
>>       unsigned long flags;
>>       size_t blksize, len, chunk;
>> -     u32 scratch;
>> +     u32 uninitialized_var(scratch);
>>...
>
> With which gcc version?
>
> I'm not getting this warning with gcc 4.3, and IMHO it doesn't make
> sense to clutter the source code with such workarounds for older gcc
> versions (we officially support 6 years old compilers, and warning-free
> compilations with all of them are not reasonably possible).
>
> cu
> Adrian

I've seen it on GCC 4.1 and 4.2. Since lots of distributions still
haven't marked GCC >4.1 stable, it makes sense to me to kill warnings
for GCC 4.1 and above. I don't know of any current distribution
releases using less than GCC 4.1 at the moment.

- Steven

Re: [PATCH] sdhci: 'scratch' may be used uninitialized

[Adrian Bunk]

On Sun, Oct 05, 2008 at 03:53:28PM -0700, Steven Noonan wrote:
> On Sun, Oct 5, 2008 at 7:28 AM, Adrian Bunk <bunk@kernel.org> wrote:
> > On Wed, Oct 01, 2008 at 01:50:25AM -0700, Steven Noonan wrote:
> >> The variable 'scratch' is always initialized before it's used. The
> >> conditional which is responsible for initialization of 'scratch' will
> >> always evaluate 'true' when the first loop iteration occurs, and thus,
> >> it's properly initialized. GCC doesn't see this, of course, so using
> >> the uninitialized_var() macro seems to work for silencing this case.
> >>
> >> Signed-off-by: Steven Noonan <steven@uplinklabs.net>
> >> ---
> >>  drivers/mmc/host/sdhci.c |    2 +-
> >>  1 files changed, 1 insertions(+), 1 deletions(-)
> >>
> >> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> >> index e3a8133..6257677 100644
> >> --- a/drivers/mmc/host/sdhci.c
> >> +++ b/drivers/mmc/host/sdhci.c
> >> @@ -177,7 +177,7 @@ static void sdhci_read_block_pio(struct sdhci_host *host)
> >>  {
> >>       unsigned long flags;
> >>       size_t blksize, len, chunk;
> >> -     u32 scratch;
> >> +     u32 uninitialized_var(scratch);
> >>...
> >
> > With which gcc version?
> >
> > I'm not getting this warning with gcc 4.3, and IMHO it doesn't make
> > sense to clutter the source code with such workarounds for older gcc
> > versions (we officially support 6 years old compilers, and warning-free
> > compilations with all of them are not reasonably possible).
> >
> > cu
> > Adrian
> 
> I've seen it on GCC 4.1 and 4.2. Since lots of distributions still
> haven't marked GCC >4.1 stable, it makes sense to me to kill warnings
> for GCC 4.1 and above. I don't know of any current distribution
> releases using less than GCC 4.1 at the moment.

It will clutter our code with these workarounds forever.

And due to silencing these false warnings we will no longer get a 
warning when one of them becomes a real bug.

Working on the remaining warnings that are visible with gcc 4.3 is a 
worthwhile goal, but I see no point for silencing some warnings that 
only occur with older gcc versions (especially as long as warnings 
that are present with all gcc versions stay unfixed).

> - Steven

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: [PATCH] sdhci: 'scratch' may be used uninitialized

[Steven Noonan]

On Sun, Oct 5, 2008 at 4:16 PM, Adrian Bunk <bunk@kernel.org> wrote:
> On Sun, Oct 05, 2008 at 03:53:28PM -0700, Steven Noonan wrote:
>> On Sun, Oct 5, 2008 at 7:28 AM, Adrian Bunk <bunk@kernel.org> wrote:
>> > On Wed, Oct 01, 2008 at 01:50:25AM -0700, Steven Noonan wrote:
>> >> The variable 'scratch' is always initialized before it's used. The
>> >> conditional which is responsible for initialization of 'scratch' will
>> >> always evaluate 'true' when the first loop iteration occurs, and thus,
>> >> it's properly initialized. GCC doesn't see this, of course, so using
>> >> the uninitialized_var() macro seems to work for silencing this case.
>> >>
>> >> Signed-off-by: Steven Noonan <steven@uplinklabs.net>
>> >> ---
>> >>  drivers/mmc/host/sdhci.c |    2 +-
>> >>  1 files changed, 1 insertions(+), 1 deletions(-)
>> >>
>> >> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>> >> index e3a8133..6257677 100644
>> >> --- a/drivers/mmc/host/sdhci.c
>> >> +++ b/drivers/mmc/host/sdhci.c
>> >> @@ -177,7 +177,7 @@ static void sdhci_read_block_pio(struct sdhci_host *host)
>> >>  {
>> >>       unsigned long flags;
>> >>       size_t blksize, len, chunk;
>> >> -     u32 scratch;
>> >> +     u32 uninitialized_var(scratch);
>> >>...
>> >
>> > With which gcc version?
>> >
>> > I'm not getting this warning with gcc 4.3, and IMHO it doesn't make
>> > sense to clutter the source code with such workarounds for older gcc
>> > versions (we officially support 6 years old compilers, and warning-free
>> > compilations with all of them are not reasonably possible).
>> >
>> > cu
>> > Adrian
>>
>> I've seen it on GCC 4.1 and 4.2. Since lots of distributions still
>> haven't marked GCC >4.1 stable, it makes sense to me to kill warnings
>> for GCC 4.1 and above. I don't know of any current distribution
>> releases using less than GCC 4.1 at the moment.
>
> It will clutter our code with these workarounds forever.
>
> And due to silencing these false warnings we will no longer get a
> warning when one of them becomes a real bug.
>
> Working on the remaining warnings that are visible with gcc 4.3 is a
> worthwhile goal, but I see no point for silencing some warnings that
> only occur with older gcc versions (especially as long as warnings
> that are present with all gcc versions stay unfixed).
>
I feel like there's a logical fallacy here. Sure, we can fix GCC 4.3
warnings, but what about when GCC 4.3 becomes an "old version"?
uninitialized_var and other such workarounds will still exist in the
code. It seems like the logical progression of your argument should be
to never fix false warnings.

- Steven

Re: [PATCH] sdhci: 'scratch' may be used uninitialized

[Adrian Bunk]

On Sun, Oct 05, 2008 at 04:48:49PM -0700, Steven Noonan wrote:
> On Sun, Oct 5, 2008 at 4:16 PM, Adrian Bunk <bunk@kernel.org> wrote:
> > On Sun, Oct 05, 2008 at 03:53:28PM -0700, Steven Noonan wrote:
> >> On Sun, Oct 5, 2008 at 7:28 AM, Adrian Bunk <bunk@kernel.org> wrote:
> >> > On Wed, Oct 01, 2008 at 01:50:25AM -0700, Steven Noonan wrote:
> >> >> The variable 'scratch' is always initialized before it's used. The
> >> >> conditional which is responsible for initialization of 'scratch' will
> >> >> always evaluate 'true' when the first loop iteration occurs, and thus,
> >> >> it's properly initialized. GCC doesn't see this, of course, so using
> >> >> the uninitialized_var() macro seems to work for silencing this case.
> >> >>
> >> >> Signed-off-by: Steven Noonan <steven@uplinklabs.net>
> >> >> ---
> >> >>  drivers/mmc/host/sdhci.c |    2 +-
> >> >>  1 files changed, 1 insertions(+), 1 deletions(-)
> >> >>
> >> >> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> >> >> index e3a8133..6257677 100644
> >> >> --- a/drivers/mmc/host/sdhci.c
> >> >> +++ b/drivers/mmc/host/sdhci.c
> >> >> @@ -177,7 +177,7 @@ static void sdhci_read_block_pio(struct sdhci_host *host)
> >> >>  {
> >> >>       unsigned long flags;
> >> >>       size_t blksize, len, chunk;
> >> >> -     u32 scratch;
> >> >> +     u32 uninitialized_var(scratch);
> >> >>...
> >> >
> >> > With which gcc version?
> >> >
> >> > I'm not getting this warning with gcc 4.3, and IMHO it doesn't make
> >> > sense to clutter the source code with such workarounds for older gcc
> >> > versions (we officially support 6 years old compilers, and warning-free
> >> > compilations with all of them are not reasonably possible).
> >> >
> >> > cu
> >> > Adrian
> >>
> >> I've seen it on GCC 4.1 and 4.2. Since lots of distributions still
> >> haven't marked GCC >4.1 stable, it makes sense to me to kill warnings
> >> for GCC 4.1 and above. I don't know of any current distribution
> >> releases using less than GCC 4.1 at the moment.
> >
> > It will clutter our code with these workarounds forever.
> >
> > And due to silencing these false warnings we will no longer get a
> > warning when one of them becomes a real bug.
> >
> > Working on the remaining warnings that are visible with gcc 4.3 is a
> > worthwhile goal, but I see no point for silencing some warnings that
> > only occur with older gcc versions (especially as long as warnings
> > that are present with all gcc versions stay unfixed).
> >
> I feel like there's a logical fallacy here. Sure, we can fix GCC 4.3
> warnings, but what about when GCC 4.3 becomes an "old version"?
> uninitialized_var and other such workarounds will still exist in the
> code. It seems like the logical progression of your argument should be
> to never fix false warnings.

There is no logical fallacy here - getting a warning-free compilation 
with the latest gcc (so that new warnings get more obvious) makes sense,
but workarounds for warnings only present with older gcc versions are 
not worth the price.

If compilation with gcc 4.3 was always warning-free you might have a 
point, but considering that it's unlikely that we get all warnings with 
gcc 4.3 fixed in the forseeable future [1] it would make more sense to 
fix one of the non-trivial warnings present with all gcc version than 
clutter the code with workarounds for warnings only present with older 
gcc versions.

> - Steven

cu
Adrian

[1] e.g. the MCA legacy stuff gives #warning's since
    more than 5 years (sic)

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: [PATCH] sdhci: 'scratch' may be used uninitialized

[Ingo Molnar]

* Steven Noonan <steven@uplinklabs.net> wrote:

> On Sun, Oct 5, 2008 at 4:16 PM, Adrian Bunk <bunk@kernel.org> wrote:
> > On Sun, Oct 05, 2008 at 03:53:28PM -0700, Steven Noonan wrote:
> >> On Sun, Oct 5, 2008 at 7:28 AM, Adrian Bunk <bunk@kernel.org> wrote:
> >> > On Wed, Oct 01, 2008 at 01:50:25AM -0700, Steven Noonan wrote:
> >> >> The variable 'scratch' is always initialized before it's used. The
> >> >> conditional which is responsible for initialization of 'scratch' will
> >> >> always evaluate 'true' when the first loop iteration occurs, and thus,
> >> >> it's properly initialized. GCC doesn't see this, of course, so using
> >> >> the uninitialized_var() macro seems to work for silencing this case.
> >> >>
> >> >> Signed-off-by: Steven Noonan <steven@uplinklabs.net>
> >> >> ---
> >> >>  drivers/mmc/host/sdhci.c |    2 +-
> >> >>  1 files changed, 1 insertions(+), 1 deletions(-)
> >> >>
> >> >> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> >> >> index e3a8133..6257677 100644
> >> >> --- a/drivers/mmc/host/sdhci.c
> >> >> +++ b/drivers/mmc/host/sdhci.c
> >> >> @@ -177,7 +177,7 @@ static void sdhci_read_block_pio(struct sdhci_host *host)
> >> >>  {
> >> >>       unsigned long flags;
> >> >>       size_t blksize, len, chunk;
> >> >> -     u32 scratch;
> >> >> +     u32 uninitialized_var(scratch);
> >> >>...
> >> >
> >> > With which gcc version?
> >> >
> >> > I'm not getting this warning with gcc 4.3, and IMHO it doesn't make
> >> > sense to clutter the source code with such workarounds for older gcc
> >> > versions (we officially support 6 years old compilers, and warning-free
> >> > compilations with all of them are not reasonably possible).
> >> >
> >> > cu
> >> > Adrian
> >>
> >> I've seen it on GCC 4.1 and 4.2. Since lots of distributions still
> >> haven't marked GCC >4.1 stable, it makes sense to me to kill warnings
> >> for GCC 4.1 and above. I don't know of any current distribution
> >> releases using less than GCC 4.1 at the moment.
> >
> > It will clutter our code with these workarounds forever.
> >
> > And due to silencing these false warnings we will no longer get a
> > warning when one of them becomes a real bug.
> >
> > Working on the remaining warnings that are visible with gcc 4.3 is a
> > worthwhile goal, but I see no point for silencing some warnings that
> > only occur with older gcc versions (especially as long as warnings
> > that are present with all gcc versions stay unfixed).
> >
> I feel like there's a logical fallacy here. Sure, we can fix GCC 4.3
> warnings, but what about when GCC 4.3 becomes an "old version"?
> uninitialized_var and other such workarounds will still exist in the
> code. It seems like the logical progression of your argument should be
> to never fix false warnings.

Correct. Would you be interested in sending a patch for a (default-off) 
debug feature that allows the disabling of all the gcc annotations? That 
way we can do regular sweeps to determine whether old annotations are 
still relevant on latest and greatest GCC.

Something like CONFIG_CC_DEBUG_ALLOW_WARNINGS=y in lib/Kconfig.debug, 
then use that to #ifdef the uninitialized_var() 
include/linux/compiler-gcc[34].h?

Also, please try Alan's suggestion as well: does the __attribute_ 
((unused)) trick work equally well? If yes then please introduce a 
__annotate_initialized tag instead of the weird-looking 
uninitialized_var() construct.

	Ingo

Re: [PATCH] sdhci: 'scratch' may be used uninitialized

[Pierre Ossman]

[-- Attachment #1: Type: text/plain, Size: 820 bytes --]

On Mon, 6 Oct 2008 08:30:35 +0200
Ingo Molnar <mingo@elte.hu> wrote:

> 
> Correct. Would you be interested in sending a patch for a (default-off) 
> debug feature that allows the disabling of all the gcc annotations? That 
> way we can do regular sweeps to determine whether old annotations are 
> still relevant on latest and greatest GCC.
> 

How would you find the ones that are no longer needed though?

Perhaps we could make it so that uninitialized_var() itself emits a
warning. That way you could turn that option on and check that you
always have pairs of warnings.

Rgds
-- 
     -- Pierre Ossman

  WARNING: This correspondence is being monitored by the
  Swedish government. Make sure your server uses encryption
  for SMTP traffic and consider using PGP for end-to-end
  encryption.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] sdhci: 'scratch' may be used uninitialized

[Ingo Molnar]

* Pierre Ossman <drzeus@drzeus.cx> wrote:

> > Correct. Would you be interested in sending a patch for a 
> > (default-off) debug feature that allows the disabling of all the gcc 
> > annotations? That way we can do regular sweeps to determine whether 
> > old annotations are still relevant on latest and greatest GCC.
> 
> How would you find the ones that are no longer needed though?

ideally gcc should not emit _any_ bogus warning. Life is too short to 
comb through crappy warnings and to keep in mind which ones are relevant 
and which ones are not. compiler-intel.h does not make use of the 
annotations for example.

> Perhaps we could make it so that uninitialized_var() itself emits a 
> warning. That way you could turn that option on and check that you 
> always have pairs of warnings.

ok - when CONFIG_CC_DEBUG_ALLOW_WARNINGS=y - and then the work flow 
would be to go for single-occurance warnings and eliminate them (because 
their annotation is moot).

	Ingo