diff mbox

ipa-visibility TLC 2/n

Message ID 20140525222349.GA30013@kam.mff.cuni.cz
State New
Headers show

Commit Message

Jan Hubicka May 25, 2014, 10:23 p.m. UTC
> On Sun, May 25, 2014 at 6:54 AM, Jan Hubicka <hubicka@ucw.cz> wrote:
> > Hi,
> > this patch adds code to rerite references in vtable initializers to local aliases
> > when doing so is a win.
> >
> > Bootstrapped/regtested x86_64-linux, comitted.
> 
> This is the most likely patch to have caused build failures on
> arm-linux-gnueabihf
>          ^
> /work/trunk-nightlies/builds/build-210913/armv7l-unknown-linux-gnueabihf/libstdc++-v3/include/backward/strstream:143:9:
> note: _std::ostrstream::_ZTVSt10ostrstream_ was declared here
> /work/trunk-nightlies/builds/build-210913/armv7l-unknown-linux-gnueabihf/libstdc++-v3/include/backward/strstream:160:9:
> error: std::strstream::_ZTVSt9strstream.localalias.2 causes a section
> type conflict with std::strstream::_ZTVSt9strstream
>    class strstream : public basic_iostream<char>
>          ^
> /work/trunk-nightlies/builds/build-210913/armv7l-unknown-linux-gnueabihf/libstdc++-v3/include/backward/strstream:160:9:
> note: _std::strstream::_ZTVSt9strstream_ was declared here

I suppose it is because wrong readonly flag on the alias created. Does the following patch help?

> 
> 
> regards
> Ramana
> 
> 
> >
> > Honza
> >
> >         * ipa-visibility.c (can_replace_by_local_alias_in_vtable): New function.
> >         (update_vtable_references): New function.
> >         (function_and_variable_visibility): Rewrite also vtable initializers.
> >         * varpool.c (cgraph_variable_initializer_availability): Remove assert.
> > Index: varpool.c
> > ===================================================================
> > --- varpool.c   (revision 210908)
> > +++ varpool.c   (working copy)
> > @@ -355,7 +355,6 @@ varpool_add_new_variable (tree decl)
> >  enum availability
> >  cgraph_variable_initializer_availability (varpool_node *node)
> >  {
> > -  gcc_assert (cgraph_function_flags_ready);
> >    if (!node->definition)
> >      return AVAIL_NOT_AVAILABLE;
> >    if (!TREE_PUBLIC (node->decl))
> > Index: ipa-visibility.c
> > ===================================================================
> > --- ipa-visibility.c    (revision 210908)
> > +++ ipa-visibility.c    (working copy)
> > @@ -343,6 +343,36 @@ can_replace_by_local_alias (symtab_node
> >           && !symtab_can_be_discarded (node));
> >  }
> >
> > +/* Return true if we can replace refernece to NODE by local alias
> > +   within a virtual table.  Generally we can replace function pointers
> > +   and virtual table pointers.  */
> > +
> > +bool
> > +can_replace_by_local_alias_in_vtable (symtab_node *node)
> > +{
> > +  if (is_a <varpool_node *> (node)
> > +      && !DECL_VIRTUAL_P (node->decl))
> > +    return false;
> > +  return can_replace_by_local_alias (node);
> > +}
> > +
> > +/* walk_tree callback that rewrites initializer references.   */
> > +
> > +static tree
> > +update_vtable_references (tree *tp, int *walk_subtrees, void *data ATTRIBUTE_UNUSED)
> > +{
> > +  if (TREE_CODE (*tp) == VAR_DECL
> > +      || TREE_CODE (*tp) == FUNCTION_DECL)
> > +    {
> > +      if (can_replace_by_local_alias_in_vtable (symtab_get_node (*tp)))
> > +       *tp = symtab_nonoverwritable_alias (symtab_get_node (*tp))->decl;
> > +      *walk_subtrees = 0;
> > +    }
> > +  else if (IS_TYPE_OR_DECL_P (*tp))
> > +    *walk_subtrees = 0;
> > +  return NULL;
> > +}
> > +
> >  /* In LTO we can remove COMDAT groups and weak symbols.
> >     Either turn them into normal symbols or external symbol depending on
> >     resolution info.  */
> > @@ -625,6 +655,34 @@ function_and_variable_visibility (bool w
> >           vnode->resolution = LDPR_PREVAILING_DEF_IRONLY;
> >         }
> >        update_visibility_by_resolution_info (vnode);
> > +
> > +      /* Update virutal tables to point to local aliases where possible.  */
> > +      if (DECL_VIRTUAL_P (vnode->decl)
> > +         && !DECL_EXTERNAL (vnode->decl))
> > +       {
> > +         int i;
> > +         struct ipa_ref *ref;
> > +         bool found = false;
> > +
> > +         /* See if there is something to update.  */
> > +         for (i = 0; ipa_ref_list_referring_iterate (&vnode->ref_list,
> > +                                                     i, ref); i++)
> > +           if (ref->use == IPA_REF_ADDR
> > +               && can_replace_by_local_alias_in_vtable (ref->referred))
> > +             {
> > +               found = true;
> > +               break;
> > +             }
> > +         if (found)
> > +           {
> > +             struct pointer_set_t *visited_nodes = pointer_set_create ();
> > +             walk_tree (&DECL_INITIAL (vnode->decl),
> > +                        update_vtable_references, NULL, visited_nodes);
> > +             pointer_set_destroy (visited_nodes);
> > +             ipa_remove_all_references (&vnode->ref_list);
> > +             record_references_in_initializer (vnode->decl, false);
> > +           }
> > +       }
> >      }
> >
> >    if (dump_file)

