<html>
<head>
<style><!--
.hmmessage P
{
margin:0px;
padding:0px
}
body.hmmessage
{
font-size: 12pt;
font-family:Calibri
}
--></style></head>
<body class='hmmessage'><div dir='ltr'><div>> Date: Tue, 15 Oct 2013 11:01:34 -0700<br>> Subject: Re: r192570 - Fixed "ArgSize may be used uninitialised" error whencompiling with gcc.<br>> From: echristo@gmail.com<br>> <br>> On Tue, Oct 15, 2013 at 9:42 AM, David Blaikie wrote:<br>> ><br>> > On Mon, Oct 14, 2013 at 11:20 PM, Andy Gibbs wrote:<br>> >><br>> >> On Monday, October 14, 2013 6:16 PM, David Blaikie wrote:<br>> >><br>> >>> Generally I believe we would prefer to disable the warning<br>> >>> than needlessly initialize the variable (is this needless<br>> >>> initialization? I haven't looked at the rest of the code,<br>> >>> but I assume it is). That way dynamic tools (MSan, Valgrind,<br>> >>> etc) can properly diagnose when the algorithm has bugs that<br>> >>> fail to initialize this variable.<br>> >><br>> >><br>> >> And on Monday, October 14, 2013 6:33 PM, Robert Lytton wrote:<br>> >><br>> >>> Hi David,<br>> >>><br>> >>> Just to confirm that the initialization was needless.<br>> >>> All enum types are covered in switch statement.<br>> >>> Each case assigns to the variable.<br>> >><br>> >><br>> >> Yes, each case assigns to the variable (even the one with the<br>> >> llvm_unreachable), but gcc seems to be over-zealous with this<br>> >> warning and doesn't consider the switch statement to be fully<br>> >> covered. I think it happens under some specific optimisation<br>> >> level, but I don't know the ins and outs of it, this is just<br>> >> the impression I've gathered over time.<br>> >><br>> >> David, what would be the best best way to disable the warning<br>> >> here? I take your point about using dynamic tools, but the<br>> >> code in its current state will not compile under release mode<br>> >> with -Werror on gcc. I wouldn't want to do some sort of<br>> >> kluge in the order of a diagnostic pragma; would you be happy<br>> >> with an alternative kludge: a LLVM_WEAK_INITIALIZE(...) macro<br>> >> inside llvm/Compiler.h that expands to nothing unless using<br>> >> gcc? I guess there are loads of places across the whole code<br>> >> where this might be used; I don't think this is an isolated<br>> >> case of a technically unnecessary initialisation.<br>> ><br>> ><br>> > You're right, this isn't isolated - the solution is to literally disable the<br>> > warning for the whole project from within our build files. I believe we<br>> > already have some GCC diagnostics disabled, this would just be another to<br>> > add to that list.<br>> ><br>> > Chandler (or many other people) might know more about the build system and<br>> > where this is best handled than I would, my naive guess would be (for the<br>> > Makefile build, at least - cc'ng Eric as the owner of that) in the "Options<br>> > To Invoke Tools" section of Makefile.rules, simply CompileCommonOpts +=<br>> > -Wno-maybe-uninitialized. Clang should have a no-op alias for this warning<br>> > (I haven't checked if it does) and supports more accurate warnings like<br>> > -Wsometimes-uninitialized (which should already be on in our builds) anyway.<br><br>I'm surprised this would be the preferred option since wholesale disabling of a diagnostic means real issues can be missed - a false positive or two is better than a host of missed mistakes. Forgive me if I'm wrong, but tools like msan and valgrind are only useful if we have 100% coverage testing of the code. Given this, I'd personally prefer an unnecessary initialisation (which, after all, doesn't seriously impact performance), or to go the macro route (if such micro-performance issues are important!) where a human makes the decision on a case-by-case basis. But I'm not about to make any last stands over this either way: the vote of the masses is enough for me since I only use gcc for building the bootstrap compiler - it's clang all the way after that.<br><br>> We already have support for -Wno-maybe-initialized in the makefile if<br>> the gcc being used supports it. What version, etc is this? Can you<br>> check if the option works with your gcc? If so, there might be a<br>> problem in the autoconf check for it.<br><br>I'm using cmake, so maybe its not in there; I can check tomorrow. The gcc version is 4.7.2.<br><br>> -eric
</div><div><br></div> </div></body>
</html>