[Bug c++/33579] New: INIT_PRIORITY is broken

[Bug c++/33579] New: INIT_PRIORITY is broken

[danglin at gcc dot gnu dot org]

FAIL: gcc.dg/initpri1.c execution test
FAIL: g++.dg/special/conpr-2.C execution test
FAIL: g++.dg/special/conpr-4.C execution test
FAIL: g++.dg/special/initp1.C execution test
FAIL: g++.dg/special/initpri1.C execution test

On January 4, 2003, I installed a patch which in included support for
init_priority on hppa*-*-hpux11*,
<http://gcc.gnu.org/ml/gcc-patches/2003-01/msg00208.html>.  This used collect2
to support constructors/destructors.

Things started to go wrong with Mark's February 24, 2007 patch,
<http://gcc.gnu.org/ml/gcc-patches/2007-02/msg01950.html>.  However, the
number of init_priority fails has varied somewhat with time.  At the
time of Mark's change, I believe on gcc.dg/initpri1.c started failing.


-- 
           Summary: INIT_PRIORITY is broken
           Product: gcc
           Version: 4.3.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c++
        AssignedTo: unassigned at gcc dot gnu dot org
        ReportedBy: danglin at gcc dot gnu dot org
 GCC build triplet: hppa2.0w-hp-hpux11.11
  GCC host triplet: hppa2.0w-hp-hpux11.11
GCC target triplet: hppa2.0w-hp-hpux11.11


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=33579

[Bug c++/33579] INIT_PRIORITY is broken

[dave at hiauly1 dot hia dot nrc dot ca]

------- Comment #1 from dave at hiauly1 dot hia dot nrc dot ca  2007-09-28 01:41 -------
Subject: Re:   New: INIT_PRIORITY is broken

On Fri, Sep 28, 2007 at 01:35:04AM -0000, danglin at gcc dot gnu dot org wrote:
> FAIL: gcc.dg/initpri1.c execution test

I've attached the assembler output and the collect2/ld log.

Dave


