| Message ID | 1370599329-16682-1-git-send-email-xiawenc@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
On Fri, Jun 07, 2013 at 06:02:09PM +0800, Wenchao Xia wrote: > This patch simply remove "variable may be used uninitialized" warning. > > Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> > --- > V2: Address Stefan and Peter's comments, use 0 in send_msg() instead of > initialize mhHeader. > > libcacard/vscclient.c | 3 +-- > util/iov.c | 2 +- > 2 files changed, 2 insertions(+), 3 deletions(-) Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Wenchao Xia <xiawenc@linux.vnet.ibm.com> writes: > This patch simply remove "variable may be used uninitialized" warning. > > Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> > --- > V2: Address Stefan and Peter's comments, use 0 in send_msg() instead of > initialize mhHeader. > > libcacard/vscclient.c | 3 +-- > util/iov.c | 2 +- > 2 files changed, 2 insertions(+), 3 deletions(-) > > diff --git a/libcacard/vscclient.c b/libcacard/vscclient.c > index ac23647..7fbf1da 100644 > --- a/libcacard/vscclient.c > +++ b/libcacard/vscclient.c > @@ -641,7 +641,6 @@ main( > GIOChannel *channel_stdin; > char *qemu_host; > char *qemu_port; > - VSCMsgHeader mhHeader; > > VCardEmulOptions *command_line_options = NULL; > > @@ -750,7 +749,7 @@ main( > .magic = VSCARD_MAGIC, > .capabilities = {0} > }; > - send_msg(VSC_Init, mhHeader.reader_id, &init, sizeof(init)); > + send_msg(VSC_Init, 0, &init, sizeof(init)); > > g_main_loop_run(loop); > g_main_loop_unref(loop); This one's actually a bit more than just a warning suppression. The uninitialized value gets passed to send_msg(), which prints it if verbose > 10. Makes no sense to me. Comes from commit 2ac85b9; cc'ing its author for advice. > diff --git a/util/iov.c b/util/iov.c > index cc6e837..b91cfb9 100644 > --- a/util/iov.c > +++ b/util/iov.c > @@ -146,7 +146,7 @@ ssize_t iov_send_recv(int sockfd, struct iovec *iov, unsigned iov_cnt, > { > ssize_t total = 0; > ssize_t ret; > - size_t orig_len, tail; > + size_t orig_len = 0, tail; > unsigned niov; > > while (bytes > 0) { Here are the uses of orig_len: if (tail) { /* second, fixup the last element, and remember the original * length */ assert(niov < iov_cnt); assert(iov[niov].iov_len > tail); orig_len = iov[niov].iov_len; iov[niov++].iov_len = tail; } ret = do_send_recv(sockfd, iov, niov, do_send); /* Undo the changes above before checking for errors */ if (tail) { iov[niov-1].iov_len = orig_len; } gcc is too stupid to understand the control flow. The initialization shuts it up. Personally, I dislike "shut up" initializations, because when you mistakenly adds a new uninitialized use, you get the arbitrary "shut up" value without warning. Possible alternative: if (tail) { /* second, fixup the last element, and remember the original * length */ assert(niov < iov_cnt); assert(iov[niov].iov_len > tail); orig_len = iov[niov].iov_len; iov[niov++].iov_len = tail; ret = do_send_recv(sockfd, iov, niov, do_send); /* Undo the changes above before checking for errors */ iov[niov-1].iov_len = orig_len; } else { ret = do_send_recv(sockfd, iov, niov, do_send); }
I insist to remove compile warning since I want gcc check my code with strict rule. > Wenchao Xia <xiawenc@linux.vnet.ibm.com> writes: > >> This patch simply remove "variable may be used uninitialized" warning. >> >> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> >> --- >> V2: Address Stefan and Peter's comments, use 0 in send_msg() instead of >> initialize mhHeader. >> >> libcacard/vscclient.c | 3 +-- >> util/iov.c | 2 +- >> 2 files changed, 2 insertions(+), 3 deletions(-) >> > >> diff --git a/util/iov.c b/util/iov.c >> index cc6e837..b91cfb9 100644 >> --- a/util/iov.c >> +++ b/util/iov.c >> @@ -146,7 +146,7 @@ ssize_t iov_send_recv(int sockfd, struct iovec *iov, unsigned iov_cnt, >> { >> ssize_t total = 0; >> ssize_t ret; >> - size_t orig_len, tail; >> + size_t orig_len = 0, tail; >> unsigned niov; >> >> while (bytes > 0) { > > Here are the uses of orig_len: > > if (tail) { > /* second, fixup the last element, and remember the original > * length */ > assert(niov < iov_cnt); > assert(iov[niov].iov_len > tail); > orig_len = iov[niov].iov_len; > iov[niov++].iov_len = tail; > } > > ret = do_send_recv(sockfd, iov, niov, do_send); > > /* Undo the changes above before checking for errors */ > if (tail) { > iov[niov-1].iov_len = orig_len; > } > > > gcc is too stupid to understand the control flow. The initialization > shuts it up. > > Personally, I dislike "shut up" initializations, because when you > mistakenly adds a new uninitialized use, you get the arbitrary "shut up" > value without warning. > > Possible alternative: > > if (tail) { > /* second, fixup the last element, and remember the original > * length */ > assert(niov < iov_cnt); > assert(iov[niov].iov_len > tail); > orig_len = iov[niov].iov_len; > iov[niov++].iov_len = tail; > ret = do_send_recv(sockfd, iov, niov, do_send); > /* Undo the changes above before checking for errors */ > iov[niov-1].iov_len = orig_len; > } else { > ret = do_send_recv(sockfd, iov, niov, do_send); > } > OK to work, but duplicated a line. I think it is not bad to give default value as zero, even there will be new use later.
Il 07/06/2013 14:17, Markus Armbruster ha scritto: >> diff --git a/util/iov.c b/util/iov.c >> index cc6e837..b91cfb9 100644 >> --- a/util/iov.c >> +++ b/util/iov.c >> @@ -146,7 +146,7 @@ ssize_t iov_send_recv(int sockfd, struct iovec *iov, unsigned iov_cnt, >> { >> ssize_t total = 0; >> ssize_t ret; >> - size_t orig_len, tail; >> + size_t orig_len = 0, tail; >> unsigned niov; >> >> while (bytes > 0) { > > Here are the uses of orig_len: > > if (tail) { > /* second, fixup the last element, and remember the original > * length */ > assert(niov < iov_cnt); > assert(iov[niov].iov_len > tail); > orig_len = iov[niov].iov_len; > iov[niov++].iov_len = tail; > } > > ret = do_send_recv(sockfd, iov, niov, do_send); > > /* Undo the changes above before checking for errors */ > if (tail) { > iov[niov-1].iov_len = orig_len; > } > > > gcc is too stupid to understand the control flow. The initialization > shuts it up. Looks like most people's GCC is not that stupid, or I would have broken build for everyone, right? Paolo
Il 08/06/2013 04:04, Wenchao Xia ha scritto: > I insist to remove compile warning since I want gcc check my code with > strict rule. What is your version of GCC? Paolo
于 2013-6-18 18:13, Paolo Bonzini 写道: > Il 07/06/2013 14:17, Markus Armbruster ha scritto: >>> diff --git a/util/iov.c b/util/iov.c >>> index cc6e837..b91cfb9 100644 >>> --- a/util/iov.c >>> +++ b/util/iov.c >>> @@ -146,7 +146,7 @@ ssize_t iov_send_recv(int sockfd, struct iovec *iov, unsigned iov_cnt, >>> { >>> ssize_t total = 0; >>> ssize_t ret; >>> - size_t orig_len, tail; >>> + size_t orig_len = 0, tail; >>> unsigned niov; >>> >>> while (bytes > 0) { >> >> Here are the uses of orig_len: >> >> if (tail) { >> /* second, fixup the last element, and remember the original >> * length */ >> assert(niov < iov_cnt); >> assert(iov[niov].iov_len > tail); >> orig_len = iov[niov].iov_len; >> iov[niov++].iov_len = tail; >> } >> >> ret = do_send_recv(sockfd, iov, niov, do_send); >> >> /* Undo the changes above before checking for errors */ >> if (tail) { >> iov[niov-1].iov_len = orig_len; >> } >> >> >> gcc is too stupid to understand the control flow. The initialization >> shuts it up. > > Looks like most people's GCC is not that stupid, or I would have broken > build for everyone, right? > > Paolo > my gcc version: [xiawenc@RH63Wenchao ~]$ gcc -v Using built-in specs. Target: x86_64-redhat-linux Configured with: ../configure --prefix=/usr --mandir=/usr/share/man --infodir=/usr/share/info --with-bugurl=http://bugzilla.redhat.com/bugzilla --enable-bootstrap --enable-shared --enable-threads=posix --enable-checking=release --with-system-zlib --enable-__cxa_atexit --disable-libunwind-exceptions --enable-gnu-unique-object --enable-languages=c,c++,objc,obj-c++,java,fortran,ada --enable-java-awt=gtk --disable-dssi --with-java-home=/usr/lib/jvm/java-1.5.0-gcj-1.5.0.0/jre --enable-libgcj-multifile --enable-java-maintainer-mode --with-ecj-jar=/usr/share/java/eclipse-ecj.jar --disable-libjava-multilib --with-ppl --with-cloog --with-tune=generic --with-arch_32=i686 --build=x86_64-redhat-linux Thread model: posix gcc version 4.4.6 20120305 (Red Hat 4.4.6-4) (GCC) By default configure, it seems qemu didn't set -Werror to break build.
Il 19/06/2013 08:27, Wenchao Xia ha scritto: >>> gcc is too stupid to understand the control flow. The initialization >>> shuts it up. >> >> Looks like most people's GCC is not that stupid, or I would have broken >> build for everyone, right? > > my gcc version: > [xiawenc@RH63Wenchao ~]$ gcc -v > Using built-in specs. > Target: x86_64-redhat-linux > Configured with: ../configure --prefix=/usr --mandir=/usr/share/man > --infodir=/usr/share/info > --with-bugurl=http://bugzilla.redhat.com/bugzilla --enable-bootstrap > --enable-shared --enable-threads=posix --enable-checking=release > --with-system-zlib --enable-__cxa_atexit --disable-libunwind-exceptions > --enable-gnu-unique-object > --enable-languages=c,c++,objc,obj-c++,java,fortran,ada > --enable-java-awt=gtk --disable-dssi > --with-java-home=/usr/lib/jvm/java-1.5.0-gcj-1.5.0.0/jre > --enable-libgcj-multifile --enable-java-maintainer-mode > --with-ecj-jar=/usr/share/java/eclipse-ecj.jar > --disable-libjava-multilib --with-ppl --with-cloog --with-tune=generic > --with-arch_32=i686 --build=x86_64-redhat-linux > Thread model: posix > gcc version 4.4.6 20120305 (Red Hat 4.4.6-4) (GCC) > > > By default configure, it seems qemu didn't set -Werror to break build. It sets -Werror here: gcc (GCC) 4.7.2 20121109 (Red Hat 4.7.2-8) So configure is doing the right thing for both old and new-enough compilers, I don't think there's a reason to change the code. Paolo
Am 18.06.2013 12:13, schrieb Paolo Bonzini: > Il 07/06/2013 14:17, Markus Armbruster ha scritto: >>> diff --git a/util/iov.c b/util/iov.c >>> index cc6e837..b91cfb9 100644 >>> --- a/util/iov.c >>> +++ b/util/iov.c >>> @@ -146,7 +146,7 @@ ssize_t iov_send_recv(int sockfd, struct iovec *iov, unsigned iov_cnt, >>> { >>> ssize_t total = 0; >>> ssize_t ret; >>> - size_t orig_len, tail; >>> + size_t orig_len = 0, tail; >>> unsigned niov; >>> >>> while (bytes > 0) { >> Here are the uses of orig_len: >> >> if (tail) { >> /* second, fixup the last element, and remember the original >> * length */ >> assert(niov < iov_cnt); >> assert(iov[niov].iov_len > tail); >> orig_len = iov[niov].iov_len; >> iov[niov++].iov_len = tail; >> } >> >> ret = do_send_recv(sockfd, iov, niov, do_send); >> >> /* Undo the changes above before checking for errors */ >> if (tail) { >> iov[niov-1].iov_len = orig_len; >> } >> >> >> gcc is too stupid to understand the control flow. The initialization >> shuts it up. > Looks like most people's GCC is not that stupid, or I would have broken > build for everyone, right? > > Paolo Hi Paolo, I get this warning, too, when I run a normal cross compilation with MinGW-w64: util/iov.c:190:33: warning: ‘orig_len’ may be used uninitialized in this function [-Wuninitialized] My build environment: Debian wheezy with packages gcc-mingw-w64-i686, gcc-mingw-w64-x86-64 (4.6.3-14+8). A complete build results in 5 warnings. Here are the other 4 of them: hw/arm/spitz.c:280:0: warning: "MOD_SHIFT" redefined [enabled by default] hw/ppc/spapr.c:673:26: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast] hw/ppc/spapr_hcall.c:188:1: warning: control reaches end of non-void function [-Wreturn-type] hw/ppc/spapr_pci.c:454:1: warning: control reaches end of non-void function [-Wreturn-type] I already sent a patch for the MOD_SHIFT issue. The remaining 3 warnings are also caused by code which makes it difficult for the compiler to detect that it is correct. Regards Stefan
Il 22/06/2013 12:03, Stefan Weil ha scritto: > Am 18.06.2013 12:13, schrieb Paolo Bonzini: >> Il 07/06/2013 14:17, Markus Armbruster ha scritto: >>>> diff --git a/util/iov.c b/util/iov.c >>>> index cc6e837..b91cfb9 100644 >>>> --- a/util/iov.c >>>> +++ b/util/iov.c >>>> @@ -146,7 +146,7 @@ ssize_t iov_send_recv(int sockfd, struct iovec *iov, unsigned iov_cnt, >>>> { >>>> ssize_t total = 0; >>>> ssize_t ret; >>>> - size_t orig_len, tail; >>>> + size_t orig_len = 0, tail; >>>> unsigned niov; >>>> >>>> while (bytes > 0) { >>> Here are the uses of orig_len: >>> >>> if (tail) { >>> /* second, fixup the last element, and remember the original >>> * length */ >>> assert(niov < iov_cnt); >>> assert(iov[niov].iov_len > tail); >>> orig_len = iov[niov].iov_len; >>> iov[niov++].iov_len = tail; >>> } >>> >>> ret = do_send_recv(sockfd, iov, niov, do_send); >>> >>> /* Undo the changes above before checking for errors */ >>> if (tail) { >>> iov[niov-1].iov_len = orig_len; >>> } >>> >>> > > I get this warning, too, when I run a normal cross compilation with > MinGW-w64: > > util/iov.c:190:33: warning: ‘orig_len’ may be used uninitialized in this > function [-Wuninitialized] > > My build environment: > > Debian wheezy with packages gcc-mingw-w64-i686, gcc-mingw-w64-x86-64 > (4.6.3-14+8). > > A complete build results in 5 warnings. Here are the other 4 of them: > > hw/arm/spitz.c:280:0: warning: "MOD_SHIFT" redefined [enabled by default] > hw/ppc/spapr.c:673:26: warning: cast from pointer to integer of > different size [-Wpointer-to-int-cast] Isn't this one a Win64 bug? > hw/ppc/spapr_hcall.c:188:1: warning: control reaches end of non-void > function [-Wreturn-type] > hw/ppc/spapr_pci.c:454:1: warning: control reaches end of non-void > function [-Wreturn-type] I think you could report this to mingw. GCC should handle "if (!0) foo()" just fine if foo is noreturn, perhaps the "assertion failure" runtime function is not noreturn in mingw. > I already sent a patch for the MOD_SHIFT issue. > > The remaining 3 warnings are also caused by code which makes it difficult > for the compiler to detect that it is correct. Not as hard as this one, though. Anyway, I would be okay a fix that makes the code easier to follow for compilers (and doesn't have too much duplication so that humans are also happy). But adding "= 0" is the worst, because smart compilers will not detect a human's mistakes anymore. Paolo
Am 24.06.2013 16:44, schrieb Paolo Bonzini: > Il 22/06/2013 12:03, Stefan Weil ha scritto: >> I get this warning, too, when I run a normal cross compilation with >> MinGW-w64: >> >> util/iov.c:190:33: warning: ‘orig_len’ may be used uninitialized in this >> function [-Wuninitialized] >> >> My build environment: >> >> Debian wheezy with packages gcc-mingw-w64-i686, gcc-mingw-w64-x86-64 >> (4.6.3-14+8). >> >> A complete build results in 5 warnings. Here are the other 4 of them: >> >> hw/arm/spitz.c:280:0: warning: "MOD_SHIFT" redefined [enabled by default] >> hw/ppc/spapr.c:673:26: warning: cast from pointer to integer of >> different size [-Wpointer-to-int-cast] > Isn't this one a Win64 bug? Yes. http://patchwork.ozlabs.org/patch/252666/ fixes it. >> hw/ppc/spapr_hcall.c:188:1: warning: control reaches end of non-void >> function [-Wreturn-type] >> hw/ppc/spapr_pci.c:454:1: warning: control reaches end of non-void >> function [-Wreturn-type] > I think you could report this to mingw. GCC should handle "if (!0) > foo()" just fine if foo is noreturn, perhaps the "assertion failure" > runtime function is not noreturn in mingw. It's a gcc problem. Removing the assertion manually in the source code and compiling with NDEBUG (which we do by default)results in the same compiler warning. I have sent a small patch series which fixes both warnings and which hopefully matches your criteria for acceptable code changes :-) See http://patchwork.ozlabs.org/patch/253937/ and two more. Regards Stefan
于 2013-6-24 22:44, Paolo Bonzini 写道: > Il 22/06/2013 12:03, Stefan Weil ha scritto: >> Am 18.06.2013 12:13, schrieb Paolo Bonzini: >>> Il 07/06/2013 14:17, Markus Armbruster ha scritto: >>>>> diff --git a/util/iov.c b/util/iov.c >>>>> index cc6e837..b91cfb9 100644 >>>>> --- a/util/iov.c >>>>> +++ b/util/iov.c >>>>> @@ -146,7 +146,7 @@ ssize_t iov_send_recv(int sockfd, struct iovec *iov, unsigned iov_cnt, >>>>> { >>>>> ssize_t total = 0; >>>>> ssize_t ret; >>>>> - size_t orig_len, tail; >>>>> + size_t orig_len = 0, tail; >>>>> unsigned niov; >>>>> >>>>> while (bytes > 0) { >>>> Here are the uses of orig_len: >>>> >>>> if (tail) { >>>> /* second, fixup the last element, and remember the original >>>> * length */ >>>> assert(niov < iov_cnt); >>>> assert(iov[niov].iov_len > tail); >>>> orig_len = iov[niov].iov_len; >>>> iov[niov++].iov_len = tail; >>>> } >>>> >>>> ret = do_send_recv(sockfd, iov, niov, do_send); >>>> >>>> /* Undo the changes above before checking for errors */ >>>> if (tail) { >>>> iov[niov-1].iov_len = orig_len; >>>> } >>>> >>>> >> >> I get this warning, too, when I run a normal cross compilation with >> MinGW-w64: >> >> util/iov.c:190:33: warning: ‘orig_len’ may be used uninitialized in this >> function [-Wuninitialized] >> >> My build environment: >> >> Debian wheezy with packages gcc-mingw-w64-i686, gcc-mingw-w64-x86-64 >> (4.6.3-14+8). >> >> A complete build results in 5 warnings. Here are the other 4 of them: >> >> hw/arm/spitz.c:280:0: warning: "MOD_SHIFT" redefined [enabled by default] >> hw/ppc/spapr.c:673:26: warning: cast from pointer to integer of >> different size [-Wpointer-to-int-cast] > > Isn't this one a Win64 bug? > >> hw/ppc/spapr_hcall.c:188:1: warning: control reaches end of non-void >> function [-Wreturn-type] >> hw/ppc/spapr_pci.c:454:1: warning: control reaches end of non-void >> function [-Wreturn-type] > > I think you could report this to mingw. GCC should handle "if (!0) > foo()" just fine if foo is noreturn, perhaps the "assertion failure" > runtime function is not noreturn in mingw. > >> I already sent a patch for the MOD_SHIFT issue. >> >> The remaining 3 warnings are also caused by code which makes it difficult >> for the compiler to detect that it is correct. > > Not as hard as this one, though. > > Anyway, I would be okay a fix that makes the code easier to follow for > compilers (and doesn't have too much duplication so that humans are also > happy). But adding "= 0" is the worst, because smart compilers will not > detect a human's mistakes anymore. > > Paolo > Hi Palo, There is V3 remove uninitlized warning without adding "= 0", could u take a look for it? http://lists.nongnu.org/archive/html/qemu-devel/2013-06/msg02348.html
Il 24/06/2013 19:58, Stefan Weil ha scritto: >>> hw/ppc/spapr_hcall.c:188:1: warning: control reaches end of non-void >>> function [-Wreturn-type] >>> hw/ppc/spapr_pci.c:454:1: warning: control reaches end of non-void >>> function [-Wreturn-type] >> I think you could report this to mingw. GCC should handle "if (!0) >> foo()" just fine if foo is noreturn, perhaps the "assertion failure" >> runtime function is not noreturn in mingw. > > It's a gcc problem. Removing the assertion manually in the source > code and compiling with NDEBUG (which we do by default)results > in the same compiler warning. Huh? That seems wrong, assertions are there for a reason... Paolo
diff --git a/libcacard/vscclient.c b/libcacard/vscclient.c index ac23647..7fbf1da 100644 --- a/libcacard/vscclient.c +++ b/libcacard/vscclient.c @@ -641,7 +641,6 @@ main( GIOChannel *channel_stdin; char *qemu_host; char *qemu_port; - VSCMsgHeader mhHeader; VCardEmulOptions *command_line_options = NULL; @@ -750,7 +749,7 @@ main( .magic = VSCARD_MAGIC, .capabilities = {0} }; - send_msg(VSC_Init, mhHeader.reader_id, &init, sizeof(init)); + send_msg(VSC_Init, 0, &init, sizeof(init)); g_main_loop_run(loop); g_main_loop_unref(loop); diff --git a/util/iov.c b/util/iov.c index cc6e837..b91cfb9 100644 --- a/util/iov.c +++ b/util/iov.c @@ -146,7 +146,7 @@ ssize_t iov_send_recv(int sockfd, struct iovec *iov, unsigned iov_cnt, { ssize_t total = 0; ssize_t ret; - size_t orig_len, tail; + size_t orig_len = 0, tail; unsigned niov; while (bytes > 0) {
This patch simply remove "variable may be used uninitialized" warning. Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> --- V2: Address Stefan and Peter's comments, use 0 in send_msg() instead of initialize mhHeader. libcacard/vscclient.c | 3 +-- util/iov.c | 2 +- 2 files changed, 2 insertions(+), 3 deletions(-)