Comments

Jan Hubicka May 26, 2014, 1:04 a.m. UTC | #1
> > On Sun, May 25, 2014 at 6:54 AM, Jan Hubicka <hubicka@ucw.cz> wrote:
> > > Hi,
> > > this patch adds code to rerite references in vtable initializers to local aliases
> > > when doing so is a win.
> > >
> > > Bootstrapped/regtested x86_64-linux, comitted.
> > 
> > This is the most likely patch to have caused build failures on
> > arm-linux-gnueabihf
> >          ^
> > /work/trunk-nightlies/builds/build-210913/armv7l-unknown-linux-gnueabihf/libstdc++-v3/include/backward/strstream:143:9:
> > note: _std::ostrstream::_ZTVSt10ostrstream_ was declared here
> > /work/trunk-nightlies/builds/build-210913/armv7l-unknown-linux-gnueabihf/libstdc++-v3/include/backward/strstream:160:9:
> > error: std::strstream::_ZTVSt9strstream.localalias.2 causes a section
> > type conflict with std::strstream::_ZTVSt9strstream
> >    class strstream : public basic_iostream<char>
> >          ^
> > /work/trunk-nightlies/builds/build-210913/armv7l-unknown-linux-gnueabihf/libstdc++-v3/include/backward/strstream:160:9:
> > note: _std::strstream::_ZTVSt9strstream_ was declared here
> 
> I suppose it is because wrong readonly flag on the alias created. Does the following patch help?

I bootstrapped & comitted the patch in meantime.  Even if it does not fix the error, it is obviously
right thing to do.

