[Bug c++/49568] New: [4.7 regression] g++.dg/torture/pr41257-2.C FAILs to link on Tru64 UNIX

[Bug c++/49568] New: [4.7 regression] g++.dg/torture/pr41257-2.C FAILs to link on Tru64 UNIX

[ro at gcc dot gnu.org]

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

           Summary: [4.7 regression] g++.dg/torture/pr41257-2.C FAILs to
                    link on Tru64 UNIX
           Product: gcc
           Version: 4.7.0
            Status: UNCONFIRMED
          Keywords: link-failure
          Severity: normal
          Priority: P3
         Component: c++
        AssignedTo: unassigned@gcc.gnu.org
        ReportedBy: ro@gcc.gnu.org
              Host: alpha-dec-osf5.1b
            Target: alpha-dec-osf5.1b
             Build: alpha-dec-osf5.1b


Between 20110502 and 20110518, g++.dg/torture/pr41257-2.C started to FAIL
on Tru64 UNIX:

A::~A()
typeinfo for A
collect2: error: ld returned 1 exit status

which is a link error (undefined symbols).  This is a regression from the 4.6
branch.  While the input file is identical, the generated code is far longer on
mainline, and contains a call to an undefined function _ZN1AD2Ev (A::~A()).

This may be related to the fact that Tru64 UNIX doesn't support weak
definitions,
but the code is plain wrong.

I'm attaching assembler output from 4.6 and 4.7.

[Bug c++/49568] [4.7 regression] g++.dg/torture/pr41257-2.C FAILs to link on Tru64 UNIX

[ro at gcc dot gnu.org]

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

--- Comment #2 from Rainer Orth <ro at gcc dot gnu.org> 2011-06-28 17:39:00 UTC ---
Created attachment 24622
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=24622
gcc 4.7 assembler output at -O0

[Bug c++/49568] [4.7 regression] g++.dg/torture/pr41257-2.C FAILs to link on Tru64 UNIX

[ro at gcc dot gnu.org]

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

--- Comment #1 from Rainer Orth <ro at gcc dot gnu.org> 2011-06-28 17:38:31 UTC ---
Created attachment 24621
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=24621
gcc 4.6 assembler output at -O0

[Bug c++/49568] [4.7 regression] g++.dg/torture/pr41257-2.C FAILs to link on Tru64 UNIX

[rguenth at gcc dot gnu.org]

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

Richard Guenther <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |4.7.0

[Bug c++/49568] [4.7 regression] g++.dg/torture/pr41257-2.C FAILs to link on Tru64 UNIX

[jason at gcc dot gnu.org]

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

Jason Merrill <jason at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2011.06.29 18:09:33
                 CC|                            |hubicka at gcc dot gnu.org,
                   |                            |jason at gcc dot gnu.org
     Ever Confirmed|0                           |1

--- Comment #3 from Jason Merrill <jason at gcc dot gnu.org> 2011-06-29 18:09:33 UTC ---
Confirmed, we shouldn't be emitting ~B because it's not needed.  Probably
introduced by r173517,

2011-05-06  Jan Hubicka  <jh@suse.cz>

        * cgraph.c (cgraph_add_thunk): Create real function node instead
        of alias node; finalize it and mark needed/reachale; arrange visibility
        to be right and add it into the corresponding same comdat group list.
        ...

[Bug c++/49568] [4.7 regression] g++.dg/torture/pr41257-2.C FAILs to link on Tru64 UNIX

[hubicka at ucw dot cz]

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

--- Comment #4 from Jan Hubicka <hubicka at ucw dot cz> 2011-07-01 08:40:13 UTC ---
> Confirmed, we shouldn't be emitting ~B because it's not needed.  Probably
> introduced by r173517,
> 
> 2011-05-06  Jan Hubicka  <jh@suse.cz>
> 
>         * cgraph.c (cgraph_add_thunk): Create real function node instead
>         of alias node; finalize it and mark needed/reachale; arrange visibility
>         to be right and add it into the corresponding same comdat group list.
>         ...
I believed that thunks always belong to same comdat group as the function they
are associated with.
Perhaps same comdat groups behave differently on HP?

I will check.
Honza

[Bug c++/49568] [4.7 regression] g++.dg/torture/pr41257-2.C FAILs to link on Tru64 UNIX

[ro at CeBiTec dot Uni-Bielefeld.DE]

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

--- Comment #5 from ro at CeBiTec dot Uni-Bielefeld.DE <ro at CeBiTec dot Uni-Bielefeld.DE> 2011-07-01 08:48:07 UTC ---
> --- Comment #4 from Jan Hubicka <hubicka at ucw dot cz> 2011-07-01 08:40:13 UTC ---
[...]
> I believed that thunks always belong to same comdat group as the function they
> are associated with.
> Perhaps same comdat groups behave differently on HP?

This is Tru64 UNIX with ECOFF, no named sections, no COMDAT groups.

    Rainer

[Bug c++/49568] [4.7 regression] g++.dg/torture/pr41257-2.C FAILs to link on Tru64 UNIX

[hubicka at ucw dot cz]

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

--- Comment #6 from Jan Hubicka <hubicka at ucw dot cz> 2011-07-01 09:30:41 UTC ---
> This is Tru64 UNIX with ECOFF, no named sections, no COMDAT groups.
Yep, I know, but the question is how absence of COMDAT groups changes behaviour
of thunks.

The problem here is that the thunks are
!cgraph_can_remove_if_no_direct_calls_and_refs_p
and thus we don't optimize them out.
The reason is the test:
2902      if (node->local.externally_visible
2903          && (!DECL_COMDAT (node->decl)
2904              || cgraph_used_from_object_file_p (node)))
2905        return false;

the decl is not comdat, just weak and thus we think we must keep it in program.

The declaration of the destructor itself do have COMDAT flag.
The following patch should fix the problem:
Index: ipa.c
===================================================================
--- ipa.c       (revision 175748)
+++ ipa.c       (working copy)
@@ -871,9 +871,9 @@ function_and_variable_visibility (bool w

             We also need to arrange the thunk into the same comdat group as
             the function it reffers to.  */
+          DECL_COMDAT (node->decl) = DECL_COMDAT (decl_node->decl);
          if (DECL_ONE_ONLY (decl_node->decl))
            {
-             DECL_COMDAT (node->decl) = DECL_COMDAT (decl_node->decl);
              DECL_COMDAT_GROUP (node->decl) = DECL_COMDAT_GROUP
(decl_node->decl);
              if (DECL_ONE_ONLY (decl_node->decl) && !node->same_comdat_group)
                {


Jason, the whole code copying visibilities to thunk decls is just a hack.  Do
you think
you can make C++ FE to put proper visibility flags on thunks and same body
aliases?

Honza

[Bug c++/49568] [4.7 regression] g++.dg/torture/pr41257-2.C FAILs to link on Tru64 UNIX

[ro at CeBiTec dot Uni-Bielefeld.DE]

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

--- Comment #7 from ro at CeBiTec dot Uni-Bielefeld.DE <ro at CeBiTec dot Uni-Bielefeld.DE> 2011-07-01 10:03:24 UTC ---
> The declaration of the destructor itself do have COMDAT flag.
> The following patch should fix the problem:
> Index: ipa.c
> ===================================================================
> --- ipa.c       (revision 175748)
> +++ ipa.c       (working copy)
> @@ -871,9 +871,9 @@ function_and_variable_visibility (bool w
>
>              We also need to arrange the thunk into the same comdat group as
>              the function it reffers to.  */
                                  ^ typo

> +          DECL_COMDAT (node->decl) = DECL_COMDAT (decl_node->decl);
>           if (DECL_ONE_ONLY (decl_node->decl))
>             {
> -             DECL_COMDAT (node->decl) = DECL_COMDAT (decl_node->decl);
>               DECL_COMDAT_GROUP (node->decl) = DECL_COMDAT_GROUP
> (decl_node->decl);
>               if (DECL_ONE_ONLY (decl_node->decl) && !node->same_comdat_group)
>                 {

I can include it in this weekend's bootstrap.

    Rainer

[Bug c++/49568] [4.7 regression] g++.dg/torture/pr41257-2.C FAILs to link on Tru64 UNIX

[jason at gcc dot gnu.org]

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

--- Comment #8 from Jason Merrill <jason at gcc dot gnu.org> 2011-07-01 21:10:46 UTC ---
(In reply to comment #6)
> +          DECL_COMDAT (node->decl) = DECL_COMDAT (decl_node->decl);
>           if (DECL_ONE_ONLY (decl_node->decl))
>             {
> -             DECL_COMDAT (node->decl) = DECL_COMDAT (decl_node->decl);

Looks good to me.

> Jason, the whole code copying visibilities to thunk decls is just a hack.  Do
> you think you can make C++ FE to put proper visibility flags on thunks and
> same body aliases?

I can certainly copy some flags across.  But it seems rather cumbersome to have
to manage same_comdat_group in the front end.  There's also the issue that in
order to set DECL_COMDAT_GROUP (and thus DECL_ONE_ONLY) we need to build the
mangled name for the main decl which otherwise might not be needed.

[Bug c++/49568] [4.7 regression] g++.dg/torture/pr41257-2.C FAILs to link on Tru64 UNIX

[ro at CeBiTec dot Uni-Bielefeld.DE]

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

--- Comment #9 from ro at CeBiTec dot Uni-Bielefeld.DE <ro at CeBiTec dot Uni-Bielefeld.DE> 2011-07-04 17:24:39 UTC ---
> The following patch should fix the problem:
> Index: ipa.c
> ===================================================================
> --- ipa.c       (revision 175748)
> +++ ipa.c       (working copy)
> @@ -871,9 +871,9 @@ function_and_variable_visibility (bool w
>
>              We also need to arrange the thunk into the same comdat group as
>              the function it reffers to.  */
> +          DECL_COMDAT (node->decl) = DECL_COMDAT (decl_node->decl);
>           if (DECL_ONE_ONLY (decl_node->decl))
>             {
> -             DECL_COMDAT (node->decl) = DECL_COMDAT (decl_node->decl);
>               DECL_COMDAT_GROUP (node->decl) = DECL_COMDAT_GROUP
> (decl_node->decl);
>               if (DECL_ONE_ONLY (decl_node->decl) && !node->same_comdat_group)
>                 {

It does indeed and introduces no regressions.

Thanks.
        Rainer

[Bug c++/49568] [4.7 regression] g++.dg/torture/pr41257-2.C FAILs to link on Tru64 UNIX

[jason at gcc dot gnu.org]

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

--- Comment #10 from Jason Merrill <jason at gcc dot gnu.org> 2011-07-06 21:22:21 UTC ---
Author: jason
Date: Wed Jul  6 21:22:16 2011
New Revision: 175938

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=175938
Log:
    PR c++/49568
    * method.c (make_thunk, use_thunk): Copy DECL_COMDAT.

Modified:
    trunk/gcc/cp/ChangeLog
    trunk/gcc/cp/method.c

[Bug c++/49568] [4.7 regression] g++.dg/torture/pr41257-2.C FAILs to link on Tru64 UNIX

[jason at gcc dot gnu.org]

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

Jason Merrill <jason at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |WAITING
         AssignedTo|unassigned at gcc dot       |jason at gcc dot gnu.org
                   |gnu.org                     |

--- Comment #11 from Jason Merrill <jason at gcc dot gnu.org> 2011-07-06 21:23:33 UTC ---
Should be fixed now.

[Bug c++/49568] [4.7 regression] g++.dg/torture/pr41257-2.C FAILs to link on Tru64 UNIX

[ro at gcc dot gnu.org]

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

Rainer Orth <ro at gcc dot gnu.org> changed:

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

--- Comment #12 from Rainer Orth <ro at gcc dot gnu.org> 2011-07-11 10:21:14 UTC ---
Indeed, thanks.

  Rainer