[PATCH 0/7] trivial: fix some compilation warnings for 2.6..

Felipe Contreras · External Since: Oct 07, 2009 Posts: 13

These fixes almost all the warnings I get while compiling a custom kernel for
my laptop.

I'm not so sure about adding __cpuinit to acpi_processor_add() but otherwise
these should be safe to apply.

Felipe Contreras (7):
usb: trivial cleanups
ipc: fix trivial warning
crypto: testmgr: fix warning
acpi: processor: fix section mismatch
acpi: fix a bunch of style issues on 'actypes.h'
acpi: fix trivial warning
acpi: fix trivial warnings caused by previous commmit

crypto/testmgr.c | 2 +-
drivers/acpi/acpica/exfldio.c | 8 ++++----
drivers/acpi/processor_core.c | 2 +-
drivers/usb/core/hcd.c | 7 +++----
include/acpi/actypes.h | 36 ++++++++++++++++++------------------
ipc/msg.c | 2 +-
6 files changed, 28 insertions(+), 29 deletions(-)

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo.DeleteThis@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Felipe Contreras · External Since: Oct 07, 2009 Posts: 13

On Mon, Oct 19, 2009 at 1:54 AM, Felipe Contreras
<felipe.contreras.TakeThisOut@gmail.com> wrote:
> It seems 'min_t' was used before, and looks cleaner, plus white-space
> stuff.
>
> Signed-off-by: Felipe Contreras <felipe.contreras.TakeThisOut@gmail.com>
> ---
> drivers/usb/core/hcd.c | 7 +++----
> 1 files changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> index 34de475..e6d754e 100644
> --- a/drivers/usb/core/hcd.c
> +++ b/drivers/usb/core/hcd.c
> @@ -396,8 +396,7 @@ rh_string(int id, struct usb_hcd const *hcd, u8 *data, unsigned len)
> case 0:
> /* Array of LANGID codes (0x0409 is MSFT-speak for "en-us") */
> /* See http://www.usb.org/developers/docs/USB_LANGIDs.pdf */
> - if (len > 4)
> - len = 4;
> + len = min_t(unsigned, len, sizeof(langids));
> memcpy(data, langids, len);
> return len;
> case 1:

I still have an unfixed warning:
drivers/usb/core/hcd.c: In function 'rh_string':
/data/public/src/linux/arch/x86/include/asm/string_32.h:74: warning:
array subscript is above array bounds

Apparently there's a problem with the optimized memcpy for x86 with this code:
static char const langids[4] = {4, USB_DT_STRING, 0x09, 0x04};
len = min_t(unsigned, len, sizeof(langids));
memcpy(data, langids, len);
return len;

gcc 4.4 is trying to optimize the memcpy, but it's not able to realize
that 'len' will always be <= 4 and the memcpy will not exceed langids.
One way to solve this is by replacing len with the min_t expression:
memcpy(data, langids, min_t(unsigned, len, sizeof(langids)));

However, that looks ugly and we need the expression again for the
return. Another way is to remove 'const' from langids.

AFAIK the code is perfectly correct as it is, I think the fact that
gcc 4.4 complains is a bug on gcc side.

Cheers.

--
Felipe Contreras
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo.TakeThisOut@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Greg KH · External Since: Nov 18, 2004 Posts: 1094

On Mon, Oct 19, 2009 at 01:54:28AM +0300, Felipe Contreras wrote:
> It seems 'min_t' was used before, and looks cleaner, plus white-space
> stuff.
>
> Signed-off-by: Felipe Contreras <felipe.contreras RemoveThis @gmail.com>

But this doesn't fix a kernel warning, does it? If so, what one?

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo RemoveThis @vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Felipe Contreras · External Since: Oct 07, 2009 Posts: 13

On Mon, Oct 19, 2009 at 3:02 AM, Greg KH <greg.RemoveThis@kroah.com> wrote:
> On Mon, Oct 19, 2009 at 01:54:28AM +0300, Felipe Contreras wrote:
>> It seems 'min_t' was used before, and looks cleaner, plus white-space
>> stuff.
>>
>> Signed-off-by: Felipe Contreras <felipe.contreras.RemoveThis@gmail.com>
>
> But this doesn't fix a kernel warning, does it? If so, what one?

