[llvm] r176341 - GCC thinks that this variable might be used uninitialized (it isn't).

[Michael Liao]

On Thu, 2013-03-21 at 23:17 +0100, Arnaud de Grandmaison wrote:
> I understand your point, but do we really need this back-compatibility here 
> --- for the '-W flags' ?
> 
> the add_(llvm_)definitions is for sure convenient because it sets the flags 
> for C and C++ at the same time, whereas the append_if only set one at a time.
> 
> My other concern is that as far as I know, the respective ordering in the 
> compilation command line of definitions and flags depends on the cmake 
> toolchain file : we simply can not rely on that, as it could even change from 
> one revision to the other. Beside, we may want to have cross-compilation one 
> day with cmake. On the other hand, the append_if is quite clear in its intent 
> and does not have this issue.

I don't mean that's the issue. The issue is LLVM specific. Its cmake
prepares command line options through different ways. '-Wall' is added
through 'add_definition', and '-Wno-maybe-uninitialized' is added
through 'CMAKE_CXX_FLAGS'. We should follow a consistent way adding
them.

Yours
- Michael 

> 
> Cheers,
> --
> Arnaud
> 
> On Thursday 21 March 2013 14:55:00 Michael Liao wrote:
> > Cmake doc states add_definition() is OK to add non-macro-definition
> > options for back-compatibility issue. LLVM cmake has lots of such use.
> > 
> > We probably still need to stick on add_llvm_definitions() as it will
> > record the flags used to compile LLVM itself and ship these flags with
> > LLVM release (I guess llvm-config and cmake tools will use them.)
> > However, it's better to revise add_llvm_definitions() to tell different
> > flags, macro definitions, C flags, C++ flags, and etc and populate Cmake
> > variable according to avoid parameter order issue.
> > 
> > - michael
> > 
> > On Thu, 2013-03-21 at 22:08 +0100, Arnaud de Grandmaison wrote:
> > > On Thursday 21 March 2013 18:19:14 Duncan Sands wrote:
> > > > I looked into this a bit more, and the problem occurs when using cmake
> > > > not
> > > > when using configure+make.  The problem is that cmake puts
> > > > -Wno-maybe-uninitialized before -Wall while configure+make has them the
> > > > other way round.  I'm not sure how to fix this, not knowing anything
> > > > about
> > > > cmake.
> > > 
> > > In file "modules/HandleLLVMOptions.cmake", line 189 looks weird to me:
> > > 
> > > add_llvm_definitions( -Wall -W -Wno-unused-parameter -Wwrite-strings )
> > > 
> > > The add_definition (called inside the upper wrapper) is normally used fo
> > > " -DVAR=blabla" things.
> > > 
> > > You would have rather expected the append_if(CMAKE_CXX_FLAGS... or
> > > append_if(CMAKE_C_FLAGS... to be used to setup the warning filters.
> > > 
> > > Cheers,
> > > --
> > > Arnaud
> > > 
> > > > Ciao, Duncan.
> > > > 
> > > > PS: I'm using cmake+ninja.
> > > > 
> > > > On 01/03/13 18:01, Michael Liao wrote:
> > > > > On Fri, 2013-03-01 at 02:50 -0800, Chandler Carruth wrote:
> > > > >> On Fri, Mar 1, 2013 at 1:46 AM, Duncan Sands <baldrick at free.fr> 
> wrote:
> > > > >>          GCC thinks that this variable might be used uninitialized
> > > > >>          (it
> > > > >>          isn't).
> > > > >> 
> > > > >> Sorry to keep kicking the horse here, but I really oppose adding dead
> > > > >> stores to silence a broken warning.
> > > > >> 
> > > > >> 
> > > > >> We'll no longer be able to use Valgrind to catch a bug where the
> > > > >> control flow that intends to initialize this variable suddenly
> > > > >> changes
> > > > >> and fails to do so in one case. =[ That seems really bad.
> > > > >> 
> > > > >> 
> > > > >> Clang's warnings don't have this false positive, and there is even a
> > > > >> way to silence GCCs in modern GCCs (-Wno-maybe-uninitialized) because
> > > > >> of this problem. Can we just turn off old GCC -Wuninitialized and
> > > > >> rely
> > > > >> on the modern compilers' warnings instead?
> > > > > 
> > > > > shall we add that option by default in our makefile*?
> > > > > 
> > > > > - michael
> > > > > 
> > > > >> _______________________________________________
> > > > >> llvm-commits mailing list
> > > > >> llvm-commits at cs.uiuc.edu
> > > > >> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> > > > 
> > > > _______________________________________________
> > > > llvm-commits mailing list
> > > > llvm-commits at cs.uiuc.edu
> > > > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits