[U-Boot] [PATCH] fix: tools: kwbimage.c: Initialize headersz to suppress warning

[U-Boot] [PATCH] fix: tools: kwbimage.c: Initialize headersz to suppress warning

[Lukasz Majewski]

When building with my toolchain (4.8.2):
CROSS_COMPILE=/home/lukma/work/ptxdist/toolchains/arm/OSELAS.Toolchain-2013.12.0/arm-v7a-linux-gnueabi/gcc-4.8.2-glibc-2.18-binutils-2.24-kernel-3.12-sanitized/bin/arm-v7a-linux-gnueabi-

I see following WARNING:
tools/kwbimage.c: In function "kwbimage_set_header":
tools/kwbimage.c:803:8: warning: "headersz" may be used uninitialized in this function [-Wmaybe-uninitialized]
  memcpy(ptr, image, headersz);
        ^
This fix aims to suppress it.

Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>
---
 tools/kwbimage.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/kwbimage.c b/tools/kwbimage.c
index c50f2e2..2c302e5 100644
--- a/tools/kwbimage.c
+++ b/tools/kwbimage.c
@@ -728,7 +728,7 @@ static void kwbimage_set_header(void *ptr, struct stat *sbuf, int ifd,
 	FILE *fcfg;
 	void *image = NULL;
 	int version;
-	size_t headersz;
+	size_t headersz = 0;
 	uint32_t checksum;
 	int ret;
 	int size;
-- 
2.0.0.rc2

[U-Boot] [PATCH] fix: tools: kwbimage.c: Initialize headersz to suppress warning

[Thomas Petazzoni]

Dear Lukasz Majewski,

Thanks for your patch.

On Fri, 21 Nov 2014 09:22:43 +0100, Lukasz Majewski wrote:
> When building with my toolchain (4.8.2):
> CROSS_COMPILE=/home/lukma/work/ptxdist/toolchains/arm/OSELAS.Toolchain-2013.12.0/arm-v7a-linux-gnueabi/gcc-4.8.2-glibc-2.18-binutils-2.24-kernel-3.12-sanitized/bin/arm-v7a-linux-gnueabi-

Well, your target toolchain doesn't have much to do about the issue.
tools/kwbimage.c is built for the host.

> I see following WARNING:
> tools/kwbimage.c: In function "kwbimage_set_header":
> tools/kwbimage.c:803:8: warning: "headersz" may be used uninitialized in this function [-Wmaybe-uninitialized]
>   memcpy(ptr, image, headersz);
>         ^
> This fix aims to suppress it.
> 
> Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>
> ---
>  tools/kwbimage.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/kwbimage.c b/tools/kwbimage.c
> index c50f2e2..2c302e5 100644
> --- a/tools/kwbimage.c
> +++ b/tools/kwbimage.c
> @@ -728,7 +728,7 @@ static void kwbimage_set_header(void *ptr, struct stat *sbuf, int ifd,
>  	FILE *fcfg;
>  	void *image = NULL;
>  	int version;
> -	size_t headersz;
> +	size_t headersz = 0;
>  	uint32_t checksum;
>  	int ret;
>  	int size;

Looking briefly again at the code, I believe the warning from gcc is
probably bogus. Here is the code:

        size_t headersz;
	[...]
        version = image_get_version();
        switch (version) {
                /*
                 * Fallback to version 0 if no version is provided in the
                 * cfg file
                 */
        case -1:
        case 0:
                image = image_create_v0(&headersz, params, sbuf->st_size);
                break;

        case 1:
                image = image_create_v1(&headersz, params, sbuf->st_size);
                break;

        default:
                fprintf(stderr, "Unsupported version %d\n", version);
                free(image_cfg);
                exit(EXIT_FAILURE);
        }
	[...]
        /* Finally copy the header into the image area */
        memcpy(ptr, image, headersz);

So the usage of 'headersz' is only done if we have gone through either
the -1/0/1 cases. In the 'default' case, we exit the tool, so the
memcpy() is never reached. Maybe gcc doesn't realize we're getting out
of the function in the default case.

But oh well, if it fixes a warning :-)

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[U-Boot] [PATCH] fix: tools: kwbimage.c: Initialize headersz to suppress warning

[Lukasz Majewski]

Hi Thomas,

> Dear Lukasz Majewski,
> 
> Thanks for your patch.
> 
> On Fri, 21 Nov 2014 09:22:43 +0100, Lukasz Majewski wrote:
> > When building with my toolchain (4.8.2):
> > CROSS_COMPILE=/home/lukma/work/ptxdist/toolchains/arm/OSELAS.Toolchain-2013.12.0/arm-v7a-linux-gnueabi/gcc-4.8.2-glibc-2.18-binutils-2.24-kernel-3.12-sanitized/bin/arm-v7a-linux-gnueabi-
> 
> Well, your target toolchain doesn't have much to do about the issue.
> tools/kwbimage.c is built for the host.

Yes. Correct.

Host: gcc version 4.7.2 (Debian 4.7.2-5)

> 
> > I see following WARNING:
> > tools/kwbimage.c: In function "kwbimage_set_header":
> > tools/kwbimage.c:803:8: warning: "headersz" may be used
> > uninitialized in this function [-Wmaybe-uninitialized] memcpy(ptr,
> > image, headersz); ^
> > This fix aims to suppress it.
> > 
> > Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>
> > ---
> >  tools/kwbimage.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/tools/kwbimage.c b/tools/kwbimage.c
> > index c50f2e2..2c302e5 100644
> > --- a/tools/kwbimage.c
> > +++ b/tools/kwbimage.c
> > @@ -728,7 +728,7 @@ static void kwbimage_set_header(void *ptr,
> > struct stat *sbuf, int ifd, FILE *fcfg;
> >  	void *image = NULL;
> >  	int version;
> > -	size_t headersz;
> > +	size_t headersz = 0;
> >  	uint32_t checksum;
> >  	int ret;
> >  	int size;
> 
> Looking briefly again at the code, I believe the warning from gcc is
> probably bogus. Here is the code:
> 
>         size_t headersz;
> 	[...]
>         version = image_get_version();
>         switch (version) {
>                 /*
>                  * Fallback to version 0 if no version is provided in
> the
>                  * cfg file
>                  */
>         case -1:
>         case 0:
>                 image = image_create_v0(&headersz, params,
> sbuf->st_size); break;
> 
>         case 1:
>                 image = image_create_v1(&headersz, params,
> sbuf->st_size); break;
> 
>         default:
>                 fprintf(stderr, "Unsupported version %d\n", version);
>                 free(image_cfg);
>                 exit(EXIT_FAILURE);
>         }
> 	[...]
>         /* Finally copy the header into the image area */
>         memcpy(ptr, image, headersz);
> 
> So the usage of 'headersz' is only done if we have gone through either
> the -1/0/1 cases. In the 'default' case, we exit the tool, so the
> memcpy() is never reached. Maybe gcc doesn't realize we're getting out
> of the function in the default case.
> 
> But oh well, if it fixes a warning :-)

I didn't claim that there is a bug in the code :-).

I just get annoying when on my continuous integration script I see the
same warning for all cross compiled boards.

Anyway, I assume that you don't have any objections to this patch :-) .

> 
> Best regards,
> 
> Thomas


-- 
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group

[U-Boot] [PATCH] fix: tools: kwbimage.c: Initialize headersz to suppress warning

[Stefan Roese]

On 21.11.2014 09:22, Lukasz Majewski wrote:
> When building with my toolchain (4.8.2):
> CROSS_COMPILE=/home/lukma/work/ptxdist/toolchains/arm/OSELAS.Toolchain-2013.12.0/arm-v7a-linux-gnueabi/gcc-4.8.2-glibc-2.18-binutils-2.24-kernel-3.12-sanitized/bin/arm-v7a-linux-gnueabi-
>
> I see following WARNING:
> tools/kwbimage.c: In function "kwbimage_set_header":
> tools/kwbimage.c:803:8: warning: "headersz" may be used uninitialized in this function [-Wmaybe-uninitialized]
>    memcpy(ptr, image, headersz);
>          ^
> This fix aims to suppress it.
>
> Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>

Yes, lets get rid of this warning. So:

Acked-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

[U-Boot] [PATCH] fix: tools: kwbimage.c: Initialize headersz to suppress warning

[Heiko Schocher]

Hello Lukasz,

Am 21.11.2014 09:22, schrieb Lukasz Majewski:
> When building with my toolchain (4.8.2):
> CROSS_COMPILE=/home/lukma/work/ptxdist/toolchains/arm/OSELAS.Toolchain-2013.12.0/arm-v7a-linux-gnueabi/gcc-4.8.2-glibc-2.18-binutils-2.24-kernel-3.12-sanitized/bin/arm-v7a-linux-gnueabi-
>
> I see following WARNING:
> tools/kwbimage.c: In function "kwbimage_set_header":
> tools/kwbimage.c:803:8: warning: "headersz" may be used uninitialized in this function [-Wmaybe-uninitialized]
>    memcpy(ptr, image, headersz);
>          ^
> This fix aims to suppress it.
>
> Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>
> ---
>   tools/kwbimage.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

I did the same fix locally, but did not found time for posting it, so:

Acked-by: Heiko Schocher <hs@denx.de>

bye,
Heiko
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

[U-Boot] [PATCH] fix: tools: kwbimage.c: Initialize headersz to suppress warning

[Jeroen Hofstee]

Hi,

On 21-11-14 10:20, Lukasz Majewski wrote:
> Hi Thomas,
>
>> Dear Lukasz Majewski,
>>
>> Thanks for your patch.
>>
>> On Fri, 21 Nov 2014 09:22:43 +0100, Lukasz Majewski wrote:
>>> When building with my toolchain (4.8.2):
>>> CROSS_COMPILE=/home/lukma/work/ptxdist/toolchains/arm/OSELAS.Toolchain-2013.12.0/arm-v7a-linux-gnueabi/gcc-4.8.2-glibc-2.18-binutils-2.24-kernel-3.12-sanitized/bin/arm-v7a-linux-gnueabi-
>> Well, your target toolchain doesn't have much to do about the issue.
>> tools/kwbimage.c is built for the host.
> Yes. Correct.
>
> Host: gcc version 4.7.2 (Debian 4.7.2-5)
>
>>> I see following WARNING:
>>> tools/kwbimage.c: In function "kwbimage_set_header":
>>> tools/kwbimage.c:803:8: warning: "headersz" may be used
>>> uninitialized in this function [-Wmaybe-uninitialized] memcpy(ptr,
>>> image, headersz); ^
>>> This fix aims to suppress it.
>>>
>>> Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>
>>> ---
>>>   tools/kwbimage.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/tools/kwbimage.c b/tools/kwbimage.c
>>> index c50f2e2..2c302e5 100644
>>> --- a/tools/kwbimage.c
>>> +++ b/tools/kwbimage.c
>>> @@ -728,7 +728,7 @@ static void kwbimage_set_header(void *ptr,
>>> struct stat *sbuf, int ifd, FILE *fcfg;
>>>   	void *image = NULL;
>>>   	int version;
>>> -	size_t headersz;
>>> +	size_t headersz = 0;
>>>   	uint32_t checksum;
>>>   	int ret;
>>>   	int size;
>> Looking briefly again at the code, I believe the warning from gcc is
>> probably bogus. Here is the code:
>>
>>          size_t headersz;
>> 	[...]
>>          version = image_get_version();
>>          switch (version) {
>>                  /*
>>                   * Fallback to version 0 if no version is provided in
>> the
>>                   * cfg file
>>                   */
>>          case -1:
>>          case 0:
>>                  image = image_create_v0(&headersz, params,
>> sbuf->st_size); break;
>>
>>          case 1:
>>                  image = image_create_v1(&headersz, params,
>> sbuf->st_size); break;
>>
>>          default:
>>                  fprintf(stderr, "Unsupported version %d\n", version);
>>                  free(image_cfg);
>>                  exit(EXIT_FAILURE);
>>          }
>> 	[...]
>>          /* Finally copy the header into the image area */
>>          memcpy(ptr, image, headersz);
>>
>> So the usage of 'headersz' is only done if we have gone through either
>> the -1/0/1 cases. In the 'default' case, we exit the tool, so the
>> memcpy() is never reached. Maybe gcc doesn't realize we're getting out
>> of the function in the default case.
>>
>> But oh well, if it fixes a warning :-)
> I didn't claim that there is a bug in the code :-).
>
> I just get annoying when on my continuous integration script I see the
> same warning for all cross compiled boards.

Wouldn't it be better to simply disable the -Wmaybe-uninitialized for
gcc?

Regards,
Jeroen

[U-Boot] [PATCH] fix: tools: kwbimage.c: Initialize headersz to suppress warning

[Albert ARIBAUD]

Hello Jeroen,

On Fri, 21 Nov 2014 13:34:41 +0100, Jeroen Hofstee
<jeroen@myspectrum.nl> wrote:

> >> But oh well, if it fixes a warning :-)
> > I didn't claim that there is a bug in the code :-).
> >
> > I just get annoying when on my continuous integration script I see the
> > same warning for all cross compiled boards.
> 
> Wouldn't it be better to simply disable the -Wmaybe-uninitialized for
> gcc?

