<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Tue, Oct 15, 2013 at 2:33 PM, Andy Gibbs <span dir="ltr"><<a href="mailto:andyg1001@hotmail.co.uk" target="_blank">andyg1001@hotmail.co.uk</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">


<div><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: <a href="mailto:echristo@gmail.com" target="_blank">echristo@gmail.com</a><br>
> <br>> On Tue, Oct 15, 2013 at 9:42 AM, David Blaikie wrote:<div><div class="h5"><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></div></div>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. </div>
</div></div></blockquote><div><br></div><div>This isn't the prevailing attitude of the project, or at least those loud enough to speak up. Clang certainly doesn't adopt this attitude - if we did, we would have implemented GCC's -Wmaybe-uninitialized, but generally it's shouted down as "this has too many false positives, users will just disable it and off-by-default warnings are bad because they aren't well tested".</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div dir="ltr"><div> 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.<div class="im">
<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></div>I'm using cmake, so maybe its not in there; I can check tomorrow.  The gcc version is 4.7.2.</div></div></div></blockquote><div><br></div><div>I imagine Chandler would've addressed this in CMake a while ago, but given a lot of us only selfhost (and don't bother using -Werror in our GCC builds because we treat Clang as the authority on warning cleanliness) I imagine it could've been missed. Personally I can't find the logic even in the configure/Makefile build that disables this warning, so I'm just not reading things very well.<br>
<br>- David </div></div></div></div>