| Message ID | 201009272029.o8RKTDM18735@lucas.cup.hp.com |
|---|---|
| State | New |
| Headers | show |
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~
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
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~
> 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~
> > 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)
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~
> 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~
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
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
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. */