Disabling a warning is hiding potential dust under the carpet IMO, and
the only justification I see as acceptable for doing so is when leaving
the warning enabled would cause an obnoxiously high number of false
positives.

> Regards,
> Jeroen

Amicalement,
-- 
Albert.

[U-Boot] [PATCH] fix: tools: kwbimage.c: Initialize headersz to suppress warning

[Jeroen Hofstee]

Hello Albert,

On 21-11-14 16:30, Albert ARIBAUD wrote:
> On Fri, 21 Nov 2014 13:34:41 +0100, Jeroen Hofstee
> <jeroen@myspectrum.nl> wrote:
>
>>>> But oh well, if it fixes a warning :-)
>>> I didn't claim that there is a bug in the code :-).
>>>
>>> I just get annoying when on my continuous integration script I see the
>>> same warning for all cross compiled boards.
>> Wouldn't it be better to simply disable the -Wmaybe-uninitialized for
>> gcc?
> Disabling a warning is hiding potential dust under the carpet IMO

Agreed in general, but not for this one, since "fixing" is the
carpet, as far a I can tell. This is roughly the case which causes
the warning e.g. (and variant like this with a switch, etc):

----------------------------------------------------------------------------------
char *a;

if (something)
     a = something_valid

[...]

if (something)
    *a = 1;
