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

[David Blaikie]

On Tue, Oct 15, 2013 at 2:33 PM, Andy Gibbs <andyg1001 at hotmail.co.uk> wrote:

> > 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.
>

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".


>  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.
>

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.

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