On Tue, Oct 15, 2013 at 2:33 PM, Andy Gibbs <[email protected]> wrote:
> > Date: Tue, 15 Oct 2013 11:01:34 -0700 > > Subject: Re: r192570 - Fixed "ArgSize may be used uninitialised" error > whencompiling with gcc. > > From: [email protected] > > > > 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
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