----------------------------------------------------------------------------------

Some gcc versions start complaining about the second instance,
that it _might_ be used uninitialized.

With the "fix" this will no longer warn:

----------------------------------------------------------------------------------
char *a = 0; /* not valid, just set to stop gcc from complaining */

*a = 1;  // paved away _error_, to suppress an invalid warning..

if (something)
     a = something_valid

....

if (something)
    *a = 1;
----------------------------------------------------------------------------------

Since 0 is a perfectly valid address in u-boot it should emit
no warning whatsoever, just crash at runtime.

> and
> the only justification I see as acceptable for doing so is when leaving
> the warning enabled would cause an obnoxiously high number of false
> positives.

Well let me add, if "fixing the warning" causes real error
to be hidden, we shouldn't "fix" the warnings by modifying
valid code.

Regards,
Jeroen

[U-Boot] [PATCH] fix: tools: kwbimage.c: Initialize headersz to suppress warning

[Albert ARIBAUD]

Hello Lukasz,

On Fri, 21 Nov 2014 09:22:43 +0100, Lukasz Majewski
<l.majewski@samsung.com> wrote:
> When building with my toolchain (4.8.2):
> CROSS_COMPILE=/home/lukma/work/ptxdist/toolchains/arm/OSELAS.Toolchain-2013.12.0/arm-v7a-linux-gnueabi/gcc-4.8.2-glibc-2.18-binutils-2.24-kernel-3.12-sanitized/bin/arm-v7a-linux-gnueabi-
> 
> I see following WARNING:
> tools/kwbimage.c: In function "kwbimage_set_header":
> tools/kwbimage.c:803:8: warning: "headersz" may be used uninitialized in this function [-Wmaybe-uninitialized]
>   memcpy(ptr, image, headersz);
>         ^
> This fix aims to suppress it.
> 
> Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>
> ---
>  tools/kwbimage.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/kwbimage.c b/tools/kwbimage.c
> index c50f2e2..2c302e5 100644
> --- a/tools/kwbimage.c
> +++ b/tools/kwbimage.c
> @@ -728,7 +728,7 @@ static void kwbimage_set_header(void *ptr, struct stat *sbuf, int ifd,
>  	FILE *fcfg;
>  	void *image = NULL;
>  	int version;
> -	size_t headersz;
> +	size_t headersz = 0;
>  	uint32_t checksum;
>  	int ret;
>  	int size;
> -- 
> 2.0.0.rc2

As I was wondering whether there could not be a better way to prevent
the warning, I tried to reproduce the case. I've tried gcc 4.8.3 as well
as 4.9.1 and gcc 4.7.4, and none of them emits the warning above. 

Lukasz, where can I find the toolchain that you are using and which
emits the warning?

Amicalement,
-- 
Albert.

[U-Boot] [PATCH] fix: tools: kwbimage.c: Initialize headersz to suppress warning

[Lukasz Majewski]

Hi Jeroen

> Hello Albert,
> 
> On 21-11-14 16:30, Albert ARIBAUD wrote:
> > On Fri, 21 Nov 2014 13:34:41 +0100, Jeroen Hofstee
> > <jeroen@myspectrum.nl> wrote:
> >
> >>>> But oh well, if it fixes a warning :-)
> >>> I didn't claim that there is a bug in the code :-).
> >>>
> >>> I just get annoying when on my continuous integration script I
> >>> see the same warning for all cross compiled boards.
> >> Wouldn't it be better to simply disable the -Wmaybe-uninitialized
> >> for gcc?
> > Disabling a warning is hiding potential dust under the carpet IMO
> 
> Agreed in general, but not for this one, since "fixing" is the
> carpet, 

