fix: tools: kwbimage.c: Initialize headersz to suppress warning

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 <[email protected]>

---
 tools/kwbimage.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

tools/kwbimage.c +0 -0

index c50f2e2..2c302e5 100644

Dear Lukasz Majewski,

Thanks for your patch.

Quoted 2 lines · 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-

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

Quoted 18 lines

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 <[email protected]>

---
 tools/kwbimage.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

tools/kwbimage.c +0 -0

index c50f2e2..2c302e5 100644

tools/kwbimage.c static void kwbimage_set_header(void *ptr, · line 734

Quoted 8 lines

tools/kwbimage.c +1 -1

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

Hi Thomas,

Quoted 9 lines

Dear Lukasz Majewski,

Thanks for your patch.

Quoted 2 lines · 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-

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)

Quoted 60 lines

Quoted 17 lines

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 <[email protected]>

---
  tools/kwbimage.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

tools/kwbimage.c +0 -0

index c50f2e2..2c302e5 100644

tools/kwbimage.c static void kwbimage_set_header(void *ptr, · line 734

Quoted 8 lines

tools/kwbimage.c +1 -1

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

Quoted 1 line

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.

Quoted 5 lines

Quoted 1 line

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

Quoted 7 lines · Jeroen Hofstee

Quoted 5 lines

Quoted 1 line

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.

Hello Albert,

Quoted 6 lines · Albert ARIBAUD

Quoted 3 lines · Jeroen Hofstee

Quoted 3 lines

Quoted 1 line

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

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

Quoted 5 lines

Quoted 4 lines

Quoted 2 lines

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.

Quoted 5 lines

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

Quoted 2 lines

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.

Quoted 8 lines

Quoted 4 lines

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.

Quoted 2 lines

Regards,
Jeroen

Best regards,
Lukasz Majewski

Hello Lukasz,

Quoted 8 lines · Lukasz Majewski

Quoted 2 lines

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

Quoted 11 lines

Quoted 8 lines

Quoted 4 lines

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?

Quoted 6 lines

Quoted 3 lines

Regards,
Jeroen

Best regards,
Lukasz Majewski

Amicalement,

Hi Albert,

Sorry for a late reply.

Quoted 42 lines

Hello Lukasz,

Quoted 8 lines · Lukasz Majewski

Quoted 2 lines

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

Quoted 11 lines

Quoted 8 lines

Quoted 4 lines

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.

Quoted 6 lines

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?

Quoted 8 lines

Quoted 6 lines

Quoted 3 lines

Regards,
Jeroen

Best regards,
Lukasz Majewski

Amicalement,

Best regards,
Lukasz Majewski

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,

Hi Albert,

Sorry for a late reply.

Quoted 37 lines

Hello Lukasz,

Quoted 7 lines · Lukasz Majewski

Quoted 2 lines

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

Quoted 9 lines

Quoted 7 lines

Quoted 4 lines

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.

Quoted 6 lines

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)

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 <[email protected]>

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 <[email protected]>

---
  tools/kwbimage.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

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 <[email protected]>

---
 tools/kwbimage.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

tools/kwbimage.c +1 -1

index c50f2e2..2c302e5 100644
@@ -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;

Signature · 1 line

2.0.0.rc2

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 <[email protected]>

Acked-by Stefan Roese <[email protected]>

Acked-by Heiko Schocher <[email protected]>