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

--

[ message continues ]

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

--

[ message continues ]

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

--

[ message continues ]

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

--

[ message continues ]

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

--

[ message continues ]

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
--

[ message continues ]

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
--

[ message continues ]

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

--

[ message continues ]

On Wed,  1 Oct 2008 01:47:07 -0700

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
--

[ message continues ]

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
--

[ message continues ]

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
--

[ message continues ]

On Wed,  1 Oct 2008 01:16:57 -0700

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
--

[ message continues ]

On Wed, 1 Oct 2008 10:01:50 +0200

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
--

[ message continues ]

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 

that would indeed moot the patch.

	Ingo
--

[ message continues ]

Any particular reason that

#define __used		__attribute__((used))


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

Alan
--

[ message continues ]

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

	Ingo
--

[ message continues ]

(Cc:-ed Pierre Ossman)

--

[ message continues ]

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

--

[ message continues ]

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
--

[ message continues ]

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
--

[ message continues ]

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

--

[ message continues ]

On Wed,  1 Oct 2008 01:50:25 -0700

Queued, thanks.

Rgds
--=20
     -- 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.
[ message continues ]

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

--

[ message continues ]

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
--

[ message continues ]

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 

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

--

[ message continues ]

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
--

[ message continues ]

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 

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

--

[ message continues ]

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
--

[ message continues ]

On Mon, 6 Oct 2008 08:30:35 +0200

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
--=20
     -- 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.
[ message continues ]

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 

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
--

[ message continues ]

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

--

[ message continues ]

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

--

[ message continues ]

At Wed, 1 Oct 2008 09:59:21 +0200,

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

thanks,

--

[ message continues ]

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
--

[ message continues ]

At Wed, 1 Oct 2008 10:32:24 +0200,

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


thanks,

Takashi
--

[ message continues ]