I assume that you are presenting below an answer to a "general" case.

However, as Thomas pointed out earlier, this "fix" is perfectly safe
regarding the underlying kwbimage code.

> as far a I can tell. This is roughly the case which causes
> the warning e.g. (and variant like this with a switch, etc):
> 
> ----------------------------------------------------------------------------------
> char *a;
> 
> if (something)
>      a = something_valid
> 
> [...]
> 
> if (something)
>     *a = 1;
> ----------------------------------------------------------------------------------
> 
> Some gcc versions start complaining about the second instance,
> that it _might_ be used uninitialized.
> 
> With the "fix" this will no longer warn:
> 
> ----------------------------------------------------------------------------------
> char *a = 0; /* not valid, just set to stop gcc from complaining */
> 
> *a = 1;  // paved away _error_, to suppress an invalid warning..
> 
> if (something)
>      a = something_valid
> 
> ....
> 
> if (something)
>     *a = 1;
> ----------------------------------------------------------------------------------
> 
> Since 0 is a perfectly valid address in u-boot it should emit
> no warning whatsoever, just crash at runtime.

I got your point.

> 
> > and
> > the only justification I see as acceptable for doing so is when
> > leaving the warning enabled would cause an obnoxiously high number
> > of false positives.
> 
> Well let me add, if "fixing the warning" causes real error
> to be hidden, we shouldn't "fix" the warnings by modifying
> valid code.