------- Comment #2 from dave at hiauly1 dot hia dot nrc dot ca  2007-09-28 01:41 -------
Created an attachment (id=14261)
 --> (http://gcc.gnu.org/bugzilla/attachment.cgi?id=14261&action=view)


------- Comment #3 from dave at hiauly1 dot hia dot nrc dot ca  2007-09-28 01:41 -------
Created an attachment (id=14262)
 --> (http://gcc.gnu.org/bugzilla/attachment.cgi?id=14262&action=view)


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=33579

[Bug target/33579] INIT_PRIORITY is broken

[danglin at gcc dot gnu dot org]

------- Comment #4 from danglin at gcc dot gnu dot org  2007-10-27 23:45 -------
I believe the SUPPORTS_INIT_PRIORITY portion of my 2003 patch needs
to be reverted.


-- 

danglin at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
          Component|c++                         |target


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=33579

[Bug target/33579] INIT_PRIORITY is broken

[danglin at gcc dot gnu dot org]

------- Comment #5 from danglin at gcc dot gnu dot org  2007-10-28 15:28 -------
Changed my mind about my last comment.  The new constructor priority
attribute breaks the previous C++ init_priority handling using collect2.

With respect to initpr1.c, it can be seen that only one "GLOBAL" constructor,
_GLOBAL__I_0_c1, and one "GLOBAL" destructor, _GLOBAL__D_1_c1, are created.
These respectively call all the constructors and destructors.  The order of
the calls is not sorted based on constructor priority, so the test fails.

However, sorting the calls in _GLOBAL__I_0_c1 and _GLOBAL__D_1_c1 isn't
going provide a consistent ordering across translation units.  The previous
init_priority mechanism was more sophisticated.

A global constructor visible to collect2 was output for each constructor/
destructor priority (e.g., _GLOBAL__I$01000_foo).  These would call
a static function, _Z41__static_initialization_and_destruction_0ii, with
two arguments, construct/destruct and priority.  It would arrange to
call the constructor/destructor for a given priority.  Collect2 sorts
the "GLOBAL" cdtors in terms of priority.  The overall running of constructors
and destructors is done using HP ld's +init and +fini arguments.

At the moment, I don't see any other viable mechanism.  The HP linker
supports a SORT keyword on subspaces (sections) but it is only an eight
bit field and it looks like we need 16 bit priorities.  We also don't
have the equivalent of crtbegin and crtend.

To fix initpri1.c, there would have to be a mechanism similar to the
old init_priority mechanism in GNU C++.


-- 

danglin at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |mark at codesourcery dot com


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=33579

[Bug target/33579] INIT_PRIORITY is broken

[mark at codesourcery dot com]

------- Comment #6 from mark at codesourcery dot com  2007-10-28 22:46 -------
Subject: Re:  INIT_PRIORITY is broken

danglin at gcc dot gnu dot org wrote:

> With respect to initpr1.c, it can be seen that only one "GLOBAL" constructor,
> _GLOBAL__I_0_c1, and one "GLOBAL" destructor, _GLOBAL__D_1_c1, are created.
> These respectively call all the constructors and destructors.  The order of
> the calls is not sorted based on constructor priority, so the test fails.

I'm sorry to hear of this breakage.

> A global constructor visible to collect2 was output for each constructor/
> destructor priority (e.g., _GLOBAL__I$01000_foo).  These would call
> a static function, _Z41__static_initialization_and_destruction_0ii, with
> two arguments, construct/destruct and priority.  It would arrange to
> call the constructor/destructor for a given priority.  Collect2 sorts
> the "GLOBAL" cdtors in terms of priority.  The overall running of constructors
> and destructors is done using HP ld's +init and +fini arguments.

Right now, there are two primary cases for back ends: either they
support constructors and destructors (targetm.have_ctors_dtors is false,
and collect2's special handling is not required), or they don't
(targetm.have_ctors_dtors is false, and collect2 threads the _GLOBAL_*
functions together.)

I believe you're correct that my changes broke the handling of
prioritized constructors in the case where we use collect2.  I didn't
realize that there were targets that did that before.

In order to fix this, I think the correct change would be to have
cgraphunit.c:cgraph_build_cdtor be smarter.  In particular, it should
build one function for each priority, rather than building one function
for everything.  Then, collect2 will work as before.

I don't think this will be too hard to implement.  In
cgraph_build_cdtor_fns, we need to partition/sort the static_[cd]tors by
priority, and then pass each batch off to build_cdtor separately.  Do
you want to work on this, or do you want me to do it?

Thanks,


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=33579

[Bug target/33579] INIT_PRIORITY is broken

[dave at hiauly1 dot hia dot nrc dot ca]

------- Comment #7 from dave at hiauly1 dot hia dot nrc dot ca  2007-10-29 00:35 -------
Subject: Re:  INIT_PRIORITY is broken

> I believe you're correct that my changes broke the handling of
> prioritized constructors in the case where we use collect2.  I didn't
> realize that there were targets that did that before.

It used to be that constructors and destructors were run from _main
and atexit, respectively.  However, this didn't work for dynamically
loaded objects, so I reworked the 32-bit handling to use the linker
+init and +fini options.  While doing this, I noticed that somebody
had added the infrastructure to run prioritized constructors using
collect2.  So, I enabled this feature.

I think Solaris still doesn't have this.

> In order to fix this, I think the correct change would be to have
> cgraphunit.c:cgraph_build_cdtor be smarter.  In particular, it should
> build one function for each priority, rather than building one function
> for everything.  Then, collect2 will work as before.

Sounds right.

> I don't think this will be too hard to implement.  In
> cgraph_build_cdtor_fns, we need to partition/sort the static_[cd]tors by
> priority, and then pass each batch off to build_cdtor separately.  Do
> you want to work on this, or do you want me to do it?

At the moment, I'm finding it more and more difficult to keep up with
GCC issues.  Think to some extent that this is caused by the growth of
the GCC project.  I'm doing fulltime electrical and software design for
a small company here in Ottawa.  At the moment, I have two ARM based
boards on my desk and a video board in the works.  So, I don't expect
to have any significant time off until Christmas.  I appreciate that
you are also extremely busy.

There are two related constructor issues on this target.  PR 33772
is a bug in collect2.  We don't strip the version info from shared
library file names because HP-UX uses .sl.  It is done for shared
libraries using the .so extension.  This causes a problem when a
library is upgraded to a newer minor version using libtool's
versioning scheme.

The problem is libtool links shared libraries against dependent
shared libraries.  When this is done, the HP linker creates
undefined symbols in the library for global symbols in the
dependent libraries.  Thus, a GLOBAL constructor with version
string in a dependent library gets hardcoded into the library.
As a result, it's not possible to replace dependent libraries
without breaking other libraries and applications ;(

In looking at this, I realized it was a bad idea to put DWARF2
EH data in the data section.  We need a lot of `_GLOBAL_F*'
constructors.  This is the main reason we have a problems with
duplicate constructor symbols (PR 30151).  I think these symbols
potentially will cause version update problems since they contain
a random portion.  It's possible PR 30151 is fixed as I don't
see any failures related to this in recent builds.

I think the `_GLOBAL_F*' constructors need to be hidden in a
manner similar to that on linux.  However, we still need collect2
to handle prioritized constructors and destructors since the sort
capability of HP ld are limited.

I have a little patch for the collect2 problem but I'm still
pondering whether to do something different to handle the F
constructors.

Thanks,
Dave


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=33579

[Bug target/33579] INIT_PRIORITY is broken

[mark at codesourcery dot com]

------- Comment #8 from mark at codesourcery dot com  2007-10-30 02:50 -------
Subject: Re:  INIT_PRIORITY is broken

dave at hiauly1 dot hia dot nrc dot ca wrote:

>> I don't think this will be too hard to implement.  In
>> cgraph_build_cdtor_fns, we need to partition/sort the static_[cd]tors by
>> priority, and then pass each batch off to build_cdtor separately.  Do
>> you want to work on this, or do you want me to do it?
> 
> At the moment, I'm finding it more and more difficult to keep up with
> GCC issues.

No problem; it's my bug.  I'll work on a patch, and send it to you to
for testing -- I no longer have an HP-UX machine to work on.

Thanks,


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=33579

[Bug target/33579] INIT_PRIORITY is broken

[mmitchel at gcc dot gnu dot org]

------- Comment #9 from mmitchel at gcc dot gnu dot org  2007-10-30 16:34 -------
Created an attachment (id=14442)
 --> (http://gcc.gnu.org/bugzilla/attachment.cgi?id=14442&action=view)
patch

David --

Would you please try this patch?  I have lightly tested this on a hacked-up x86
configuration, and it seems to do the right thing -- but I'm not certain.  If
it's not right, I expect that it is at least close; please let me know what
happens.

-- Mark


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=33579

[Bug target/33579] INIT_PRIORITY is broken

[dave at hiauly1 dot hia dot nrc dot ca]

------- Comment #10 from dave at hiauly1 dot hia dot nrc dot ca  2007-10-30 17:47 -------
Subject: Re:  INIT_PRIORITY is broken

> Would you please try this patch?

I'll give it a try when I get home tonight.

Thanks,
Dave


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=33579

[Bug target/33579] INIT_PRIORITY is broken

[danglin at gcc dot gnu dot org]

------- Comment #11 from danglin at gcc dot gnu dot org  2007-11-01 03:05 -------
Mark,

This is major progress.  All the priority tests pass and there are no
regressions on hppa2.0w-hp-hpux11.11 and hppa-unknown-linux-gnu.

However, I don't think the patch is quite right.  For example, in the
gcc.dg/initpri1.c test, two identical routines for "c1" are emitted:

        .EXPORT c1,ENTRY,PRIV_LEV=3
c1:
        .PROC
        .CALLINFO FRAME=64,CALLS,SAVE_RP
        .ENTRY
        stw %r2,-20(%r30)
        addil LR'i-$global$,%r27
        ldo 64(%r30),%r30
        ldw RR'i-$global$(%r1),%r19
        ldo 1(%r19),%r28
        comib,<> 0,%r19,L$0036
        stw %r28,RR'i-$global$(%r1)
        ldw -84(%r30),%r2
        bv %r0(%r2)
        ldo -64(%r30),%r30
L$0036:
        .CALL
        bl abort,%r2
        nop
        nop
        .EXIT
        .PROCEND

and

        .EXPORT _GLOBAL__I_500_0_c1,ENTRY,PRIV_LEV=3
_GLOBAL__I_500_0_c1:
        .PROC
        .CALLINFO FRAME=64,CALLS,SAVE_RP
        .ENTRY
        stw %r2,-20(%r30)
        addil LR'i-$global$,%r27
        ldo 64(%r30),%r30
        ldw RR'i-$global$(%r1),%r19
        ldo 1(%r19),%r28
        comib,<> 0,%r19,L$0040
        stw %r28,RR'i-$global$(%r1)
        ldw -84(%r30),%r2
        bv %r0(%r2)
        ldo -64(%r30),%r30
L$0040:
        .CALL
        bl abort,%r2
        nop
        nop
        .EXIT
        .PROCEND

In the test, only _GLOBAL__I_500_0_c1 is actually called.  I believe
_GLOBAL__I_500_0_c1 should call c1, or more optimally be an alias for
c1.  For example, this handles the case where there is static data
private to c1, and c1 is called more than once.

I'm not particularly concerned about using aliases although I think
it is doable with GNU as.

Dave 


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=33579

[Bug target/33579] INIT_PRIORITY is broken

[mark at codesourcery dot com]

------- Comment #12 from mark at codesourcery dot com  2007-11-01 16:50 -------
Subject: Re:  INIT_PRIORITY is broken

danglin at gcc dot gnu dot org wrote:
> ------- Comment #11 from danglin at gcc dot gnu dot org  2007-11-01 03:05 -------
> Mark,
> 
> This is major progress.  All the priority tests pass and there are no
> regressions on hppa2.0w-hp-hpux11.11 and hppa-unknown-linux-gnu.
> 
> However, I don't think the patch is quite right.  For example, in the
> gcc.dg/initpri1.c test, two identical routines for "c1" are emitted:

I don't think that's actually a bug -- except maybe its a
misoptimization.  The compiler's just inlining the calls to c1 from the
_GLOBAL_... functions due to code in record_cdtor_fn:

  node->local.disregard_inline_limits = 1;

I'm not sure that's a great idea -- especially with -Os! -- but I also
don't think it's related to the bug I introduced.

Do you agree?

Thanks,


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=33579

[Bug target/33579] INIT_PRIORITY is broken

[dave at hiauly1 dot hia dot nrc dot ca]

------- Comment #13 from dave at hiauly1 dot hia dot nrc dot ca  2007-11-01 19:45 -------
Subject: Re:  INIT_PRIORITY is broken

> I don't think that's actually a bug -- except maybe its a
> misoptimization.  The compiler's just inlining the calls to c1 from the
> _GLOBAL_... functions due to code in record_cdtor_fn:

I rechecked and the compiler's just inlining the calls.

> Do you agree?

Yes, please go ahead with your fix.

Thanks,
Dave


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=33579

[Bug target/33579] INIT_PRIORITY is broken

[mmitchel at gcc dot gnu dot org]

------- Comment #14 from mmitchel at gcc dot gnu dot org  2007-11-06 00:31 -------
Subject: Bug 33579

Author: mmitchel
Date: Tue Nov  6 00:30:52 2007
New Revision: 129918

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=129918
Log:
        PR target/33579
        * tree.h (DECL_INIT_PRIORITY): Do not require
        DECL_HAS_INIT_PRIORITY_P.
        (DECL_FINI_PRIORITY): Likewise.
        * tree.c (decl_init_priority_lookup): Remove assert.
        (decl_fini_priority_insert): Likewise.
        * cgraphunit.c (static_ctors): Make it a VEC.
        (static_dtors): Likewise.
        (record_cdtor_fn): Adjust accordingly.
        (build_cdtor): Generate multiple functions for each initialization
        priority.
        (compare_ctor): New function.
        (compare_dtor): Likewise.
        (cgraph_build_cdtor_fns): Sort the functions by priority before
        calling build_cdtor.
        (cgraph_build_static_cdtor): Put the priority in the function's
        name.

Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/cgraphunit.c
    trunk/gcc/tree.c
    trunk/gcc/tree.h


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=33579

[Bug target/33579] INIT_PRIORITY is broken

[mmitchel at gcc dot gnu dot org]

------- Comment #15 from mmitchel at gcc dot gnu dot org  2007-11-06 00:43 -------
Fixed in 4.3.0.


-- 

mmitchel at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |RESOLVED
         Resolution|                            |FIXED


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=33579