Honza
Ramana Radhakrishnan May 26, 2014, 5:28 a.m. UTC | #2
On Mon, May 26, 2014 at 2:04 AM, Jan Hubicka <hubicka@ucw.cz> wrote:
>> > On Sun, May 25, 2014 at 6:54 AM, Jan Hubicka <hubicka@ucw.cz> wrote:
>> > > Hi,
>> > > this patch adds code to rerite references in vtable initializers to local aliases
>> > > when doing so is a win.
>> > >
>> > > Bootstrapped/regtested x86_64-linux, comitted.
>> >
>> > This is the most likely patch to have caused build failures on
>> > arm-linux-gnueabihf
>> >          ^
>> > /work/trunk-nightlies/builds/build-210913/armv7l-unknown-linux-gnueabihf/libstdc++-v3/include/backward/strstream:143:9:
>> > note: _std::ostrstream::_ZTVSt10ostrstream_ was declared here
>> > /work/trunk-nightlies/builds/build-210913/armv7l-unknown-linux-gnueabihf/libstdc++-v3/include/backward/strstream:160:9:
>> > error: std::strstream::_ZTVSt9strstream.localalias.2 causes a section
>> > type conflict with std::strstream::_ZTVSt9strstream
>> >    class strstream : public basic_iostream<char>
>> >          ^
>> > /work/trunk-nightlies/builds/build-210913/armv7l-unknown-linux-gnueabihf/libstdc++-v3/include/backward/strstream:160:9:
>> > note: _std::strstream::_ZTVSt9strstream_ was declared here
>>
>> I suppose it is because wrong readonly flag on the alias created. Does the following patch help?
>
> I bootstrapped & comitted the patch in meantime.  Even if it does not fix the error, it is obviously
> right thing to do.

Sadly this doesn't appear to have fixed the issue.

Ramana
>
> Honza
Jan Hubicka May 27, 2014, 8:06 p.m. UTC | #3
> 
> Sadly this doesn't appear to have fixed the issue.

I have now reproduced it on AIX. We die at:
          /* Sanity check user variables for flag changes.  */
          if (sect->named.decl != NULL
              && DECL_P (sect->named.decl)
              && decl != sect->named.decl)
            {
              if (decl != NULL && DECL_P (decl))
                error ("%+D causes a section type conflict with %D",
                       decl, sect->named.decl);
              else
                error ("section type conflict with %D", sect->named.decl);
              inform (DECL_SOURCE_LOCATION (sect->named.decl),
                      "%qD was declared here", sect->named.decl);
            }

here we have decl and its local alias:
(gdb) p debug_tree (sect->named.decl)
 <var_decl 70f7d060 _ZTCSt14basic_ifstreamIcSt11char_traitsIcEE0_Si
    type <array_type 70dd8840
        type <pointer_type 700f5840 __vtbl_ptr_type type <function_type 700f57e0>
            unsigned SI
            size <integer_cst 70006498 constant 32>
            unit size <integer_cst 700064b0 constant 4>
            align 32 symtab 45 alias set 3 canonical type 700f5840
            pointer_to_this <pointer_type 700f5900>>
        BLK
        size <integer_cst 708002d0 constant 320>
        unit size <integer_cst 708000d8 constant 40>
        align 32 symtab 0 alias set 3 canonical type 70dd8840
        domain <integer_type 701a78a0 type <integer_type 7001d000 sizetype>
            type_6 SI size <integer_cst 70006498 32> unit size <integer_cst 700064b0 4>
            align 32 symtab 0 alias set -1 canonical type 701a78a0 precision 32 min <integer_cst 700064c8 0> max <integer_cst 701a5b88 9>>
        pointer_to_this <pointer_type 71272d80>>
    readonly addressable used public static tree_1 tree_5 tree_6 ignored weak in_system_header virtual decl_5 SI file /home/jh/trunk/c/powerpc-ibm-aix7.1.0.0/libstdc++-v3/include/fstream line 444 col 11 size <integer_cst 708002d0 320> unit size <integer_cst 708000d8 40>
    user align 32 context <record_type 701eb000 basic_ifstream> initial <constructor 70f63680>
   
    (mem/u/c:SI (symbol_ref/i:SI ("_ZTCSt14basic_ifstreamIcSt11char_traitsIcEE0_Si") [flags 0x82] <var_decl 70f7d060 _ZTCSt14basic_ifstreamIcSt11char_traitsIcEE0_Si>) [3 _ZTCSt14basic_ifstreamIcSt11char_traitsIcEE0_Si+0 S40 A32])>
