Patchwork HPPA constructor merge patch, PR middle-end/45388

login
register
mail settings
Submitter Steve Ellcey
Date Sept. 27, 2010, 8:29 p.m.
Message ID <201009272029.o8RKTDM18735@lucas.cup.hp.com>
Download mbox | patch
Permalink /patch/65919/
State New
Headers show

Comments

Steve Ellcey - Sept. 27, 2010, 8:29 p.m.
A lot of C++ tests started failing on HPPA with r163443, the constructor
merge patch.  HPPA (in 32 bit mode) does not have .ctors and .dtors
sections so it uses the collect2 magic name functionality to handle
static constructors.

In ipa.c at build_cdtor_fns the code says:

/* Generate functions to call static constructors and destructors
   for targets that do not support .ctors/.dtors sections.  These
   functions have magic names which are detected by collect2.  */

The problem is that while build_cdtor turns off DECL_STATIC_CONSTRUCTOR
(and DECL_STATIC_DESTRUCTOR) it does net set TREE_PUBLIC to 1 for these
functions and it is TREE_PUBLIC that is used in
ASM_DECLARE_FUNCTION_NAME to decide whether or not to make the
constructor or destructor name externally visible.  If it is not
externally visible then collect2 will not find the names in order to
set up the needed constructors.

Tested on hppa2.0w-hp-hpux11.11 (32 bit mode) with no regressions.
OK to checkin?

Steve Ellcey
sje@cup.hp.com


2010-09-27  Steve Ellcey  <sje@cup.hp.com>

	PR middle-end/45388
	* ipa.c: Set TREE_PUBLIC on constructors/destructors.
Richard Henderson - Sept. 27, 2010, 8:33 p.m.
On 09/27/2010 01:29 PM, Steve Ellcey wrote:
> 	PR middle-end/45388
> 	* ipa.c: Set TREE_PUBLIC on constructors/destructors.

Not ok.  Constructors should not be public when we
do have .ctor/.dtor support.

This is supposed to be handled in cgraph_build_static_cdtor,

  if (!targetm.have_ctors_dtors)
    {
      TREE_PUBLIC (decl) = 1;
      DECL_PRESERVE_P (decl) = 1;
    }

there.  Can you figure out if that bit isn't being triggered?


r~
Steve Ellcey - Sept. 27, 2010, 9:05 p.m.
On Mon, 2010-09-27 at 13:33 -0700, Richard Henderson wrote:
> On 09/27/2010 01:29 PM, Steve Ellcey wrote:
> > 	PR middle-end/45388
> > 	* ipa.c: Set TREE_PUBLIC on constructors/destructors.
> 
> Not ok.  Constructors should not be public when we
> do have .ctor/.dtor support.
> 
> This is supposed to be handled in cgraph_build_static_cdtor,
> 
>   if (!targetm.have_ctors_dtors)
>     {
>       TREE_PUBLIC (decl) = 1;
>       DECL_PRESERVE_P (decl) = 1;
>     }
> 
> there.  Can you figure out if that bit isn't being triggered?
> 
> 
> r~

It is being triggered, but for a different routine.  In build_cdtor we
are setting TREE_PUBLIC on _GLOBAL__I__ZN2c12f6Ev.  In
cgraph_build_static_cdtor we are setting TREE_PUBLIC on
_GLOBAL__I_65535_0__ZN2c12f6Ev.  This is for the test case
g++.dg/abi/covariant3.C.

When I look at the code, I see that _GLOBAL__I__ZN2c12f6Ev is never
(directly) called by anything but it calls
_Z41__static_initialization_and_destruction_0ii, which
_GLOBAL__I_65535_0__ZN2c12f6Ev also calls.  Is _GLOBAL__I__ZN2c12f6Ev
the old (premerged) constructor that should no longer be used and has
been replaced by GLOBAL__I_65535_0__ZN2c12f6Ev?  In this case we
wouldn't want it global and collect2 should not see or call it, but
because the HP nm/linker/whatever HP-UX collect2 uses to find global
names sees it, we still try to call it even when we shouldn't.  But if
it has essentially been replaced, why wasn't it just removed entirely?

Steve Ellcey
sje@cup.hp.com
Richard Henderson - Sept. 27, 2010, 9:23 p.m.
On 09/27/2010 02:05 PM, Steve Ellcey wrote:
> When I look at the code, I see that _GLOBAL__I__ZN2c12f6Ev is never
> (directly) called by anything but it calls
> _Z41__static_initialization_and_destruction_0ii, which
> _GLOBAL__I_65535_0__ZN2c12f6Ev also calls.  Is _GLOBAL__I__ZN2c12f6Ev
> the old (premerged) constructor that should no longer be used and has
> been replaced by GLOBAL__I_65535_0__ZN2c12f6Ev?

