Fix PR 31903, type info for types in anonymous namespaces

Fix PR 31903, type info for types in anonymous namespaces

[Geoffrey Keating]

In the 'else' clause right below the code I remove, there is:

      else if (TREE_CODE (decl) == VAR_DECL && DECL_TINFO_P (decl))
        {
          /* tinfo visibility is based on the type it's for.  */
          constrain_visibility
            (decl, type_visibility (TREE_TYPE (DECL_NAME (decl))));
        }

which is more correct, and in the presence of the code I deleted, is
never used for type info of class types.

Tested with 'make bootstrap', rebuilding libsdc++, and running the C++
dejagnu testsuite on i386-apple-darwin9.

-- 
- Geoffrey Keating <geoffk@apple.com>

===File ~/patches/gcc-cp-tinfovis.patch=====================
Index: gcc/cp/ChangeLog
2007-06-13  geoffk  <geoffk@apple.com>

	* decl2.c (determine_visibility): Remove duplicate code for
	handling type info.

Index: gcc/testsuite/ChangeLog
2007-06-13  Geoff Keating  <geoffk@apple.com>

	* g++.dg/ext/visibility/anon4.C: New.

Index: gcc/cp/decl2.c
===================================================================
--- gcc/cp/decl2.c	(revision 125627)
+++ gcc/cp/decl2.c	(working copy)
@@ -1698,10 +1698,6 @@
      class can influence the visibility of the DECL.  */
   if (DECL_CLASS_SCOPE_P (decl))
     class_type = DECL_CONTEXT (decl);
-  else if (TREE_CODE (decl) == VAR_DECL
-	   && DECL_TINFO_P (decl)
-	   && CLASS_TYPE_P (TREE_TYPE (DECL_NAME (decl))))
-    class_type = TREE_TYPE (DECL_NAME (decl));
   else
     {
       /* Not a class member.  */
Index: gcc/testsuite/g++.dg/ext/visibility/anon4.C
===================================================================
--- gcc/testsuite/g++.dg/ext/visibility/anon4.C	(revision 0)
+++ gcc/testsuite/g++.dg/ext/visibility/anon4.C	(revision 0)
@@ -0,0 +1,16 @@
+// PR c++/31903
+// Test for anonymous namespace internal linkage, for typeinfo
+
+// { dg-do compile }
+// { dg-final { scan-assembler-not "globl.*_ZTIN*3fooE" } }
+
+#include <typeinfo>
+namespace 
+{
+  class foo 
+  {
+    virtual void bar();
+  };
+}
+
+const std::type_info &X = typeid(foo);
============================================================

Re: Fix PR 31903, type info for types in anonymous namespaces

[Richard Guenther]

On 6/14/07, Geoffrey Keating <gkeating@apple.com> wrote:
>
> In the 'else' clause right below the code I remove, there is:
>
>       else if (TREE_CODE (decl) == VAR_DECL && DECL_TINFO_P (decl))
>         {
>           /* tinfo visibility is based on the type it's for.  */
>           constrain_visibility
>             (decl, type_visibility (TREE_TYPE (DECL_NAME (decl))));
>         }
>
> which is more correct, and in the presence of the code I deleted, is
> never used for type info of class types.
>
> Tested with 'make bootstrap', rebuilding libsdc++, and running the C++
> dejagnu testsuite on i386-apple-darwin9.

This causes PR33871 and reverting the change still ends up with the testcase
working.  On *-linux-gnu that is.  Any idea why?

Please investigate.

Thanks,
Richard.

> --
> - Geoffrey Keating <geoffk@apple.com>
>
> ===File ~/patches/gcc-cp-tinfovis.patch=====================
> Index: gcc/cp/ChangeLog
> 2007-06-13  geoffk  <geoffk@apple.com>
>
>         * decl2.c (determine_visibility): Remove duplicate code for
>         handling type info.
>
> Index: gcc/testsuite/ChangeLog
> 2007-06-13  Geoff Keating  <geoffk@apple.com>
>
>         * g++.dg/ext/visibility/anon4.C: New.
>
> Index: gcc/cp/decl2.c
> ===================================================================
> --- gcc/cp/decl2.c      (revision 125627)
> +++ gcc/cp/decl2.c      (working copy)
> @@ -1698,10 +1698,6 @@
>       class can influence the visibility of the DECL.  */
>    if (DECL_CLASS_SCOPE_P (decl))
>      class_type = DECL_CONTEXT (decl);
> -  else if (TREE_CODE (decl) == VAR_DECL
> -          && DECL_TINFO_P (decl)
> -          && CLASS_TYPE_P (TREE_TYPE (DECL_NAME (decl))))
> -    class_type = TREE_TYPE (DECL_NAME (decl));
>    else
>      {
>        /* Not a class member.  */
> Index: gcc/testsuite/g++.dg/ext/visibility/anon4.C
> ===================================================================
> --- gcc/testsuite/g++.dg/ext/visibility/anon4.C (revision 0)
> +++ gcc/testsuite/g++.dg/ext/visibility/anon4.C (revision 0)
> @@ -0,0 +1,16 @@
> +// PR c++/31903
> +// Test for anonymous namespace internal linkage, for typeinfo
> +
> +// { dg-do compile }
> +// { dg-final { scan-assembler-not "globl.*_ZTIN*3fooE" } }
> +
> +#include <typeinfo>
> +namespace
> +{
> +  class foo
> +  {
> +    virtual void bar();
> +  };
> +}
> +
> +const std::type_info &X = typeid(foo);
> ============================================================
>

Re: Fix PR 31903, type info for types in anonymous namespaces

[Richard Guenther]

On 10/29/07, Richard Guenther <richard.guenther@gmail.com> wrote:
> On 6/14/07, Geoffrey Keating <gkeating@apple.com> wrote:
> >
> > In the 'else' clause right below the code I remove, there is:
> >
> >       else if (TREE_CODE (decl) == VAR_DECL && DECL_TINFO_P (decl))
> >         {
> >           /* tinfo visibility is based on the type it's for.  */
> >           constrain_visibility
> >             (decl, type_visibility (TREE_TYPE (DECL_NAME (decl))));
> >         }
> >
> > which is more correct, and in the presence of the code I deleted, is
> > never used for type info of class types.
> >
> > Tested with 'make bootstrap', rebuilding libsdc++, and running the C++
> > dejagnu testsuite on i386-apple-darwin9.
>
> This causes PR33871 and reverting the change still ends up with the testcase
> working.  On *-linux-gnu that is.  Any idea why?
>
> Please investigate.

In fact, I think the new testcase should _fail_ now, but the matching
pattern doesn't
fail on

        .file   "anon4.C"
.globl X
        .section        .rodata
        .align 8
        .type   X, @object
        .size   X, 8
X:
        .quad   _ZTIN12_GLOBAL__N_13fooE
        .section        .gnu.linkonce.r._ZTSN12_GLOBAL__N_13fooE,"a",@progbits
        .align 16
        .type   _ZTSN12_GLOBAL__N_13fooE, @object
        .size   _ZTSN12_GLOBAL__N_13fooE, 21
_ZTSN12_GLOBAL__N_13fooE:
        .string "N12_GLOBAL__N_13fooE"
        .section        .rodata
        .align 16
        .type   _ZTIN12_GLOBAL__N_13fooE, @object
        .size   _ZTIN12_GLOBAL__N_13fooE, 16
_ZTIN12_GLOBAL__N_13fooE:
        .quad   _ZTVN10__cxxabiv117__class_type_infoE+16
        .quad   _ZTSN12_GLOBAL__N_13fooE
        .ident  "GCC: (GNU) 4.3.0 20071029 (experimental)"
        .section        .note.GNU-stack,"",@progbits

Richard.

Re: Fix PR 31903, type info for types in anonymous namespaces

[Geoffrey Keating]

[-- Attachment #1: Type: text/plain, Size: 941 bytes --]


On 29/10/2007, at 6:25 AM, Richard Guenther wrote:

> On 6/14/07, Geoffrey Keating <gkeating@apple.com> wrote:
>>
>> In the 'else' clause right below the code I remove, there is:
>>
>>      else if (TREE_CODE (decl) == VAR_DECL && DECL_TINFO_P (decl))
>>        {
>>          /* tinfo visibility is based on the type it's for.  */
>>          constrain_visibility
>>            (decl, type_visibility (TREE_TYPE (DECL_NAME (decl))));
>>        }
>>
>> which is more correct, and in the presence of the code I deleted, is
>> never used for type info of class types.
>>
>> Tested with 'make bootstrap', rebuilding libsdc++, and running the C 
>> ++
>> dejagnu testsuite on i386-apple-darwin9.
>
> This causes PR33871 and reverting the change still ends up with the  
> testcase
> working.  On *-linux-gnu that is.  Any idea why?

Darwin and linux are probably different in the paths they take through  
this code.  I commented more in the PR.

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 2462 bytes --]

Re: Fix PR 31903, type info for types in anonymous namespaces

[Geoffrey Keating]

[-- Attachment #1: Type: text/plain, Size: 2211 bytes --]


On 29/10/2007, at 6:41 AM, Richard Guenther wrote:

> On 10/29/07, Richard Guenther <richard.guenther@gmail.com> wrote:
>> On 6/14/07, Geoffrey Keating <gkeating@apple.com> wrote:
>>>
>>> In the 'else' clause right below the code I remove, there is:
>>>
>>>      else if (TREE_CODE (decl) == VAR_DECL && DECL_TINFO_P (decl))
>>>        {
>>>          /* tinfo visibility is based on the type it's for.  */
>>>          constrain_visibility
>>>            (decl, type_visibility (TREE_TYPE (DECL_NAME (decl))));
>>>        }
>>>
>>> which is more correct, and in the presence of the code I deleted, is
>>> never used for type info of class types.
>>>
>>> Tested with 'make bootstrap', rebuilding libsdc++, and running the  
>>> C++
>>> dejagnu testsuite on i386-apple-darwin9.
>>
>> This causes PR33871 and reverting the change still ends up with the  
>> testcase
>> working.  On *-linux-gnu that is.  Any idea why?
>>
>> Please investigate.
>
> In fact, I think the new testcase should _fail_ now, but the matching
> pattern doesn't
> fail on
>
>        .file   "anon4.C"
> .globl X
>        .section        .rodata
>        .align 8
>        .type   X, @object
>        .size   X, 8
> X:
>        .quad   _ZTIN12_GLOBAL__N_13fooE
>        .section 
>         .gnu.linkonce.r._ZTSN12_GLOBAL__N_13fooE,"a",@progbits
>        .align 16
>        .type   _ZTSN12_GLOBAL__N_13fooE, @object
>        .size   _ZTSN12_GLOBAL__N_13fooE, 21
> _ZTSN12_GLOBAL__N_13fooE:
>        .string "N12_GLOBAL__N_13fooE"
>        .section        .rodata
>        .align 16
>        .type   _ZTIN12_GLOBAL__N_13fooE, @object
>        .size   _ZTIN12_GLOBAL__N_13fooE, 16
> _ZTIN12_GLOBAL__N_13fooE:
>        .quad   _ZTVN10__cxxabiv117__class_type_infoE+16
>        .quad   _ZTSN12_GLOBAL__N_13fooE
>        .ident  "GCC: (GNU) 4.3.0 20071029 (experimental)"
>        .section        .note.GNU-stack,"",@progbits

Could you explain more about why you think it should fail?  I only see  
one instance of '.globl' in this snippet of assembler and it's not the  
one the testcase is checking for, which would look like

.globl _ZTIN12_GLOBAL__N_13fooE

(note that the testcase is trying to make sure that .globl does *not*  
appear.)


[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 2462 bytes --]

Re: Fix PR 31903, type info for types in anonymous namespaces

[Richard Guenther]

On 10/30/07, Geoffrey Keating <geoffk@apple.com> wrote:
>
> On 29/10/2007, at 6:41 AM, Richard Guenther wrote:
>
> > On 10/29/07, Richard Guenther <richard.guenther@gmail.com> wrote:
> >> On 6/14/07, Geoffrey Keating <gkeating@apple.com> wrote:
> >>>
> >>> In the 'else' clause right below the code I remove, there is:
> >>>
> >>>      else if (TREE_CODE (decl) == VAR_DECL && DECL_TINFO_P (decl))
> >>>        {
> >>>          /* tinfo visibility is based on the type it's for.  */
> >>>          constrain_visibility
> >>>            (decl, type_visibility (TREE_TYPE (DECL_NAME (decl))));
> >>>        }
> >>>
> >>> which is more correct, and in the presence of the code I deleted, is
> >>> never used for type info of class types.
> >>>
> >>> Tested with 'make bootstrap', rebuilding libsdc++, and running the
> >>> C++
> >>> dejagnu testsuite on i386-apple-darwin9.
> >>
> >> This causes PR33871 and reverting the change still ends up with the
> >> testcase
> >> working.  On *-linux-gnu that is.  Any idea why?
> >>
> >> Please investigate.
> >
> > In fact, I think the new testcase should _fail_ now, but the matching
> > pattern doesn't
> > fail on
> >
> >        .file   "anon4.C"
> > .globl X
> >        .section        .rodata
> >        .align 8
> >        .type   X, @object
> >        .size   X, 8
> > X:
> >        .quad   _ZTIN12_GLOBAL__N_13fooE
> >        .section
> >         .gnu.linkonce.r._ZTSN12_GLOBAL__N_13fooE,"a",@progbits
> >        .align 16
> >        .type   _ZTSN12_GLOBAL__N_13fooE, @object
> >        .size   _ZTSN12_GLOBAL__N_13fooE, 21
> > _ZTSN12_GLOBAL__N_13fooE:
> >        .string "N12_GLOBAL__N_13fooE"
> >        .section        .rodata
> >        .align 16
> >        .type   _ZTIN12_GLOBAL__N_13fooE, @object
> >        .size   _ZTIN12_GLOBAL__N_13fooE, 16
> > _ZTIN12_GLOBAL__N_13fooE:
> >        .quad   _ZTVN10__cxxabiv117__class_type_infoE+16
> >        .quad   _ZTSN12_GLOBAL__N_13fooE
> >        .ident  "GCC: (GNU) 4.3.0 20071029 (experimental)"
> >        .section        .note.GNU-stack,"",@progbits
>
> Could you explain more about why you think it should fail?  I only see
> one instance of '.globl' in this snippet of assembler and it's not the
> one the testcase is checking for, which would look like
>
> .globl _ZTIN12_GLOBAL__N_13fooE
>
> (note that the testcase is trying to make sure that .globl does *not*
> appear.)

The symbols were weak before and are now not weak.  readelf shows
after your patch:

Symbol table '.symtab' contains 38 entries:
   Num:    Value          Size Type    Bind   Vis      Ndx Name
...
    14: 0000000000000000     0 SECTION LOCAL  DEFAULT   16
    15: 0000000000000020    24 OBJECT  LOCAL  DEFAULT   16
_ZTIN12_GLOBAL__N_11tE
    16: 0000000000000000     0 SECTION LOCAL  DEFAULT   18
    17: 0000000000000000    19 OBJECT  LOCAL  DEFAULT   18
_ZTSN12_GLOBAL__N_11tE

while before your patch

 26: 0000000000000000    24 OBJECT  WEAK   DEFAULT   12 _ZTIN12_GLOBAL__N_11tE
28: 0000000000000000    19 OBJECT  WEAK   DEFAULT   14 _ZTSN12_GLOBAL__N_11tE

I don't know what version is more "correct", but certainly you did change
behavior and that changed behavior broke numerous applications at
link time.  So I ask you to provide either reasoning for the change
(citing ABIs or whatever) or to restore the previous behavior to unbreak
applications.

Thanks,
Richard.

Re: Fix PR 31903, type info for types in anonymous namespaces

[Andrew Pinski]

On 10/30/07, Richard Guenther <richard.guenther@gmail.com> wrote:
> The symbols were weak before and are now not weak.  readelf shows
> after your patch:

Yes and they changed from being global to being local symbols.  So
this change is correct and something else is wrong.

-- Pinski

Re: Fix PR 31903, type info for types in anonymous namespaces

[Geoffrey Keating]

[-- Attachment #1: Type: text/plain, Size: 3205 bytes --]


On 30/10/2007, at 11:39 AM, Richard Guenther wrote:

> On 10/30/07, Geoffrey Keating <geoffk@apple.com> wrote:
>>
>> On 29/10/2007, at 6:41 AM, Richard Guenther wrote:
>>
>>> On 10/29/07, Richard Guenther <richard.guenther@gmail.com> wrote:
>>>> On 6/14/07, Geoffrey Keating <gkeating@apple.com> wrote:
>>>>>
>>>>> In the 'else' clause right below the code I remove, there is:
>>>>>
>>>>>     else if (TREE_CODE (decl) == VAR_DECL && DECL_TINFO_P (decl))
>>>>>       {
>>>>>         /* tinfo visibility is based on the type it's for.  */
>>>>>         constrain_visibility
>>>>>           (decl, type_visibility (TREE_TYPE (DECL_NAME (decl))));
>>>>>       }
>>>>>
>>>>> which is more correct, and in the presence of the code I  
>>>>> deleted, is
>>>>> never used for type info of class types.
>>>>>
>>>>> Tested with 'make bootstrap', rebuilding libsdc++, and running the
>>>>> C++
>>>>> dejagnu testsuite on i386-apple-darwin9.
>>>>
>>>> This causes PR33871 and reverting the change still ends up with the
>>>> testcase
>>>> working.  On *-linux-gnu that is.  Any idea why?
>>>>
>>>> Please investigate.
>>>
>>> In fact, I think the new testcase should _fail_ now, but the  
>>> matching
>>> pattern doesn't
>>> fail on
>>>
>>>       .file   "anon4.C"
>>> .globl X
>>>       .section        .rodata
>>>       .align 8
>>>       .type   X, @object
>>>       .size   X, 8
>>> X:
>>>       .quad   _ZTIN12_GLOBAL__N_13fooE
>>>       .section
>>>        .gnu.linkonce.r._ZTSN12_GLOBAL__N_13fooE,"a",@progbits
>>>       .align 16
>>>       .type   _ZTSN12_GLOBAL__N_13fooE, @object
>>>       .size   _ZTSN12_GLOBAL__N_13fooE, 21
>>> _ZTSN12_GLOBAL__N_13fooE:
>>>       .string "N12_GLOBAL__N_13fooE"
>>>       .section        .rodata
>>>       .align 16
>>>       .type   _ZTIN12_GLOBAL__N_13fooE, @object
>>>       .size   _ZTIN12_GLOBAL__N_13fooE, 16
>>> _ZTIN12_GLOBAL__N_13fooE:
>>>       .quad   _ZTVN10__cxxabiv117__class_type_infoE+16
>>>       .quad   _ZTSN12_GLOBAL__N_13fooE
>>>       .ident  "GCC: (GNU) 4.3.0 20071029 (experimental)"
>>>       .section        .note.GNU-stack,"",@progbits
>>
>> Could you explain more about why you think it should fail?  I only  
>> see
>> one instance of '.globl' in this snippet of assembler and it's not  
>> the
>> one the testcase is checking for, which would look like
>>
>> .globl _ZTIN12_GLOBAL__N_13fooE
>>
>> (note that the testcase is trying to make sure that .globl does *not*
>> appear.)
>
> The symbols were weak before and are now not weak.

Of course they are not weak, only global symbols can be weak.

> I don't know what version is more "correct", but certainly you did  
> change
> behavior and that changed behavior broke numerous applications at
> link time.

It can't be that many, it took four months for anyone to report a  
problem.  Perhaps what you're seeing only happens with specific buggy  
versions of GNU ld?

>  So I ask you to provide either reasoning for the change
> (citing ABIs or whatever) or to restore the previous behavior to  
> unbreak
> applications.

What's unreasonable about "tinfo visibility is based on the type it's  
for"?

Let's continue this discussion in PR33871 where I provide additional  
explanation.

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 2462 bytes --]

Re: Fix PR 31903, type info for types in anonymous namespaces

[Richard Guenther]

On 10/30/07, Andrew Pinski <pinskia@gmail.com> wrote:
> On 10/30/07, Richard Guenther <richard.guenther@gmail.com> wrote:
> > The symbols were weak before and are now not weak.  readelf shows
> > after your patch:
>
> Yes and they changed from being global to being local symbols.  So
> this change is correct and something else is wrong.

They are referenced from the global (but weak) vtable for s via

Relocation section '.rela.gnu.linkonce.r._ZTI1s' at offset 0xf38
contains 2 entries:
  Offset          Info           Type           Sym. Value    Sym. Name + Addend
000000000000  001c00000001 R_X86_64_64       0000000000000000
_ZTVN10__cxxabiv117__c + 10
000000000008  001d00000001 R_X86_64_64       0000000000000000 _ZTS1s + 0

Relocation section '.rela.gnu.linkonce.r._ZTV1s' at offset 0xf68
contains 2 entries:
  Offset          Info           Type           Sym. Value    Sym. Name + Addend
000000000008  001b00000001 R_X86_64_64       0000000000000000 _ZTI1s + 0
000000000010  001800000001 R_X86_64_64       0000000000000000 _ZN1s1mEv + 0

so the reference to the typeinfo name _ZTS1s is not locally bound?
Or maybe the linker is confused by this situation.

Richard.

Re: Fix PR 31903, type info for types in anonymous namespaces

[Andrew Pinski]

On 10/30/07, Richard Guenther <richard.guenther@gmail.com> wrote:
> so the reference to the typeinfo name _ZTS1s is not locally bound?
> Or maybe the linker is confused by this situation.

_ZN1s1mEv (s::m() ) should not exist as it is a pure virtual function.

-- Pinski