Each subsequent "fix" for this kind of warning should be considered
case by case IMHO, therefore I agree with Albert.

> 
> Regards,
> Jeroen

Best regards,
Lukasz Majewski
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20141122/2b42f65d/attachment.pgp>

[U-Boot] [PATCH] fix: tools: kwbimage.c: Initialize headersz to suppress warning

[Albert ARIBAUD]

Hello Lukasz,

On Sat, 22 Nov 2014 07:56:35 +0100, Lukasz Majewski
<l.majewski@majess.pl> wrote:

> > Agreed in general, but not for this one, since "fixing" is the
> > carpet, 
> 
> I assume that you are presenting below an answer to a "general" case.
> 
> However, as Thomas pointed out earlier, this "fix" is perfectly safe
> regarding the underlying kwbimage code.

Jeroen and I (full disclaimer: we have discussed the topic on IRC)
do not contend that the proposed fix would be unsafe; it *is* safe, i.e.
it does not adversely affect the code behavior in any measurable way.

What we contend is that the fix be the /right/ fix (although Jeroen and
I have slightly differing criteria for defining what "the right fix"
would be).

> > > and
> > > the only justification I see as acceptable for doing so is when
> > > leaving the warning enabled would cause an obnoxiously high number
> > > of false positives.
> > 
> > Well let me add, if "fixing the warning" causes real error
> > to be hidden, we shouldn't "fix" the warnings by modifying
> > valid code.
> 
> Each subsequent "fix" for this kind of warning should be considered
> case by case IMHO, therefore I agree with Albert.

Jeroen also agreed on IRC that disabling the compiler warning is not
the right fix either; and I agreed that there had to be a better fix
than pseudo-initializing headersz. I therefore suggested refactoring
kwbimage_set_header in order to ensure gcc does not emit the warning,
but without resorting to non-functional code such as a functionally
meaningless initialization.

Problem is, to refactor the code, one needs a gcc which emits the
warnig. I tried various versions of gcc (4.7.4, 4.8.3, 4.9.1) and all
remained silent when compiling tools/kwbimage.c.

Hence my request: Lukasz, which toolchain are you using exactly? Where
can we download it from?

> > Regards,
> > Jeroen
> 
> Best regards,
> Lukasz Majewski

Amicalement,
-- 
Albert.

[U-Boot] [PATCH] fix: tools: kwbimage.c: Initialize headersz to suppress warning

[Lukasz Majewski]

Hi Albert,

Sorry for a late reply.

