I'm getting a build failure with -Werror:
[off-dbg] : [llvm] /ptmp/dag/llvm/official/llvm/lib/IR/Use.cpp: In member function 'llvm::User* llvm::Use::getUser() const':
[off-dbg] : [llvm] /ptmp/dag/llvm/official/llvm/lib/IR/Use.cpp:142:14: error: cast from type 'const llvm::Use*' to type 'llvm::User*' casts away qualifiers [-Werror=cast-qual]
-David
There's little effort to keep the GCC build warning clean, but I've
seen people cleaning up this particular warning, so feel free to
commit a fix for it (replacing the c-style cast with a const_cast if
it's intended, or just adding "const" to the cast if it wasn't
intended/necessary to remove const).
We should probably setup a GCC -Werror build bot if we are warning
clean for some sufficiently modern version of GCC.
- David
David Blaikie <dblaikie@gmail.com> writes:
I'm getting a build failure with -Werror:
[off-dbg] : [llvm] /ptmp/dag/llvm/official/llvm/lib/IR/Use.cpp: In member function 'llvm::User* llvm::Use::getUser() const':
[off-dbg] : [llvm]
/ptmp/dag/llvm/official/llvm/lib/IR/Use.cpp:142:14: error: cast from
type 'const llvm::Use*' to type 'llvm::User*' casts away qualifiers
[-Werror=cast-qual]There's little effort to keep the GCC build warning clean,
Why? It seems like a pertty important compiler to support.
We should probably setup a GCC -Werror build bot if we are warning
clean for some sufficiently modern version of GCC.
That would be nice. How can I help move such a thing along?
This is with 4.7.1, BTW.
-David
David Blaikie <dblaikie@gmail.com> writes:
I'm getting a build failure with -Werror:
[off-dbg] : [llvm] /ptmp/dag/llvm/official/llvm/lib/IR/Use.cpp: In member function 'llvm::User* llvm::Use::getUser() const':
[off-dbg] : [llvm]
/ptmp/dag/llvm/official/llvm/lib/IR/Use.cpp:142:14: error: cast from
type 'const llvm::Use*' to type 'llvm::User*' casts away qualifiers
[-Werror=cast-qual]There's little effort to keep the GCC build warning clean,
Why? It seems like a pertty important compiler to support.
It has some silly warnings (at least in some versions) that we don't
want to workaround/suppress. Arguably we could have the build system
check version information & suppress certain known-bad warnings on
specific GCC versions.
We should probably setup a GCC -Werror build bot if we are warning
clean for some sufficiently modern version of GCC.That would be nice. How can I help move such a thing along?
Honestly we need to cleanup the buildbot infrastructure in general.
That's a work in progress. David Dean's bringing up the new
phase-based build infrastructure & my hope is that we'll use the
transition to a separate buildmaster running that infrastructure as a
chance to hold a higher bar for builders/slaves and take some time to
really assess where/how we'll allocate our build resources, including
things like building with -Werror, C++11, etc.
This is with 4.7.1, BTW.
& your -Werror build of Clang+LLVM is otherwise clean apart from that?
David Blaikie <dblaikie@gmail.com> writes:
There's little effort to keep the GCC build warning clean,
Why? It seems like a pertty important compiler to support.
It has some silly warnings (at least in some versions) that we don't
want to workaround/suppress. Arguably we could have the build system
check version information & suppress certain known-bad warnings on
specific GCC versions.
That would be ideal, but honestly in my experience there are very few
gcc warnings that should be ignored. Which ones do you think should be
ignored?
Honestly we need to cleanup the buildbot infrastructure in general.
That's a work in progress. David Dean's bringing up the new
phase-based build infrastructure & my hope is that we'll use the
transition to a separate buildmaster running that infrastructure as a
chance to hold a higher bar for builders/slaves and take some time to
really assess where/how we'll allocate our build resources, including
things like building with -Werror, C++11, etc.
What kind of higher bar? I'm interested in contributing resources for a
build slave (particularly for -Werror) but I can't guarantee 100%
uptime. What will be the requirements for a build slave?
This is with 4.7.1, BTW.
& your -Werror build of Clang+LLVM is otherwise clean apart from that?
I'm not sure since I didn't build with -k. I patched the warning and
now it's building. I'll commit the fix if everything works.
-David
David Blaikie <dblaikie@gmail.com> writes:
>>> There's little effort to keep the GCC build warning clean,
>>
>> Why? It seems like a pertty important compiler to support.
>
> It has some silly warnings (at least in some versions) that we don't
> want to workaround/suppress. Arguably we could have the build system
> check version information & suppress certain known-bad warnings on
> specific GCC versions.That would be ideal, but honestly in my experience there are very few
gcc warnings that should be ignored. Which ones do you think should be
ignored?
The implementations of -Wuninitialized, -Wreturn-type, and a few other GCC
warnings are extremely aggressive. They have high false positive rates with
few benefits. We regularly keep -Wreturn-type working despite this (see all
of the llvm_unreachable after switch statements), but the fix to silence
GCC's -Wuninitialized false positives actively degrades the quality of the
code -- it forces dead stores to variables. These dead stores are a waste
and prevent tools like Valgrind from finding very real bugs in the logic
which cause a variable to be left uninitialized.
I'm a big fan of -Werror, but only when the warnings actually make sense.
There are many older compilers (both Clang and GCC) which have false
positives in their warnings which should *not* be fixed because the code is
in fact correct. Even with the latest GCC, we would need to suppress some
warnings due to a fundamentally different approach to when and where the
warning is appropriate.
We have a complete HowTo on the subject 
<http://llvm.org/docs/HowToAddABuilder.html>
-- Sean Silva
Chandler Carruth <chandlerc@google.com> writes:
The implementations of -Wuninitialized, -Wreturn-type, and a few other
GCC warnings are extremely aggressive. They have high false positive
rates with few benefits. We regularly keep -Wreturn-type working
despite this (see all of the llvm_unreachable after switch
statements),
So why not add -Wno-uninitialized and friends to the command line?
It seems a better option than simply ignoring warnings and then missing
a real bug in the haystack of warning messages.
but the fix to silence GCC's -Wuninitialized false positives actively
degrades the quality of the code -- it forces dead stores to
variables. These dead stores are a waste and prevent tools like
Valgrind from finding very real bugs in the logic which cause a
variable to be left uninitialized.
I'm curious about this statement. Can you give an example? 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.
-David
From: llvmdev-bounces@cs.uiuc.edu [mailto:llvmdev-bounces@cs.uiuc.edu]
On Behalf Of greened@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).
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.
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.
- Chuck
THIS COMMUNICATION MAY CONTAIN CONFIDENTIAL AND/OR OTHERWISE PROPRIETARY MATERIAL and is thus for use only by the intended recipient. If you received this in error, please contact the sender and delete the e-mail and its attachments from all computers.
The logic of the program may be such that the actual store (not the "pre-initialization") always happens before any uses, but the compiler may be unable to prove it. In such case, the added initialization will extend the live range of the object and may result in a register spill.
I believe that in the vast majority of cases this is not going to be a problem, and the warning is shown because of the basic nature of the front-end's analysis.
-Krzysztof
"Caldarale, Charles R" <Chuck.Caldarale@unisys.com> writes:
From: llvmdev-bounces@cs.uiuc.edu [mailto:llvmdev-bounces@cs.uiuc.edu]
On Behalf Of greened@obbligato.org
Subject: Re: [LLVMdev] Build FailureIt 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
Sean Silva <silvas@purdue.edu> writes:
"Caldarale, Charles R" <Chuck.Caldarale@unisys.com> writes:
From: llvmdev-bounces@cs.uiuc.edu [mailto:llvmdev-bounces@cs.uiuc.edu]
On Behalf Of greened@obbligato.org
Subject: Re: [LLVMdev] Build FailureIt 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.
Nope. If you want it then sooner is always better than later.
Also keep in mind that some warnings are only enabled with optimization
when building with gcc.
-eric
Krzysztof Parzyszek <kparzysz@codeaurora.org> writes:
I'm curious about this statement. Can you give an example? 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.The logic of the program may be such that the actual store (not the
"pre-initialization") always happens before any uses, but the compiler
may be unable to prove it. In such case, the added initialization
will extend the live range of the object and may result in a register
spill.
If that's really the case, then the variable is declared much too early
and the declaration should be moved.
I believe that in the vast majority of cases this is not going to be a
problem, and the warning is shown because of the basic nature of the
front-end's analysis.
It's a problem if we keep ignoring warnings and we miss real bugs
because there are so many warning messages that things get lost.
I have fixed several dozen warnings now.
-David
Krzysztof Parzyszek <kparzysz@codeaurora.org> writes:
The logic of the program may be such that the actual store (not the
"pre-initialization") always happens before any uses, but the compiler
may be unable to prove it. In such case, the added initialization
will extend the live range of the object and may result in a register
spill.
I also want to note that this is exactly the kind of premature
optimization that Knuth warns about. Is it really worth generating a
warning from the compiler to save 1-2 cycles in initialization code when
we don't even know that the (extremely slight) performance loss matters?
-David
Krzysztof Parzyszek <kparzysz@codeaurora.org> writes:
I'm curious about this statement. Can you give an example? 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.The logic of the program may be such that the actual store (not the
"pre-initialization") always happens before any uses, but the compiler
may be unable to prove it. In such case, the added initialization
will extend the live range of the object and may result in a register
spill.If that's really the case, then the variable is declared much too early
and the declaration should be moved.
Not necessarily. As was pointed out, a simple example of this is when
a variable is used only under one specific condition:
int x;
if (y) {
x = ...
}
...
if (y) {
func(x);
}
where the initial computation must preceed a large block of
unconditional code then be used after that under the same condition it
was computed.
If we initialize x to 0 because the warning tells us to (not because
we need to) then we'll not be able to find bugs where x is used
outside of if(y) when we use tools like Valgrind.
I believe that in the vast majority of cases this is not going to be a
problem, and the warning is shown because of the basic nature of the
front-end's analysis.It's a problem if we keep ignoring warnings and we miss real bugs
because there are so many warning messages that things get lost.
The point is we can catch the bugs with other tools (Valgrind/MemSan)
& keep the compiler focused on bugs it can /reliably/ find without
interfering with other systems. We can do that by implementing all the
valuable warnings in Clang & ensuring we're Clang -Werror clean while
ignoring other compilers warnings that make bad choices about warning
quality.
Krzysztof Parzyszek <kparzysz@codeaurora.org> writes:
The logic of the program may be such that the actual store (not the
"pre-initialization") always happens before any uses, but the compiler
may be unable to prove it. In such case, the added initialization
will extend the live range of the object and may result in a register
spill.I also want to note that this is exactly the kind of premature
optimization that Knuth warns about.
I suspect it isn't. We're not going out of our way to obfuscate the
code or spend time on this - we're just simply not doing work we don't
need to. It's the attempt to suppress the warning that's leading us
astray.
Is it really worth generating a
warning from the compiler to save 1-2 cycles in initialization code when
we don't even know that the (extremely slight) performance loss matters?
No one's suggesting that.
(if the compiler can prove that the write is unnecessary, it'll simply
optimize it away anyway - so such a warning wouldn't make sense, at
best it would be a tiny compile-time improvement, but not a runtime
issue at all (that being said, we do have warnings like -Wweak-vtables
that are a compile time perf issue only))
Worse, it is one of the most optimizer sensitive warnings in GCC.
The combination is what it makes it so problematic.
Joerg
David Blaikie <dblaikie@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