Signed-off-by: Steven Noonan <[email protected]>
---
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
Signed-off-by: Steven Noonan <[email protected]>
---
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
Signed-off-by: Steven Noonan <[email protected]>
---
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
(Cc:-ed Taskashi for this ALSA cleanup patch.)
* Steven Noonan <[email protected]> wrote:
> Signed-off-by: Steven Noonan <[email protected]>
> ---
> 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
(Cc:-ed Pierre Ossman)
* Steven Noonan <[email protected]> wrote:
> Signed-off-by: Steven Noonan <[email protected]>
> ---
> 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
( Alan Cc:-ed. Seems like gcc bogosity - so your solution of using
uninitialized_var() is the correct way to annotate this. )
* Steven Noonan <[email protected]> wrote:
> Signed-off-by: Steven Noonan <[email protected]>
> ---
> 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
Signed-off-by: Steven Noonan <[email protected]>
---
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
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 <[email protected]> wrote:
>
> > Signed-off-by: Steven Noonan <[email protected]>
> > ---
> > 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
>
Signed-off-by: Steven Noonan <[email protected]>
---
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
Signed-off-by: Steven Noonan <[email protected]>
---
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
* Steven Noonan <[email protected]> wrote:
> Signed-off-by: Steven Noonan <[email protected]>
> ---
> 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
* Takashi Iwai <[email protected]> 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
On Wed, Oct 1, 2008 at 1:14 AM, Steven Noonan <[email protected]> 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
* Steven Noonan <[email protected]> wrote:
> On Wed, Oct 1, 2008 at 1:14 AM, Steven Noonan <[email protected]> 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
On Wed, Oct 1, 2008 at 1:29 AM, Ingo Molnar <[email protected]> wrote:
>
> * Steven Noonan <[email protected]> wrote:
>
>> Signed-off-by: Steven Noonan <[email protected]>
>> ---
>> 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
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 <[email protected]>
---
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
* Steven Noonan <[email protected]> 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 <[email protected]>
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
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 <[email protected]>
---
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
At Wed, 1 Oct 2008 10:32:24 +0200,
Ingo Molnar wrote:
>
>
> * Takashi Iwai <[email protected]> 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
On Wed, 1 Oct 2008 10:01:50 +0200
Ingo Molnar <[email protected]> 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
On Wed, 1 Oct 2008 01:16:57 -0700
Steven Noonan <[email protected]> wrote:
> Signed-off-by: Steven Noonan <[email protected]>
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
* Alan Cox <[email protected]> wrote:
> On Wed, 1 Oct 2008 10:01:50 +0200
> Ingo Molnar <[email protected]> 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
> 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
On Wed, 1 Oct 2008 01:47:07 -0700
Steven Noonan <[email protected]> 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 <[email protected]>
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
* Alan Cox <[email protected]> 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
* Alan Cox <[email protected]> wrote:
> On Wed, 1 Oct 2008 01:47:07 -0700
> Steven Noonan <[email protected]> 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 <[email protected]>
>
> 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
On Wed, 1 Oct 2008 01:50:25 -0700
Steven Noonan <[email protected]> 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 <[email protected]>
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.
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 <[email protected]>
> ---
> 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
On Sun, Oct 5, 2008 at 7:28 AM, Adrian Bunk <[email protected]> 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 <[email protected]>
>> ---
>> 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
On Sun, Oct 05, 2008 at 03:53:28PM -0700, Steven Noonan wrote:
> On Sun, Oct 5, 2008 at 7:28 AM, Adrian Bunk <[email protected]> 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 <[email protected]>
> >> ---
> >> 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
On Sun, Oct 5, 2008 at 4:16 PM, Adrian Bunk <[email protected]> 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 <[email protected]> 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 <[email protected]>
>> >> ---
>> >> 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
On Sun, Oct 05, 2008 at 04:48:49PM -0700, Steven Noonan wrote:
> On Sun, Oct 5, 2008 at 4:16 PM, Adrian Bunk <[email protected]> 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 <[email protected]> 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 <[email protected]>
> >> >> ---
> >> >> 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
* Steven Noonan <[email protected]> wrote:
> On Sun, Oct 5, 2008 at 4:16 PM, Adrian Bunk <[email protected]> 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 <[email protected]> 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 <[email protected]>
> >> >> ---
> >> >> 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
On Mon, 6 Oct 2008 08:30:35 +0200
Ingo Molnar <[email protected]> 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.
* Pierre Ossman <[email protected]> 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