"Caldarale, Charles R" <Chuck.Caldarale at unisys.com> writes:>> From: llvmdev-bounces at cs.uiuc.edu [mailto:llvmdev-bounces at cs.uiuc.edu] >> On Behalf Of greened at obbligato.org >> Subject: Re: [LLVMdev] Build Failure > >> It seems a better option than simply ignoring warnings and then missing >> a real bug in the haystack of warning messages. > > Definitely agree with that. Our project coding standards require > _zero_ warnings at commit time (but that only applies to our code, not > imported libraries).So how did these slip in?>> I've committed fixes to lots of -Wuninitialized warnings in my tree. >> It's all just initializing local variables, which shouldn't result in >> extra stores. > > What do you think the initialization is? Something has to write the > initial value, and that write is frequently pointless.It's most likely a store to a register. That's hardly a performance issue. Even a store to the stack has little effect. Really, we're going to ignore errors because we can't afford one store in initialization code?> The real problem with this specific warning is that gcc doesn't > properly track that a variable is used only under the same conditions > in which it is set.That is not always true in the cases I've found. That's the consequence of ignoring warnings. -David
On Thu, Jan 3, 2013 at 8:22 AM, <dag at cray.com> wrote:> "Caldarale, Charles R" <Chuck.Caldarale at unisys.com> writes: > >>> From: llvmdev-bounces at cs.uiuc.edu [mailto:llvmdev-bounces at cs.uiuc.edu] >>> On Behalf Of greened at obbligato.org >>> Subject: Re: [LLVMdev] Build Failure >> >>> It seems a better option than simply ignoring warnings and then missing >>> a real bug in the haystack of warning messages. >> >> Definitely agree with that. Our project coding standards require >> _zero_ warnings at commit time (but that only applies to our code, not >> imported libraries). > > So how did these slip in?I assume from Charles's perspective "our project" is not LLVM but whatever he works on most of the time (& "imported libraries" is LLVM).>>> I've committed fixes to lots of -Wuninitialized warnings in my tree. >>> It's all just initializing local variables, which shouldn't result in >>> extra stores. >> >> What do you think the initialization is? Something has to write the >> initial value, and that write is frequently pointless. > > It's most likely a store to a register. That's hardly a performance > issue. Even a store to the stack has little effect. > > Really, we're going to ignore errors because we can't afford one store > in initialization code?Essentially we don't want to pessimize code just because of a bad warning. The other point Chandler was making, though, was that this sort of fix means that other tools (Valgrind/MemSan) won't catch a broader class of errors because we will have silenced them too.> >> The real problem with this specific warning is that gcc doesn't >> properly track that a variable is used only under the same conditions >> in which it is set. > > That is not always true in the cases I've found. That's the consequence > of ignoring warnings.Except it isn't. We can ignore this warning & instead use Valgrind-esque tools to catch not only these bugs, but catch & fix them better by learning which specific codepath leads to the uninitialized use, rather than just initializing a variable to zero (or whatever) even in cases where that value was never intended to be used in any computation.
David Blaikie <dblaikie at gmail.com> writes:> The other point Chandler was making, though, was that this sort of fix > means that other tools (Valgrind/MemSan) won't catch a broader class > of errors because we will have silenced them too.Does buildbot do a valgrind build with the same frequency as any other build? If so, then either we should fix the warnings or we should disable them in the Makefile.> Except it isn't. We can ignore this warning & instead use > Valgrind-esque tools to catch not only these bugs, but catch & fix > them better by learning which specific codepath leads to the > uninitialized use, rather than just initializing a variable to zero > (or whatever) even in cases where that value was never intended to be > used in any computation.So what am I to do? We build with -Werror. I am really opposed to having to sift through hundreds of warning messages to pick out actual compiler errors. How about initializing things to a garbage value and then doing an assert after all the following conditional initialization code runs to check for the garbage value? Then even non-valgrind builds will catch the problem. -David
David Blaikie <dblaikie at gmail.com> writes:>> That is not always true in the cases I've found. That's the consequence >> of ignoring warnings. > > Except it isn't. We can ignore this warning & instead use > Valgrind-esque tools to catch not only these bugs, but catch & fix > them better by learning which specific codepath leads to the > uninitialized use, rather than just initializing a variable to zero > (or whatever) even in cases where that value was never intended to be > used in any computation.Another option is to use compiler pragmas to disable warnings when we know they are false positives. Would that be acceptable to add to llvm/clang? That seems the best of all worlds. The pragma serves to document a special case we think is ok while still allowing valgrind to catch bugs. I can easily rework my patches to use pragmas. -David