Yes.  I believe that the expected full sequence is that 

  GLOBAL__I_65535_0__ZN2c12f6Ev calls
    GLOBAL__I__ZN2c12f6Ev calls
      _Z41__static_initialization_and_destruction_0ii

and that GLOBAL__I__ZN2c12f6Ev got inlined.

> But if
> it has essentially been replaced, why wasn't it just removed entirely?

An excellent question.  I wonder if it's still marked
as preserved or something?

Of course regardless it seems like we're absolutely
relying on that middle call being inlined and eliminated.
I can think of two solutions: adjust collect2 to ignore
local symbols, or rename the middle function so that it
no longer matches the collect2 pattern.  Both solutions
seem slightly unpleasent, actually.

Thoughts, Jan?


r~
Jan Hubicka - Sept. 28, 2010, 12:01 p.m.
> On 09/27/2010 02:05 PM, Steve Ellcey wrote:
> > When I look at the code, I see that _GLOBAL__I__ZN2c12f6Ev is never
> > (directly) called by anything but it calls
> > _Z41__static_initialization_and_destruction_0ii, which
> > _GLOBAL__I_65535_0__ZN2c12f6Ev also calls.  Is _GLOBAL__I__ZN2c12f6Ev
> > the old (premerged) constructor that should no longer be used and has
> > been replaced by GLOBAL__I_65535_0__ZN2c12f6Ev?
> 
> Yes.  I believe that the expected full sequence is that 
> 
>   GLOBAL__I_65535_0__ZN2c12f6Ev calls
>     GLOBAL__I__ZN2c12f6Ev calls
>       _Z41__static_initialization_and_destruction_0ii
> 
> and that GLOBAL__I__ZN2c12f6Ev got inlined.
> 
> > But if
> > it has essentially been replaced, why wasn't it just removed entirely?
> 
> An excellent question.  I wonder if it's still marked
> as preserved or something?
> 
> Of course regardless it seems like we're absolutely
> relying on that middle call being inlined and eliminated.
> I can think of two solutions: adjust collect2 to ignore
> local symbols, or rename the middle function so that it
> no longer matches the collect2 pattern.  Both solutions
> seem slightly unpleasent, actually.
> 
> Thoughts, Jan?

The code was setting disregard inline limits.  This is wrong as constructors
might contain stuff that is not really inlinable (and on LTO we might just blow
ourselves off by constructing overly huge functions).

I will take a look why it is not getting inlined in this particular case.  We might
just run off the large function growth limits or something.

But indeed I don't think we should have correctness depending on fact whether
we decided to inline or not.  My plan was to teach collect2 to ignore static symbols.
I didn't get to do that for a while, that is moslty because I am not terribly familiar
with the code and there was always always other problems to look into, but this seems
like correct solution to me.

Honza
> 
> 
> r~
Jan Hubicka - Sept. 28, 2010, 3:36 p.m.
> > I will take a look why it is not getting inlined in this particular case.  We might
> > just run off the large function growth limits or something.
> 
> I believe that it was inlined but a copy was preserved.  The copy was
> not called internally.

Ah, I see, it is bug then, I will take a look into it.
> 
> > But indeed I don't think we should have correctness depending on fact whether
> > we decided to inline or not.  My plan was to teach collect2 to ignore static symbols.
> > I didn't get to do that for a while, that is moslty because I am not terribly familiar
> > with the code and there was always always other problems to look into, but this seems
> > like correct solution to me.
> 
> It's certainly a bug that collect2 doesn't ignore static symbols as
> there is no way collect2 can arrange to call a static function from
> an external object.  However, I also think it's a mistake to use the
> magic names which are detected by collect2 for static constructors
> and destructors.  The names detected by collect2 should always have
> an encoded priority, etc.

This is bit difficult to arrange with LTO.  We produce at compile time the consturctor
function with magic names, then at LTO time we want to produce single constructor function
calling them all.   We would need to guess on what name is the magic name (by same logic
as what collect does) and rename function back.
I can definitly implement it, but it seems more hackish than the collect2 side change.

Honza
> 
> Dave
> -- 
> J. David Anglin                                  dave.anglin@nrc-cnrc.gc.ca
> National Research Council of Canada              (613) 990-0752 (FAX: 952-6602)
Richard Henderson - Sept. 28, 2010, 3:51 p.m.
On 09/28/2010 08:36 AM, Jan Hubicka wrote:
> This is bit difficult to arrange with LTO.  We produce at compile time the consturctor
> function with magic names, then at LTO time we want to produce single constructor function
> calling them all.   We would need to guess on what name is the magic name (by same logic
> as what collect does) and rename function back.
> I can definitly implement it, but it seems more hackish than the collect2 side change.

An alternative is that at compile time we emit

  _Z41__static_initialization_and_destruction_0ii

