[llvm-commits] llvm-gcc4: gimplifier fixes

[Chris Lattner]

> Hi Devang, you are right that this effects C++ also: in rare cases C++
> produces expressions of type address_of_static_constructor.  This  
> currently
> causes llvm-gcc4 to abort, at least with the testcase below.  With  
> my patch
> it no longer aborts, so the danger is that wrong code may be  
> produced.  Here
> is the testcase from gcc PR c++/23171:
>
> p.c:
> int *p = (int*)(int[1]){0};
>
> Without my patch:
>
> $ g++ -c -O p.c
> llvm-backend.cpp:491: void emit_global_to_llvm(tree_node*):  
> Assertion `((__extension__ ({ const tree __t = ((__extension__  
> ({ const tree __t = (decl); if (tree_code_type[(int) (((enum  
> tree_code) (__t)

Yuck.

> With my patch this produces the following LLVM:
>
> ; ModuleID = 'p.c'
> target datalayout = "e-p:32:32"
> target endian = little
> target pointersize = 32
> target triple = "i686-pc-linux-gnu"
> %p = global i32* null		; <i32**> [#uses=0]
> %llvm.global_ctors = appending global [0 x { i32, void ()* }]  
> zeroinitializer		; <[0 x { i32, void ()* }]*> [#uses=0]
>
> implementation   ; Functions:
>
> I think this is wrong, because the initializer is not static as it  
> should be.
> This was a problem with the original gcc patch too (svn revision  
> 109035; equivalent
> to the patch I sent), leading to a subsequent much larger patch  
> (svn revision 109075).

This is definitely wrong.  I get this machine code with native GCC  
for this test:

         .globl _p
         .data
_p:
         .long   LC0

         .const
LC0:
         .long   0


This basically corresponds to LLVM like this:

%tmp = constant i32 0
%p = global i32* %tmp

> So if you consider this bug worth fixing in C++, then both patches  
> need to be backported
> (only the patch I sent is needed for Ada, see comments later).

Yeah, it definitely is worth fixing :)

> To put some perspective on this, I put a noisy abort statement in  
> this code path and
> compiled as much C++ as I had easily available: I bootstrapped the  
> compiler, built LLVM,
> ran the testsuite, and built the boost libraries.  The abort was  
> never triggered, so it
> seems that this kind of code is rare in C++.

Ok, but while rare, it still can happen, thus it's still a bug :(

> However this construct is often produced by the Ada front-end.  In  
> fact it triggered the
> LLVM assert in the very first Ada file compiled when  
> bootstrapping!  So this patch is
> needed if you want to get anywhere with Ada.  Historically,  
> additional changes were needed
> for the Ada front-end too, however I am working with a backport of  
> the Ada front-end from
> gcc 4.3, so those fixes are already in there.  (I'm not using the  
> 4.0 Ada f-e because it
> was notoriously weak, producing all kinds of bogus trees).

Right.

> My preference is to backport the complete patch set.  I have some  
> questions about this
> though:
> (1) where should the testcase go?  Presumably in the llvm copy of  
> the testsuite;

llvm/test/C++Frontend

you can add a new llvm/test/AdaFrontend as well eventually.

> (2) should the gcc changelog entries be backported too?

This is up to Devang :)

> (3) how to test?  I have no C++ code that exercises these code  
> paths except for the
> testcase above.  I have plenty of Ada but that's useless for the  
> moment since Ada
> doesn't build.  I propose that I check that the testcase produces  
> appropriate LLVM,
> and check that the changes don't produce any new build failures in  
> the collection of
> C++ I build before.

Testing the code with the testcase above and Ada tests seem fine.

-Chris