No, it's just cleanups I noticed while trying to fix the warning I
described here:
http://lkml.org/lkml/2009/10/18/126

--
Felipe Contreras
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo.RemoveThis@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Alan Stern · External Since: May 20, 2006 Posts: 376

On Mon, 19 Oct 2009, Felipe Contreras wrote:

> I still have an unfixed warning:
> drivers/usb/core/hcd.c: In function â€˜rh_stringâ€™:
> /data/public/src/linux/arch/x86/include/asm/string_32.h:74: warning:
> array subscript is above array bounds
>
> Apparently there's a problem with the optimized memcpy for x86 with this code:
> static char const langids[4] = {4, USB_DT_STRING, 0x09, 0x04};
> len = min_t(unsigned, len, sizeof(langids));
> memcpy(data, langids, len);
> return len;
>
> gcc 4.4 is trying to optimize the memcpy, but it's not able to realize
> that 'len' will always be <= 4 and the memcpy will not exceed langids.
> One way to solve this is by replacing len with the min_t expression:
> memcpy(data, langids, min_t(unsigned, len, sizeof(langids)));
>
> However, that looks ugly and we need the expression again for the
> return. Another way is to remove 'const' from langids.
>
> AFAIK the code is perfectly correct as it is, I think the fact that
> gcc 4.4 complains is a bug on gcc side.

Yes, it is a well-known bug in gcc. Other places in the kernel get
similar warnings.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo.RemoveThis@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Jiri Kosina · External Since: Nov 29, 2006 Posts: 142

On Mon, 19 Oct 2009, Felipe Contreras wrote:

> crypto/testmgr.c: In function ?test_cprng?:
> crypto/testmgr.c:1204: warning: ?err? may be used uninitialized in this function
>
> Signed-off-by: Felipe Contreras <felipe.contreras.RemoveThis@gmail.com>
> ---
> crypto/testmgr.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/crypto/testmgr.c b/crypto/testmgr.c
> index 6d5b746..1f2357b 100644
> --- a/crypto/testmgr.c
> +++ b/crypto/testmgr.c
> @@ -1201,7 +1201,7 @@ static int test_cprng(struct crypto_rng *tfm, struct cprng_testvec *template,
> unsigned int tcount)
> {
> const char *algo = crypto_tfm_alg_driver_name(crypto_rng_tfm(tfm));
> - int err, i, j, seedsize;
> + int err = 0, i, j, seedsize;
> u8 *seed;
> char result[32];

As it is not obvious to me immediately why/whether tcount couldn't be zero
(which would cause uninitialized use of 'err'), I am not merging this
through trivial tree. Herbert?

--
Jiri Kosina
SUSE Labs, Novell Inc.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo.RemoveThis@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Jarod Wilson · External Since: Apr 06, 2009 Posts: 8

On 10/19/09 9:52 AM, Jiri Kosina wrote:
> On Mon, 19 Oct 2009, Felipe Contreras wrote:
>
>> crypto/testmgr.c: In function ?test_cprng?:
>> crypto/testmgr.c:1204: warning: ?err? may be used uninitialized in this function
>>
>> Signed-off-by: Felipe Contreras<felipe.contreras.TakeThisOut@gmail.com>
>> ---
>> crypto/testmgr.c | 2 +-
>> 1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/crypto/testmgr.c b/crypto/testmgr.c
>> index 6d5b746..1f2357b 100644
>> --- a/crypto/testmgr.c
>> +++ b/crypto/testmgr.c
>> @@ -1201,7 +1201,7 @@ static int test_cprng(struct crypto_rng *tfm, struct cprng_testvec *template,
>> unsigned int tcount)
>> {
>> const char *algo = crypto_tfm_alg_driver_name(crypto_rng_tfm(tfm));
>> - int err, i, j, seedsize;
>> + int err = 0, i, j, seedsize;
>> u8 *seed;
>> char result[32];
>
> As it is not obvious to me immediately why/whether tcount couldn't be zero
> (which would cause uninitialized use of 'err'), I am not merging this
> through trivial tree. Herbert?

I believe I'm the guilty party who wrote the code in question.
Initializing err to 0 isn't correct. tcount should always be at least 1,
if its 0, test_cprng has been called with invalid parameters. I believe
err would best be initialized to -EINVAL, lest the caller think they
were successful.

--
Jarod Wilson
jarod.TakeThisOut@redhat.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo.TakeThisOut@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Jarod Wilson · External Since: Apr 06, 2009 Posts: 8

On 10/19/09 9:58 AM, Jarod Wilson wrote:
> On 10/19/09 9:52 AM, Jiri Kosina wrote:
>> On Mon, 19 Oct 2009, Felipe Contreras wrote:
>>
>>> crypto/testmgr.c: In function ?test_cprng?:
>>> crypto/testmgr.c:1204: warning: ?err? may be used uninitialized in
>>> this function
>>>
>>> Signed-off-by: Felipe Contreras<felipe.contreras.RemoveThis@gmail.com>
>>> ---
>>> crypto/testmgr.c | 2 +-
>>> 1 files changed, 1 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/crypto/testmgr.c b/crypto/testmgr.c
>>> index 6d5b746..1f2357b 100644
>>> --- a/crypto/testmgr.c
>>> +++ b/crypto/testmgr.c
>>> @@ -1201,7 +1201,7 @@ static int test_cprng(struct crypto_rng *tfm,
>>> struct cprng_testvec *template,
>>> unsigned int tcount)
>>> {
>>> const char *algo = crypto_tfm_alg_driver_name(crypto_rng_tfm(tfm));
>>> - int err, i, j, seedsize;
>>> + int err = 0, i, j, seedsize;
>>> u8 *seed;
>>> char result[32];
>>
>> As it is not obvious to me immediately why/whether tcount couldn't be
>> zero
>> (which would cause uninitialized use of 'err'), I am not merging this
>> through trivial tree. Herbert?
>
> I believe I'm the guilty party who wrote the code in question.
> Initializing err to 0 isn't correct. tcount should always be at least 1,
> if its 0, test_cprng has been called with invalid parameters. I believe
> err would best be initialized to -EINVAL, lest the caller think they
> were successful.

....and I need to re-read said code more carefully. tcount is
desc->suite.cprng.count, which is ANSI_CPRNG_AES_TEST_VECTORS, which is
#define'd to 6, and is the count of how many cprng test vectors there
are. If someone changes that to 0, then I guess a retval of 0 would
actually be correct, since no tests failed...

So yeah, I rescind my claim that initializing err to 0 is incorrect, I
think that's just fine.

--
Jarod Wilson
jarod.RemoveThis@redhat.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo.RemoveThis@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Felipe Contreras · External Since: Oct 07, 2009 Posts: 13

On Mon, Oct 19, 2009 at 4:59 PM, Alan Stern <stern.RemoveThis@rowland.harvard.edu> wrote:
> On Mon, 19 Oct 2009, Felipe Contreras wrote:
>
>> I still have an unfixed warning:
>> drivers/usb/core/hcd.c: In function 'rh_string':
>> /data/public/src/linux/arch/x86/include/asm/string_32.h:74: warning:
>> array subscript is above array bounds
>>
>> Apparently there's a problem with the optimized memcpy for x86 with this code:
>> static char const langids[4] = {4, USB_DT_STRING, 0x09, 0x04};
>> len = min_t(unsigned, len, sizeof(langids));
>> memcpy(data, langids, len);
>> return len;
>>
>> gcc 4.4 is trying to optimize the memcpy, but it's not able to realize
>> that 'len' will always be <= 4 and the memcpy will not exceed langids.
>> One way to solve this is by replacing len with the min_t expression:
>> memcpy(data, langids, min_t(unsigned, len, sizeof(langids)));
>>
>> However, that looks ugly and we need the expression again for the
>> return. Another way is to remove 'const' from langids.
>>
>> AFAIK the code is perfectly correct as it is, I think the fact that
>> gcc 4.4 complains is a bug on gcc side.
>
> Yes, it is a well-known bug in gcc. Other places in the kernel get
> similar warnings.

So gcc guys are aware of this? Is there a bug report or something?

--
Felipe Contreras
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo.RemoveThis@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Jiri Kosina · External Since: Nov 29, 2006 Posts: 142

On Mon, 19 Oct 2009, Felipe Contreras wrote:

> Commit a0d092f introduced the following warning:
> ipc/msg.c: In function ?msgctl_down?:
> ipc/msg.c:415: warning: ?msqid64? may be used uninitialized in this function
>
> Signed-off-by: Felipe Contreras <felipe.contreras DeleteThis @gmail.com>
> ---
> ipc/msg.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/ipc/msg.c b/ipc/msg.c
> index 2ceab7f..085bd58 100644
> --- a/ipc/msg.c
> +++ b/ipc/msg.c
> @@ -412,7 +412,7 @@ static int msgctl_down(struct ipc_namespace *ns, int msqid, int cmd,
> struct msqid_ds __user *buf, int version)
> {
> struct kern_ipc_perm *ipcp;
> - struct msqid64_ds msqid64;
> + struct msqid64_ds uninitialized_var(msqid64);
> struct msg_queue *msq;
> int err;

What gcc are you using? I am not getting any warning at least with gcc
"(SUSE Linux) 4.3.1 20080507 (prerelease) [gcc-4_3-branch revision
135036]"

$ make ipc/msg.o
CHK include/linux/version.h
CHK include/linux/utsrelease.h
SYMLINK include/asm -> include/asm-x86
CALL scripts/checksyscalls.sh
CC ipc/msg.o
$

--
Jiri Kosina
SUSE Labs, Novell Inc.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo DeleteThis @vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Jiri Kosina · External Since: Nov 29, 2006 Posts: 142

On Mon, 19 Oct 2009, Felipe Contreras wrote:

> drivers/acpi/acpica/tbfadt.c: In function ?acpi_tb_create_local_fadt?:
> arch/x86/include/asm/string_32.h:74: warning: array subscript is above array bounds

Same is in your previous patch -- what gcc is that? I don't see any
warning with 4.3.1

$ make drivers/acpi/acpica/tbfadt.o
CHK include/linux/version.h
CHK include/linux/utsrelease.h
SYMLINK include/asm -> include/asm-x86
CALL scripts/checksyscalls.sh
CC drivers/acpi/acpica/tbfadt.o
$

--
Jiri Kosina
SUSE Labs, Novell Inc.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo DeleteThis @vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Alan Stern · External Since: May 20, 2006 Posts: 376

On Mon, 19 Oct 2009, Felipe Contreras wrote:

> >> AFAIK the code is perfectly correct as it is, I think the fact that
> >> gcc 4.4 complains is a bug on gcc side.
> >
> > Yes, it is a well-known bug in gcc. Â Other places in the kernel get
> > similar warnings.
>
> So gcc guys are aware of this? Is there a bug report or something?

I don't know. You should ask them.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo.RemoveThis@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Jiri Kosina · External Since: Nov 29, 2006 Posts: 142

On Mon, 19 Oct 2009, Felipe Contreras wrote:

> Many still remain.
>
> Signed-off-by: Felipe Contreras <felipe.contreras RemoveThis @gmail.com>

I have never been in favor of merging whitespace-only patches (in a sense
that the sole purpose of them being to change whitespaces, but no else
value added).

And after today's discussion on kernel summit on this topic, I wouldn't
expect any maintainer to merge it, sorry Smile

--
Jiri Kosina
SUSE Labs, Novell Inc.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo RemoveThis @vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Felipe Contreras · External Since: Oct 07, 2009 Posts: 13

On Mon, Oct 19, 2009 at 5:20 PM, Jiri Kosina <jkosina.DeleteThis@suse.cz> wrote:
> On Mon, 19 Oct 2009, Felipe Contreras wrote:
>
>> Many still remain.
>>
>> Signed-off-by: Felipe Contreras <felipe.contreras.DeleteThis@gmail.com>
>
> I have never been in favor of merging whitespace-only patches (in a sense
> that the sole purpose of them being to change whitespaces, but no else
> value added).

If somebody tries to send a patch for that file that doesn't fix the
white-space, checkpatch will complain, and people will complain that
checkpatch complains, which is precisely what happened, and I was
requested to write this patch by Daniel Walker (final mail wasn't on
the ml):

http://lkml.org/lkml/2009/9/14/183

> And after today's discussion on kernel summit on this topic, I wouldn't
> expect any maintainer to merge it, sorry Smile

What are you talking about?

--
Felipe Contreras
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo.DeleteThis@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Felipe Contreras · External Since: Oct 07, 2009 Posts: 13

On Mon, Oct 19, 2009 at 5:12 PM, Jiri Kosina <jkosina DeleteThis @suse.cz> wrote:
> On Mon, 19 Oct 2009, Felipe Contreras wrote:
>
>> Commit a0d092f introduced the following warning:
>> ipc/msg.c: In function ?msgctl_down?:
>> ipc/msg.c:415: warning: ?msqid64? may be used uninitialized in this function
>>
>> Signed-off-by: Felipe Contreras <felipe.contreras DeleteThis @gmail.com>
>> ---
>> ipc/msg.c | 2 +-
>> 1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/ipc/msg.c b/ipc/msg.c
>> index 2ceab7f..085bd58 100644
>> --- a/ipc/msg.c
>> +++ b/ipc/msg.c
>> @@ -412,7 +412,7 @@ static int msgctl_down(struct ipc_namespace *ns, int msqid, int cmd,
>> struct msqid_ds __user *buf, int version)
>> {
>> struct kern_ipc_perm *ipcp;
>> - struct msqid64_ds msqid64;
>> + struct msqid64_ds uninitialized_var(msqid64);
>> struct msg_queue *msq;
>> int err;
>
> What gcc are you using? I am not getting any warning at least with gcc
> "(SUSE Linux) 4.3.1 20080507 (prerelease) [gcc-4_3-branch revision
> 135036]"

gcc (GCC) 4.4.1 20090725 (Red Hat 4.4.1-2)

Since I moved to Fedora 11 I get more warnings than other people,
possibly because gcc 4.4.

--
Felipe Contreras
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo DeleteThis @vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Jiri Kosina · External Since: Nov 29, 2006 Posts: 142

On Mon, 19 Oct 2009, Felipe Contreras wrote:

> > I have never been in favor of merging whitespace-only patches (in a
> > sense that the sole purpose of them being to change whitespaces, but
> > no else value added).
> If somebody tries to send a patch for that file that doesn't fix the
> white-space, checkpatch will complain, and people will complain that
> checkpatch complains, which is precisely what happened,

Oh, well ... checkpatch warning about this is somewhat controversial. My
preferred way would be that it warns about whitespace only if there are
also some other (non-whitespace) changes.

> and I was requested to write this patch by Daniel Walker (final mail
> wasn't on the ml):
>
> http://lkml.org/lkml/2009/9/14/183

This is something slightly different -- he asks you to fixup whitespace
issue in the code you are newly introducing, right?

> > And after today's discussion on kernel summit on this topic, I wouldn't
> > expect any maintainer to merge it, sorry Smile

> What are you talking about?

Seems like many kernel maintainers are just tired of
'whitespace-cleanup-only' patches that bring no real added value
otherwise.

Thanks,

--
Jiri Kosina
SUSE Labs, Novell Inc.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo RemoveThis @vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Felipe Contreras · External Since: Oct 07, 2009 Posts: 13

On Mon, Oct 19, 2009 at 5:57 PM, Jiri Kosina <jkosina.DeleteThis@suse.cz> wrote:
> On Mon, 19 Oct 2009, Felipe Contreras wrote:
>
>> >> ipc/msg.c: In function ?msgctl_down?:
>> >> ipc/msg.c:415: warning: ?msqid64? may be used uninitialized in this function
>> >>
>> >> Signed-off-by: Felipe Contreras <felipe.contreras.DeleteThis@gmail.com>
>> >> ---
>> >> ipc/msg.c | 2 +-
>> >> 1 files changed, 1 insertions(+), 1 deletions(-)
>> >>
>> >> diff --git a/ipc/msg.c b/ipc/msg.c
>> >> index 2ceab7f..085bd58 100644
>> >> --- a/ipc/msg.c
>> >> +++ b/ipc/msg.c
>> >> @@ -412,7 +412,7 @@ static int msgctl_down(struct ipc_namespace *ns, int msqid, int cmd,
>> >> struct msqid_ds __user *buf, int version)
>> >> {
>> >> struct kern_ipc_perm *ipcp;
>> >> - struct msqid64_ds msqid64;
>> >> + struct msqid64_ds uninitialized_var(msqid64);
>> >> struct msg_queue *msq;
>> >> int err;
>> >
>> > What gcc are you using? I am not getting any warning at least with gcc
>> > "(SUSE Linux) 4.3.1 20080507 (prerelease) [gcc-4_3-branch revision
>> > 135036]"
>>
>> gcc (GCC) 4.4.1 20090725 (Red Hat 4.4.1-2)
>>
>> Since I moved to Fedora 11 I get more warnings than other people,
>> possibly because gcc 4.4.
>
> Wouldn't it be better just to report this to gcc developers as a bug
> instead?

If it's a gcc bug, yes.

> I have verified both with 4.1 and 4.3, and it doesn't emit this
> false-positive warning, so there have been gcc versions getting this
> right. Ergo gcc developers should rather fix this "regression" and revert
> to the old behavior, methinks.

The other possibility is that the bug was in gcc 4.1/4.3, and now 4.4
finds an actual problem in the code. I will try to dig deeper to see
what's actually happening... at first glance I don't see who is
initializing msqid64.

--
Felipe Contreras
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo.DeleteThis@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Felipe Contreras · External Since: Oct 07, 2009 Posts: 13

On Mon, Oct 19, 2009 at 6:03 PM, Jiri Kosina <jkosina DeleteThis @suse.cz> wrote:
> On Mon, 19 Oct 2009, Felipe Contreras wrote:
>
>> > I have never been in favor of merging whitespace-only patches (in a
>> > sense that the sole purpose of them being to change whitespaces, but
>> > no else value added).
>> If somebody tries to send a patch for that file that doesn't fix the
>> white-space, checkpatch will complain, and people will complain that
>> checkpatch complains, which is precisely what happened,
>
> Oh, well ... checkpatch warning about this is somewhat controversial. My
> preferred way would be that it warns about whitespace only if there are
> also some other (non-whitespace) changes.

Huh? I think we are talking about different things. See the next comment.

>> and I was requested to write this patch by Daniel Walker (final mail
>> wasn't on the ml):
>>
>> http://lkml.org/lkml/2009/9/14/183
>
> This is something slightly different -- he asks you to fixup whitespace
> issue in the code you are newly introducing, right?

No, did you read the thread?

This was my patch:
-#define ACPI_MIN(a,b) (((a)<(b))?(a) Sad

b))
-#define ACPI_MAX(a,b) (((a)>(b))?(a) Sad

b))
+#define ACPI_MIN(a,b) min(a, b)
+#define ACPI_MAX(a,b) max(a, b)

Checkpatch complains, even though my changes are ok. So that's what I
mean, if somebody wants to do a similar patch in the future so that
checkpatch doesn't complain; they would have to fix the white-spaces
again.

Or checkpatch should be fixed.

>> > And after today's discussion on kernel summit on this topic, I wouldn't
>> > expect any maintainer to merge it, sorry Smile

>> What are you talking about?
>
> Seems like many kernel maintainers are just tired of
> 'whitespace-cleanup-only' patches that bring no real added value
> otherwise.

Hm, I wonder what would happen to the current badly formatted code.
Stay there forever?

--
Felipe Contreras
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo DeleteThis @vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Felipe Contreras · External Since: Oct 07, 2009 Posts: 13

On Mon, Oct 19, 2009 at 6:29 PM, Jiri Kosina <jkosina DeleteThis @suse.cz> wrote:
> On Mon, 19 Oct 2009, Felipe Contreras wrote:
>
>> >> >> ipc/msg.c: In function ?msgctl_down?:
>> >> >> ipc/msg.c:415: warning: ?msqid64? may be used uninitialized in this function
>> >> >>
>> >> >> Signed-off-by: Felipe Contreras <felipe.contreras DeleteThis @gmail.com>
>> >> >> ---
>> >> >> ipc/msg.c | 2 +-
>> >> >> 1 files changed, 1 insertions(+), 1 deletions(-)
>> >> >>
>> >> >> diff --git a/ipc/msg.c b/ipc/msg.c
>> >> >> index 2ceab7f..085bd58 100644
>> >> >> --- a/ipc/msg.c
>> >> >> +++ b/ipc/msg.c
>> >> >> @@ -412,7 +412,7 @@ static int msgctl_down(struct ipc_namespace *ns, int msqid, int cmd,
>> >> >> struct msqid_ds __user *buf, int version)
>> >> >> {
>> >> >> struct kern_ipc_perm *ipcp;
>> >> >> - struct msqid64_ds msqid64;
>> >> >> + struct msqid64_ds uninitialized_var(msqid64);
>> >> >> struct msg_queue *msq;
>> >> >> int err;
> [ ... snip ... ]
>> > I have verified both with 4.1 and 4.3, and it doesn't emit this
>> > false-positive warning, so there have been gcc versions getting this
>> > right. Ergo gcc developers should rather fix this "regression" and revert
>> > to the old behavior, methinks.
>>
>> The other possibility is that the bug was in gcc 4.1/4.3, and now 4.4
>> finds an actual problem in the code. I will try to dig deeper to see
>> what's actually happening... at first glance I don't see who is
>> initializing msqid64.
>
> This statement of your makes me wonder why you have submitted the patch in
> the first place, as you are apparently not sure whether adding
> uninitialized_var() is a valid thing to do or not.

Because I want to get rid of the warning?

Also, if you look at the commit I mentioned in the commit message
(a0d092f), you'll see this change:
- struct msq_setbuf uninitialized_var(setbuf);
+ struct msq_setbuf setbuf;

Which is ok, because in that version msgctl_down is calling
audit_ipc_set_perm, directly, but only when cmd == IPC_SET. However,
later on (a5f75e7), ipcctl_pre_down was introduced, and the check was
delegated, and that most probably introduced the warning.

In any case, if the patch is not correct, then somebody should point
that out, which is what the patch review process is for. Alternatively
I could have sent an email asking what is happening here, but from my
point of view this patch serves exactly the same purpose, and has the
advantage that it might turn out to be correct.

It's not as if I didn't do any homework while writing this patch.

> The gcc warning in this case is actually bogus, as msqid64 is touched only
> iff cmd == IPC_SET, and in such case, copy_msqid_from_user() initializes
> it properly.

Yes, but it's used by ipcctl_pre_down, which from what I can see is
only using those arguments when cmd == IPC_SET, so everything is ok.
But still, I don't think the compiler _should_ know what
ipcctl_pre_down is going to do, if you look _only_ at msgctl_down,
then uninitialized_var is OK.

Cheers.

--
Felipe Contreras
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo DeleteThis @vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Jiri Kosina · External Since: Nov 29, 2006 Posts: 142

On Mon, 19 Oct 2009, Felipe Contreras wrote:

> > This statement of your makes me wonder why you have submitted the patch in
> > the first place, as you are apparently not sure whether adding
> > uninitialized_var() is a valid thing to do or not.
[ ... ]
> In any case, if the patch is not correct, then somebody should point
> that out, which is what the patch review process is for. Alternatively
> I could have sent an email asking what is happening here, but from my
> point of view this patch serves exactly the same purpose, and has the
> advantage that it might turn out to be correct.
>
> It's not as if I didn't do any homework while writing this patch.

Sorry if it sounded to offensive, I have probably been a little bit too
tired ysterday.

> > The gcc warning in this case is actually bogus, as msqid64 is touched only
> > iff cmd == IPC_SET, and in such case, copy_msqid_from_user() initializes
> > it properly.
>
> Yes, but it's used by ipcctl_pre_down, which from what I can see is
> only using those arguments when cmd == IPC_SET, so everything is ok.
> But still, I don't think the compiler _should_ know what
> ipcctl_pre_down is going to do, if you look _only_ at msgctl_down,
> then uninitialized_var is OK.

Well at least older compilers were able to see this, only 4.4 seems to be
warning here ... I have applied the patch for now, but I am really not
fully convinced about it yet, we should probably report it to gcc folks
anyway.

Thanks,

--
Jiri Kosina
SUSE Labs, Novell Inc.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo.TakeThisOut@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/