to the intermediate code as the constructor, and

  GLOBAL__I__ZN2c12f6Ev calls

to the object code calling _Z41.  However, we don't emit
GLOBAL to the intermediate code at all.  Thus when LTO 
replaces the object files there's no trace of the original
GLOBAL function at all, and thus collect2 does not see it.
LTO will simply see _Z41 and call that function directly.

This is not entirely different from the case in which we
have .ctor support -- it's not like we read in the piece
of the object code that contains the .ctor data.  Just
consider the GLOBAL function object file data.

This should be doable with a flag on the decl for GLOBAL
that indicates that it should not be serialized.


r~
Jan Hubicka - Sept. 28, 2010, 4:30 p.m.
> On 09/28/2010 08:36 AM, Jan Hubicka wrote:
> > This is bit difficult to arrange with LTO.  We produce at compile time the consturctor
> > function with magic names, then at LTO time we want to produce single constructor function
> > calling them all.   We would need to guess on what name is the magic name (by same logic
> > as what collect does) and rename function back.
> > I can definitly implement it, but it seems more hackish than the collect2 side change.
> 
> An alternative is that at compile time we emit
> 
>   _Z41__static_initialization_and_destruction_0ii
> 
> to the intermediate code as the constructor, and
> 
>   GLOBAL__I__ZN2c12f6Ev calls
> 
> to the object code calling _Z41.  However, we don't emit
> GLOBAL to the intermediate code at all.  Thus when LTO 
> replaces the object files there's no trace of the original
> GLOBAL function at all, and thus collect2 does not see it.
> LTO will simply see _Z41 and call that function directly.
> 
> This is not entirely different from the case in which we
> have .ctor support -- it's not like we read in the piece
> of the object code that contains the .ctor data.  Just
> consider the GLOBAL function object file data.
> 
> This should be doable with a flag on the decl for GLOBAL
> that indicates that it should not be serialized.

Or we can just produce those collected global constructors after
serialization.  I will check...

Honza
> 
> 
> r~
John David Anglin - Nov. 8, 2010, 9:47 p.m.
On Tue, 28 Sep 2010, Jan Hubicka wrote:

> Or we can just produce those collected global constructors after
> serialization.  I will check...

Any progress in fixing this?

Dave
Steve Ellcey - Dec. 6, 2010, 10:35 p.m.
On 09/28/2010 18:30 AM, Jan Hubicka wrote:

> > On 09/28/2010 08:36 AM, Jan Hubicka wrote:
> > > This is bit difficult to arrange with LTO.  We produce at compile time the consturctor
> > > function with magic names, then at LTO time we want to produce single constructor function
> > > calling them all.   We would need to guess on what name is the magic name (by same logic
> > > as what collect does) and rename function back.
> > > I can definitly implement it, but it seems more hackish than the collect2 side change.
> > 
> > An alternative is that at compile time we emit
> > 
> >   _Z41__static_initialization_and_destruction_0ii
> > 
> > to the intermediate code as the constructor, and
> > 
> >   GLOBAL__I__ZN2c12f6Ev calls
> > 
> > to the object code calling _Z41.  However, we don't emit
> > GLOBAL to the intermediate code at all.  Thus when LTO 
> > replaces the object files there's no trace of the original
> > GLOBAL function at all, and thus collect2 does not see it.
> > LTO will simply see _Z41 and call that function directly.
> > 
> > This is not entirely different from the case in which we
> > have .ctor support -- it's not like we read in the piece
> > of the object code that contains the .ctor data.  Just
> > consider the GLOBAL function object file data.
> > 
> > This should be doable with a flag on the decl for GLOBAL
> > that indicates that it should not be serialized.
> 
> Or we can just produce those collected global constructors after
> serialization.  I will check...
> 
> Honza
> > 
> > 
> > r~

Honza,

Have you looked into this any more?  I haven't seen any follow up from
you since this email.  This defect, PR middle-end/45388, is a P1 defect,
it breaks the hppa bootstrap, and it has been open for over a month.  I
would like to see this fixed, or your patch reverted, before we release
4.6.

Steve Ellcey
sje@cup.hp.com

Patch

Index: ipa.c
===================================================================
--- ipa.c	(revision 164643)
+++ ipa.c	(working copy)
@@ -1477,6 +1477,7 @@  build_cdtor (bool ctor_p, VEC (tree, hea
 	    DECL_STATIC_CONSTRUCTOR (fn) = 0;
 	  else
 	    DECL_STATIC_DESTRUCTOR (fn) = 0;
+	  TREE_PUBLIC (fn) = 1;
 	  /* We do not want to optimize away pure/const calls here.
 	     When optimizing, these should be already removed, when not
 	     optimizing, we want user to be able to breakpoint in them.  */