| Message ID | 1416558163-23614-1-git-send-email-l.majewski@samsung.com |
|---|---|
| State | Accepted |
| Delegated to: | Tom Rini |
| Headers | show |
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
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
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
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
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
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,
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
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,
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
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,
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@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
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@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
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@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
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, > 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?
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!
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;
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(-)