> 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 _______________________________________________ llvm-commits mailing list llvm-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits