diff mbox

Partial inlining

Message ID 20100626103102.GB7562@kam.mff.cuni.cz
State New
Headers show

Commit Message

Jan Hubicka June 26, 2010, 10:31 a.m. UTC
> > > On 06/26/2010 11:16 AM, Jan Hubicka wrote:
> > > >>> You seem to be consistently on less dumping by default than me.  I find dumping
> > > >>> of reasons why split points are not accepted quite useful, but I guess using
> > > >>> -details is not problem.
> > > >>>
> > > >>>       
> > > >> This breaks C++:
> > > >>
> > > >> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=44671
> > > >>     
> > > > This does not reproduce for me (at least with "Fix PHI handling in ipa-split" patch my tree)
> > > > I am now re-trying from scratch.
> > > >   
> > > The problem is definitely there. See, for example:
> > > 
> > >     http://gcc.gnu.org/ml/gcc-testresults/2010-06/msg02660.html
> > > 
> > > and I have just reproduced it myself once more with r161427.
> > 
> > It is interesting indeed, I get
> >                 === libstdc++ Summary ===
> > 
> > # of expected passes            7160
> > # of expected failures          61
> > # of unsupported tests          341
> > 
> > on build on gcc14 compilation farm.  On gcc17 the problem however reproduce and goes away when the
> > testsuite_shard is comiled with -fno-partial-inlining.
> 
> The difference is use of GOLD as linker.  I am investigating if we are hitting GNU LD bug or if GCC incorrectly
> emits direct calls to the function in question.

So it is GOLD's problem, it misses diagnostics here on PCrel relocations
pointing outside of DSO.  Ian, perhaps GOLD should be told about this?

I am testing the following fix.  The problem is that cgraph_function_versioning
calls cgraph_make_decl_local twice while doing it just once would suffice.
First time it calls the DECL on the old assembler name that makes
cgraph_make_decl_local to take away the WEAK flag from RTL symbol of the
original function.  This leads us to output the incorrect direct pointers.

I am bootstrapping/regtesting the following patch and will commit it once it
passes.

My apologizes for the problem.
Honza

	PR middle-end/44671
	* cgraphunit.c (cgraph_function_versioning): Remove wrong cgraph_make_decl_local
	call.

Comments

H.J. Lu June 26, 2010, 4:45 p.m. UTC | #1
On Sat, Jun 26, 2010 at 3:31 AM, Jan Hubicka <hubicka@ucw.cz> wrote:
>> > > On 06/26/2010 11:16 AM, Jan Hubicka wrote:
>> > > >>> You seem to be consistently on less dumping by default than me.  I find dumping
>> > > >>> of reasons why split points are not accepted quite useful, but I guess using
>> > > >>> -details is not problem.
>> > > >>>
>> > > >>>
>> > > >> This breaks C++:
>> > > >>
>> > > >> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=44671
>> > > >>
>> > > > This does not reproduce for me (at least with "Fix PHI handling in ipa-split" patch my tree)
>> > > > I am now re-trying from scratch.
>> > > >
>> > > The problem is definitely there. See, for example:
>> > >
>> > >     http://gcc.gnu.org/ml/gcc-testresults/2010-06/msg02660.html
>> > >
>> > > and I have just reproduced it myself once more with r161427.
>> >
>> > It is interesting indeed, I get
>> >                 === libstdc++ Summary ===
>> >
>> > # of expected passes            7160
>> > # of expected failures          61
>> > # of unsupported tests          341
>> >
>> > on build on gcc14 compilation farm.  On gcc17 the problem however reproduce and goes away when the
>> > testsuite_shard is comiled with -fno-partial-inlining.
>>
>> The difference is use of GOLD as linker.  I am investigating if we are hitting GNU LD bug or if GCC incorrectly
>> emits direct calls to the function in question.
>
> So it is GOLD's problem, it misses diagnostics here on PCrel relocations
> pointing outside of DSO.  Ian, perhaps GOLD should be told about this?
>
> I am testing the following fix.  The problem is that cgraph_function_versioning
> calls cgraph_make_decl_local twice while doing it just once would suffice.
> First time it calls the DECL on the old assembler name that makes
> cgraph_make_decl_local to take away the WEAK flag from RTL symbol of the
> original function.  This leads us to output the incorrect direct pointers.
>
> I am bootstrapping/regtesting the following patch and will commit it once it
> passes.
>
> My apologizes for the problem.
> Honza
>
>        PR middle-end/44671
>        * cgraphunit.c (cgraph_function_versioning): Remove wrong cgraph_make_decl_local
>        call.
> Index: cgraphunit.c
> ===================================================================
> --- cgraphunit.c        (revision 161427)
> +++ cgraphunit.c        (working copy)
> @@ -2196,7 +2196,6 @@ cgraph_function_versioning (struct cgrap
>   else
>     new_decl = build_function_decl_skip_args (old_decl, args_to_skip);
>
> -  cgraph_make_decl_local (new_decl);
>   /* Generate a new name for the new version. */
>   DECL_NAME (new_decl) = clone_function_name (old_decl, clone_name);
>   SET_DECL_ASSEMBLER_NAME (new_decl, DECL_NAME (new_decl));
>

I still see many libstdc++ run-time failures on Linux/ia32 even with this fix:

http://gcc.gnu.org/ml/gcc-testresults/2010-06/msg02683.html

GOLD isn't the default linker and gcc is configured with

--enable-clocale=gnu --with-system-zlib --enable-shared
--with-demangler-in-ld -with-plugin-ld=ld.gold --enable-gold
--with-fpmath=sse
Jan Hubicka June 27, 2010, 6:18 p.m. UTC | #2
> I still see many libstdc++ run-time failures on Linux/ia32 even with this fix:
> 
> http://gcc.gnu.org/ml/gcc-testresults/2010-06/msg02683.html
> 
> GOLD isn't the default linker and gcc is configured with
> 
> --enable-clocale=gnu --with-system-zlib --enable-shared
> --with-demangler-in-ld -with-plugin-ld=ld.gold --enable-gold
> --with-fpmath=sse

Curiosly I get a lot of excess errors that seem unrealated to the problem.  A
lot of the testcases you mention just pass for me.

I've reproduced 4 execution failures that go away with -fno-partial-inlining.
I think it is latent bug in clonning WRT builtin implementations in glibc
headers.  I am just testing patch for that.

Honza
diff mbox

Patch

Index: cgraphunit.c
===================================================================
--- cgraphunit.c	(revision 161427)
+++ cgraphunit.c	(working copy)
@@ -2196,7 +2196,6 @@  cgraph_function_versioning (struct cgrap
   else
     new_decl = build_function_decl_skip_args (old_decl, args_to_skip);
 
-  cgraph_make_decl_local (new_decl);
   /* Generate a new name for the new version. */
   DECL_NAME (new_decl) = clone_function_name (old_decl, clone_name);
   SET_DECL_ASSEMBLER_NAME (new_decl, DECL_NAME (new_decl));