$6 = 10
(gdb) p debug_tree (decl)
 <var_decl 714174e0 _ZTCSt14basic_ifstreamIcSt11char_traitsIcEE0_Si.localalias.69
    type <array_type 70dd8840
        type <pointer_type 700f5840 __vtbl_ptr_type type <function_type 700f57e0>
            unsigned SI
            size <integer_cst 70006498 constant 32>
            unit size <integer_cst 700064b0 constant 4>
            align 32 symtab 45 alias set 3 canonical type 700f5840
            pointer_to_this <pointer_type 700f5900>>
        BLK
        size <integer_cst 708002d0 constant 320>
        unit size <integer_cst 708000d8 constant 40>
        align 32 symtab 0 alias set 3 canonical type 70dd8840
        domain <integer_type 701a78a0 type <integer_type 7001d000 sizetype>
            type_6 SI size <integer_cst 70006498 32> unit size <integer_cst 700064b0 4>
            align 32 symtab 0 alias set -1 canonical type 701a78a0 precision 32 min <integer_cst 700064c8 0> max <integer_cst 701a5b88 9>>
        pointer_to_this <pointer_type 71272d80>>
    readonly addressable used static tree_1 tree_5 tree_6 ignored in_system_header decl_5 SI file /home/jh/trunk/c/powerpc-ibm-aix7.1.0.0/libstdc++-v3/include/fstream line 444 col 11 size <integer_cst 708002d0 320> unit size <integer_cst 708000d8 40>
    user align 32 context <record_type 701eb000 basic_ifstream>
   >
$7 = 10

this seems correct - the alias definitely belongs to the section that its terget is placed into.
I am not however sure what the test is really checking.   It seems to insist on named sections
to consist only of one decl.

The error happens when we attempt to produce RTL for alias:
#0  _Z5errorPKcz (gmsgid=0x11775e44 <__gmpn_bases+42484> "%+D causes a section type conflict with %D") at ../../gcc/diagnostic.c:1048
#1  0x1088d0a0 in _Z11get_sectionPKcjP9tree_node (name=0x711de9d0 "_ZTCSt14basic_ifstreamIcSt11char_traitsIcEE0_Si", flags=2097667, decl=0x714174e0) at ../../gcc/varasm.c:320
#2  0x1088d4d0 in _Z17get_named_sectionP9tree_nodePKci (decl=0x714174e0, name=0x711de9d0 "_ZTCSt14basic_ifstreamIcSt11char_traitsIcEE0_Si", reloc=0) at ../../gcc/varasm.c:419
#3  0x1088f2e8 in _Z20get_variable_sectionP9tree_nodeb (decl=0x714174e0, prefer_noswitch_p=true) at ../../gcc/varasm.c:1112
#4  0x1088f594 in _ZL18get_block_for_declP9tree_node (decl=0x714174e0) at ../../gcc/varasm.c:1161
#5  0x1088ff60 in _Z13make_decl_rtlP9tree_node (decl=0x714174e0) at ../../gcc/varasm.c:1376

Perhaps get_vairable_section should look for alias target, since that is the
decl really deciding on the section? Richard?

Honza
diff mbox

Patch

Index: symtab.c
===================================================================
--- symtab.c	(revision 210914)
+++ symtab.c	(working copy)
@@ -1163,7 +1163,10 @@ 
 				 (new_decl, node->decl);
     }
   else
-    new_node = varpool_create_variable_alias (new_decl, node->decl);
+    {
+      DECL_READONLY (new_decl) = DECL_READONLY (node->decl);
+      new_node = varpool_create_variable_alias (new_decl, node->decl);
+    }
   symtab_resolve_alias (new_node, node);  
   gcc_assert (decl_binds_to_current_def_p (new_decl));
   return new_node;