r192570 - Fixed "ArgSize may be used uninitialised" error whencompiling with gcc.

[Andy Gibbs]

> Date: Tue, 15 Oct 2013 11:01:34 -0700
> Subject: Re: r192570 - Fixed "ArgSize may be used uninitialised" error whencompiling with gcc.
> From: echristo at gmail.com
> 
> On Tue, Oct 15, 2013 at 9:42 AM, David Blaikie wrote:
> >
> > On Mon, Oct 14, 2013 at 11:20 PM, Andy Gibbs wrote:
> >>
> >> On Monday, October 14, 2013 6:16 PM, David Blaikie wrote:
> >>
> >>> Generally I believe we would prefer to disable the warning
> >>> than needlessly initialize the variable (is this needless
> >>> initialization? I haven't looked at the rest of the code,
> >>> but I assume it is). That way dynamic tools (MSan, Valgrind,
> >>> etc) can properly diagnose when the algorithm has bugs that
> >>> fail to initialize this variable.
> >>
> >>
> >> And on Monday, October 14, 2013 6:33 PM, Robert Lytton wrote:
> >>
> >>> Hi David,
> >>>
> >>> Just to confirm that the initialization was needless.
> >>> All enum types are covered in switch statement.
> >>> Each case assigns to the variable.
> >>
> >>
> >> Yes, each case assigns to the variable (even the one with the
> >> llvm_unreachable), but gcc seems to be over-zealous with this
> >> warning and doesn't consider the switch statement to be fully
> >> covered. I think it happens under some specific optimisation
> >> level, but I don't know the ins and outs of it, this is just
> >> the impression I've gathered over time.
> >>
> >> David, what would be the best best way to disable the warning
> >> here? I take your point about using dynamic tools, but the
> >> code in its current state will not compile under release mode
> >> with -Werror on gcc. I wouldn't want to do some sort of
> >> kluge in the order of a diagnostic pragma; would you be happy
> >> with an alternative kludge: a LLVM_WEAK_INITIALIZE(...) macro
> >> inside llvm/Compiler.h that expands to nothing unless using
> >> gcc? I guess there are loads of places across the whole code
> >> where this might be used; I don't think this is an isolated
> >> case of a technically unnecessary initialisation.
> >
> >
> > You're right, this isn't isolated - the solution is to literally disable the
> > warning for the whole project from within our build files. I believe we
> > already have some GCC diagnostics disabled, this would just be another to
> > add to that list.
> >
> > Chandler (or many other people) might know more about the build system and
> > where this is best handled than I would, my naive guess would be (for the
> > Makefile build, at least - cc'ng Eric as the owner of that) in the "Options
> > To Invoke Tools" section of Makefile.rules, simply CompileCommonOpts +=
> > -Wno-maybe-uninitialized. Clang should have a no-op alias for this warning
> > (I haven't checked if it does) and supports more accurate warnings like
> > -Wsometimes-uninitialized (which should already be on in our builds) anyway.

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.

> We already have support for -Wno-maybe-initialized in the makefile if
> the gcc being used supports it. What version, etc is this? Can you
> check if the option works with your gcc? If so, there might be a
> problem in the autoconf check for it.

I'm using cmake, so maybe its not in there; I can check tomorrow.  The gcc version is 4.7.2.

> -eric

 		 	   		  
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131015/36a46eb9/attachment.html>