> Hello Lukasz,
> 
> On Sat, 22 Nov 2014 07:56:35 +0100, Lukasz Majewski
> <l.majewski@majess.pl> wrote:
> 
> > > Agreed in general, but not for this one, since "fixing" is the
> > > carpet, 
> > 
> > I assume that you are presenting below an answer to a "general"
> > case.
> > 
> > However, as Thomas pointed out earlier, this "fix" is perfectly safe
> > regarding the underlying kwbimage code.
> 
> Jeroen and I (full disclaimer: we have discussed the topic on IRC)
> do not contend that the proposed fix would be unsafe; it *is* safe,
> i.e. it does not adversely affect the code behavior in any measurable
> way.
> 
> What we contend is that the fix be the /right/ fix (although Jeroen
> and I have slightly differing criteria for defining what "the right
> fix" would be).
> 
> > > > and
> > > > the only justification I see as acceptable for doing so is when
> > > > leaving the warning enabled would cause an obnoxiously high
> > > > number of false positives.
> > > 
> > > Well let me add, if "fixing the warning" causes real error
> > > to be hidden, we shouldn't "fix" the warnings by modifying
> > > valid code.
> > 
> > Each subsequent "fix" for this kind of warning should be considered
> > case by case IMHO, therefore I agree with Albert.
> 
> Jeroen also agreed on IRC that disabling the compiler warning is not
> the right fix either; and I agreed that there had to be a better fix
> than pseudo-initializing headersz. I therefore suggested refactoring
> kwbimage_set_header in order to ensure gcc does not emit the warning,
> but without resorting to non-functional code such as a functionally
> meaningless initialization.
> 
> Problem is, to refactor the code, one needs a gcc which emits the
> warnig. I tried various versions of gcc (4.7.4, 4.8.3, 4.9.1) and all
> remained silent when compiling tools/kwbimage.c.
> 
> Hence my request: Lukasz, which toolchain are you using exactly? Where
> can we download it from?

The warning appear on my personal laptop too:

lukma at jawa:~/work/embedded/u-boot-denx (master)$ cc -v
Using built-in specs.
COLLECT_GCC=cc
COLLECT_LTO_WRAPPER=/usr/lib/gcc/x86_64-linux-gnu/4.7/lto-wrapper
Target: x86_64-linux-gnu
Configured with: ../src/configure -v --with-pkgversion='Debian 4.7.2-5'
--with-bugurl=file:///usr/share/doc/gcc-4.7/README.Bugs
--enable-languages=c,c++,go,fortran,objc,obj-c++ --prefix=/usr
--program-suffix=-4.7 --enable-shared --enable-linker-build-id
--with-system-zlib --libexecdir=/usr/lib --without-included-gettext
--enable-threads=posix --with-gxx-include-dir=/usr/include/c++/4.7
--libdir=/usr/lib --enable-nls --with-sysroot=/ --enable-clocale=gnu
--enable-libstdcxx-debug --enable-libstdcxx-time=yes
--enable-gnu-unique-object --enable-plugin --enable-objc-gc
--with-arch-32=i586 --with-tune=generic --enable-checking=release
--build=x86_64-linux-gnu --host=x86_64-linux-gnu
--target=x86_64-linux-gnu Thread model: posix gcc version 4.7.2 (Debian
4.7.2-5) 


  HOSTCC  tools/kwbimage.o
tools/kwbimage.c: In function ?kwbimage_set_header?:
tools/kwbimage.c:803:8: warning: ?headersz? may be used uninitialized
in this function [-Wmaybe-uninitialized]

Debian distro: version 7.6 (Wheezy)


I will check the exact gcc on my office PC tomorrow.


Heiko also complained about this Warning. Heiko could you also share
information about your setup?

> 
> > > Regards,
> > > Jeroen
> > 
> > Best regards,
> > Lukasz Majewski
> 
> Amicalement,

Best regards,
Lukasz Majewski
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20141123/b85da7b9/attachment.pgp>

[U-Boot] [PATCH] fix: tools: kwbimage.c: Initialize headersz to suppress warning

[Lukasz Majewski]

Hi Albert,

> Hi Albert,
> 
> Sorry for a late reply.
> 
> > Hello Lukasz,
> > 
> > On Sat, 22 Nov 2014 07:56:35 +0100, Lukasz Majewski
> > <l.majewski@majess.pl> wrote:
> > 
> > > > Agreed in general, but not for this one, since "fixing" is the
> > > > carpet, 
> > > 
> > > I assume that you are presenting below an answer to a "general"
> > > case.
> > > 
> > > However, as Thomas pointed out earlier, this "fix" is perfectly
> > > safe regarding the underlying kwbimage code.
> > 
> > Jeroen and I (full disclaimer: we have discussed the topic on IRC)
> > do not contend that the proposed fix would be unsafe; it *is* safe,
> > i.e. it does not adversely affect the code behavior in any
> > measurable way.
> > 
> > What we contend is that the fix be the /right/ fix (although Jeroen
> > and I have slightly differing criteria for defining what "the right
> > fix" would be).
> > 
> > > > > and
> > > > > the only justification I see as acceptable for doing so is
> > > > > when leaving the warning enabled would cause an obnoxiously
> > > > > high number of false positives.
> > > > 
> > > > Well let me add, if "fixing the warning" causes real error
> > > > to be hidden, we shouldn't "fix" the warnings by modifying
> > > > valid code.
> > > 
> > > Each subsequent "fix" for this kind of warning should be
> > > considered case by case IMHO, therefore I agree with Albert.
> > 
> > Jeroen also agreed on IRC that disabling the compiler warning is not
> > the right fix either; and I agreed that there had to be a better fix
> > than pseudo-initializing headersz. I therefore suggested refactoring
> > kwbimage_set_header in order to ensure gcc does not emit the
> > warning, but without resorting to non-functional code such as a
> > functionally meaningless initialization.
> > 
> > Problem is, to refactor the code, one needs a gcc which emits the
> > warnig. I tried various versions of gcc (4.7.4, 4.8.3, 4.9.1) and
> > all remained silent when compiling tools/kwbimage.c.
> > 
> > Hence my request: Lukasz, which toolchain are you using exactly?
> > Where can we download it from?
> 
> The warning appear on my personal laptop too:
> 
> lukma at jawa:~/work/embedded/u-boot-denx (master)$ cc -v
> Using built-in specs.
> COLLECT_GCC=cc
> COLLECT_LTO_WRAPPER=/usr/lib/gcc/x86_64-linux-gnu/4.7/lto-wrapper
> Target: x86_64-linux-gnu
> Configured with: ../src/configure -v --with-pkgversion='Debian
> 4.7.2-5' --with-bugurl=file:///usr/share/doc/gcc-4.7/README.Bugs
> --enable-languages=c,c++,go,fortran,objc,obj-c++ --prefix=/usr
> --program-suffix=-4.7 --enable-shared --enable-linker-build-id
> --with-system-zlib --libexecdir=/usr/lib --without-included-gettext
> --enable-threads=posix --with-gxx-include-dir=/usr/include/c++/4.7
> --libdir=/usr/lib --enable-nls --with-sysroot=/ --enable-clocale=gnu
> --enable-libstdcxx-debug --enable-libstdcxx-time=yes
> --enable-gnu-unique-object --enable-plugin --enable-objc-gc
> --with-arch-32=i586 --with-tune=generic --enable-checking=release
> --build=x86_64-linux-gnu --host=x86_64-linux-gnu
> --target=x86_64-linux-gnu Thread model: posix gcc version 4.7.2
> (Debian 4.7.2-5) 
> 
> 
>   HOSTCC  tools/kwbimage.o
> tools/kwbimage.c: In function ?kwbimage_set_header?:
> tools/kwbimage.c:803:8: warning: ?headersz? may be used uninitialized
> in this function [-Wmaybe-uninitialized]
> 
> Debian distro: version 7.6 (Wheezy)
> 
> 
> I will check the exact gcc on my office PC tomorrow.

On my office PC I've got following gcc:

Using built-in specs.
COLLECT_GCC=gcc
COLLECT_LTO_WRAPPER=/usr/lib/gcc/x86_64-linux-gnu/4.7/lto-wrapper
Target: x86_64-linux-gnu
Configured with: ../src/configure -v --with-pkgversion='Debian 4.7.2-5'
--with-bugurl=file:///usr/share/doc/gcc-4.7/README.Bugs
--enable-languages=c,c++,go,fortran,objc,obj-c++ --prefix=/usr
--program-suffix=-4.7 --enable-shared --enable-linker-build-id
--with-system-zlib --libexecdir=/usr/lib --without-included-gettext
--enable-threads=posix --with-gxx-include-dir=/usr/include/c++/4.7
--libdir=/usr/lib --enable-nls --with-sysroot=/ --enable-clocale=gnu
--enable-libstdcxx-debug --enable-libstdcxx-time=yes
--enable-gnu-unique-object --enable-plugin --enable-objc-gc
--with-arch-32=i586 --with-tune=generic --enable-checking=release
--build=x86_64-linux-gnu --host=x86_64-linux-gnu
--target=x86_64-linux-gnu Thread model: posix gcc version 4.7.2 (Debian
4.7.2-5

I've got a stable debian (Wheezy - 7.4)

> 
> 
> Heiko also complained about this Warning. Heiko could you also share
> information about your setup?
> 
> > 
> > > > Regards,
> > > > Jeroen
> > > 
> > > Best regards,
> > > Lukasz Majewski
> > 
> > Amicalement,
> 
> Best regards,
> Lukasz Majewski


-- 
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group

[U-Boot] [PATCH] fix: tools: kwbimage.c: Initialize headersz to suppress warning

[Guillaume Gardet]

Hi,

Le 23/11/2014 18:38, Lukasz Majewski a ?crit :
> Hi Albert,
>
> Sorry for a late reply.
>
>> Hello Lukasz,
>>
>> On Sat, 22 Nov 2014 07:56:35 +0100, Lukasz Majewski
>> <l.majewski@majess.pl> wrote:
>>
>>>> Agreed in general, but not for this one, since "fixing" is the
>>>> carpet,
>>> I assume that you are presenting below an answer to a "general"
>>> case.
>>>
>>> However, as Thomas pointed out earlier, this "fix" is perfectly safe
>>> regarding the underlying kwbimage code.
>> Jeroen and I (full disclaimer: we have discussed the topic on IRC)
>> do not contend that the proposed fix would be unsafe; it *is* safe,
>> i.e. it does not adversely affect the code behavior in any measurable
>> way.
>>
>> What we contend is that the fix be the /right/ fix (although Jeroen
>> and I have slightly differing criteria for defining what "the right
>> fix" would be).
>>
>>>>> and
>>>>> the only justification I see as acceptable for doing so is when
>>>>> leaving the warning enabled would cause an obnoxiously high
>>>>> number of false positives.
>>>> Well let me add, if "fixing the warning" causes real error
>>>> to be hidden, we shouldn't "fix" the warnings by modifying
>>>> valid code.
>>> Each subsequent "fix" for this kind of warning should be considered
>>> case by case IMHO, therefore I agree with Albert.
>> Jeroen also agreed on IRC that disabling the compiler warning is not
>> the right fix either; and I agreed that there had to be a better fix
>> than pseudo-initializing headersz. I therefore suggested refactoring
>> kwbimage_set_header in order to ensure gcc does not emit the warning,
>> but without resorting to non-functional code such as a functionally
>> meaningless initialization.
>>
>> Problem is, to refactor the code, one needs a gcc which emits the
>> warnig. I tried various versions of gcc (4.7.4, 4.8.3, 4.9.1) and all
>> remained silent when compiling tools/kwbimage.c.
>>
>> Hence my request: Lukasz, which toolchain are you using exactly? Where
>> can we download it from?
> The warning appear on my personal laptop too:
>
> lukma at jawa:~/work/embedded/u-boot-denx (master)$ cc -v
> Using built-in specs.
> COLLECT_GCC=cc
> COLLECT_LTO_WRAPPER=/usr/lib/gcc/x86_64-linux-gnu/4.7/lto-wrapper
> Target: x86_64-linux-gnu
> Configured with: ../src/configure -v --with-pkgversion='Debian 4.7.2-5'
> --with-bugurl=file:///usr/share/doc/gcc-4.7/README.Bugs
> --enable-languages=c,c++,go,fortran,objc,obj-c++ --prefix=/usr
> --program-suffix=-4.7 --enable-shared --enable-linker-build-id
> --with-system-zlib --libexecdir=/usr/lib --without-included-gettext
> --enable-threads=posix --with-gxx-include-dir=/usr/include/c++/4.7
> --libdir=/usr/lib --enable-nls --with-sysroot=/ --enable-clocale=gnu
> --enable-libstdcxx-debug --enable-libstdcxx-time=yes
> --enable-gnu-unique-object --enable-plugin --enable-objc-gc
> --with-arch-32=i586 --with-tune=generic --enable-checking=release
> --build=x86_64-linux-gnu --host=x86_64-linux-gnu
> --target=x86_64-linux-gnu Thread model: posix gcc version 4.7.2 (Debian
> 4.7.2-5)
>
>
>    HOSTCC  tools/kwbimage.o
> tools/kwbimage.c: In function ?kwbimage_set_header?:
> tools/kwbimage.c:803:8: warning: ?headersz? may be used uninitialized
> in this function [-Wmaybe-uninitialized]
>
> Debian distro: version 7.6 (Wheezy)

I also have this warning on my openSUSE 13.1 (GCC 4.8.1). If it could help, "gcc -v" returns:
**********************************************************************
Using built-in specs.
COLLECT_GCC=gcc
COLLECT_LTO_WRAPPER=/usr/lib64/gcc/x86_64-suse-linux/4.8/lto-wrapper
Target: x86_64-suse-linux
Configured with: ../configure --prefix=/usr --infodir=/usr/share/info --mandir=/usr/share/man --libdir=/usr/lib64 --libexecdir=/usr/lib64 --enable-languages=c,c++,objc,fortran,obj-c++,java,ada --enable-checking=release --with-gxx-include-dir=/usr/include/c++/4.8 --enable-ssp --disable-libssp --disable-plugin --with-bugurl=http://bugs.opensuse.org/ --with-pkgversion='SUSE Linux' --disable-libgcj --disable-libmudflap --with-slibdir=/lib64 --with-system-zlib --enable-__cxa_atexit --enable-libstdcxx-allocator=new --disable-libstdcxx-pch --enable-version-specific-runtime-libs --enable-linker-build-id --program-suffix=-4.8 --enable-linux-futex --without-system-libunwind --with-arch-32=i586 --with-tune=generic --build=x86_64-suse-linux
Thread model: posix
gcc version 4.8.1 20130909 [gcc-4_8-branch revision 202388] (SUSE Linux)
**********************************************************************



Guillaume

[U-Boot] [PATCH] fix: tools: kwbimage.c: Initialize headersz to suppress warning

[Albert ARIBAUD]

Hello Lukasz,

Thank you and Guillaume for the indications. So gcc 4.7.2 and 4.8.1
should exhibit the problem.

Apparently, 4.7.4 and 4.8.3 don't. :(

/me goes and fetches a 4.8.1 somewhere.

Amicalement,
-- 
Albert.

[U-Boot] [PATCH] fix: tools: kwbimage.c: Initialize headersz to suppress warning

[Lukasz Majewski]

Hi Albert,

> Hello Lukasz,
> 
> Thank you and Guillaume for the indications. So gcc 4.7.2 and 4.8.1
> should exhibit the problem.
> 
> Apparently, 4.7.4 and 4.8.3 don't. :(
> 
> /me goes and fetches a 4.8.1 somewhere.
> 
> Amicalement,

Is there any progress with preparing fix for this warning?

-- 
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group

[U-Boot] fix: tools: kwbimage.c: Initialize headersz to suppress warning

[Tom Rini]

On Fri, Nov 21, 2014 at 09:22:43AM +0100, ?ukasz Majewski wrote:

> When building with my toolchain (4.8.2):
> CROSS_COMPILE=/home/lukma/work/ptxdist/toolchains/arm/OSELAS.Toolchain-2013.12.0/arm-v7a-linux-gnueabi/gcc-4.8.2-glibc-2.18-binutils-2.24-kernel-3.12-sanitized/bin/arm-v7a-linux-gnueabi-
> 
> I see following WARNING:
> tools/kwbimage.c: In function "kwbimage_set_header":
> tools/kwbimage.c:803:8: warning: "headersz" may be used uninitialized in this function [-Wmaybe-uninitialized]
>   memcpy(ptr, image, headersz);
>         ^
> This fix aims to suppress it.
> 
> Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>
> Acked-by: Stefan Roese <sr@denx.de>
> Acked-by: Heiko Schocher <hs@denx.de>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20150110/38e186ab/attachment.pgp>