On Tue, May 14, 2013 at 11:12 AM, Jan Hubicka <hubi...@ucw.cz> wrote:
> Hi,
> this patch fixes with weakrefs seen on building latest firefox. The problem
> is
> that currently we handle weakrefs as external variables/functions that makes
> us
> to merge them. In firefox there are two weakrefs with different aliases used
> in different units. This is correct and well defined even if weird use.
>
> This patch adds special cases for weakrefs into lto-symtab and lto-partition
> to make them go through correctly. I also fixed two fallouts of my previous
> change that reproduce on firefox (and the testcase addded).
>
> For lto-partition the weakrefs with defined target are even bit more special
> animals, since they needs to be duplicated into every unit that use them.
>
> It is ugly to special case wekarefs all around. My plan is to cleanup whole
> area, but it seems that the correctness issue deserve to be fixed first.
>
> I have bootstrapped/regtested x86_64-linux and tested mozilla build. Will
> commit it tonight if there are no complains.
Does this affect the 4.8 branch, too?
Thanks,
Richard.
> Honza
>
> PR lto/57038
> PR lto/47375
> * lto-symtab.c (lto_symtab_symbol_p): Add external symbol; weakrefs
> are
> not external.
> (lto_symtab_merge_decls): Fix thinko when dealing with non-lto_symtab
> decls.
> (lto_symtab_merge_cgraph_nodes): Use lto_symtab_symbol_p.
> (lto_symtab_prevailing_decl): Get int sync with lto_symtab_symbol_p.
> * varpool.c (dump_varpool_node): Dump more flags.
>
> * lto-partition.c (get_symbol_class): Fix weakrefs.
> (lto_balanced_map): Fix weakrefs.
> (privatize_symbol_name): Remove unnecesary label.
> (rename_statics): Handle weakrefs as statics.
>
> * gcc.dg/lto/attr-weakref-1_0.c: New testcase.
> * gcc.dg/lto/attr-weakref-1_1.c: New testcase.
> * gcc.dg/lto/attr-weakref-1_2.c: New testcase.
> Index: lto-symtab.c
> ===================================================================
> *** lto-symtab.c (revision 198824)
> --- lto-symtab.c (working copy)
> *************** lto_symtab_resolve_replaceable_p (symtab
> *** 227,239 ****
> }
>
> /* Return true, if the symbol E should be resolved by lto-symtab.
> ! Those are all real symbols that are not static (we handle renaming
> ! of static later in partitioning). */
>
> static bool
> lto_symtab_symbol_p (symtab_node e)
> {
> ! if (!TREE_PUBLIC (e->symbol.decl))
> return false;
> return symtab_real_symbol_p (e);
> }
> --- 227,242 ----
> }
>
> /* Return true, if the symbol E should be resolved by lto-symtab.
> ! Those are all external symbols and all real symbols that are not static
> (we
> ! handle renaming of static later in partitioning). */
>
> static bool
> lto_symtab_symbol_p (symtab_node e)
> {
> ! if (!TREE_PUBLIC (e->symbol.decl) && !DECL_EXTERNAL (e->symbol.decl))
> ! return false;
> ! /* weakrefs are really static variables that are made external by a hack.
> */
> ! if (lookup_attribute ("weakref", DECL_ATTRIBUTES (e->symbol.decl)))
> return false;
> return symtab_real_symbol_p (e);
> }
> *************** lto_symtab_merge_decls (void)
> *** 528,537 ****
> symtab_initialize_asm_name_hash ();
>
> FOR_EACH_SYMBOL (node)
> ! if (TREE_PUBLIC (node->symbol.decl)
> ! && node->symbol.next_sharing_asm_name
> ! && !node->symbol.previous_sharing_asm_name)
> ! lto_symtab_merge_decls_1 (node);
> }
>
> /* Helper to process the decl chain for the symbol table entry *SLOT. */
> --- 531,549 ----
> symtab_initialize_asm_name_hash ();
>
> FOR_EACH_SYMBOL (node)
> ! if (lto_symtab_symbol_p (node)
> ! && node->symbol.next_sharing_asm_name)
> ! {
> ! symtab_node n;
> !
> ! /* To avoid duplicated work, see if this is first real symbol in the
> ! chain. */
> ! for (n = node->symbol.previous_sharing_asm_name;
> ! n && !lto_symtab_symbol_p (n); n =
> n->symbol.previous_sharing_asm_name)
> ! ;
> ! if (!n)
> ! lto_symtab_merge_decls_1 (node);
> ! }
> }
>
> /* Helper to process the decl chain for the symbol table entry *SLOT. */
> *************** lto_symtab_merge_cgraph_nodes (void)
> *** 574,580 ****
>
> if (!flag_ltrans)
> FOR_EACH_SYMBOL (node)
> ! if (TREE_PUBLIC (node->symbol.decl)
> && node->symbol.next_sharing_asm_name
> && !node->symbol.previous_sharing_asm_name)
> lto_symtab_merge_cgraph_nodes_1 (node);
> --- 586,592 ----
>
> if (!flag_ltrans)
> FOR_EACH_SYMBOL (node)
> ! if (lto_symtab_symbol_p (node)
> && node->symbol.next_sharing_asm_name
> && !node->symbol.previous_sharing_asm_name)
> lto_symtab_merge_cgraph_nodes_1 (node);
> *************** lto_symtab_prevailing_decl (tree decl)
> *** 602,608 ****
> symtab_node ret;
>
> /* Builtins and local symbols are their own prevailing decl. */
> ! if (!TREE_PUBLIC (decl) || is_builtin_fn (decl))
> return decl;
>
> /* DECL_ABSTRACTs are their own prevailng decl. */
> --- 614,620 ----
> symtab_node ret;
>
> /* Builtins and local symbols are their own prevailing decl. */
> ! if ((!TREE_PUBLIC (decl) && !DECL_EXTERNAL (decl)) || is_builtin_fn
> (decl))
> return decl;
>
> /* DECL_ABSTRACTs are their own prevailng decl. */
> *************** lto_symtab_prevailing_decl (tree decl)
> *** 614,619 ****
> --- 626,636 ----
> if (TREE_CODE (decl) == FUNCTION_DECL && DECL_BUILT_IN (decl))
> return decl;
>
> + /* As an anoying special cases weakrefs are really static variables with
> + EXTERNAL flag. */
> + if (lookup_attribute ("weakref", DECL_ATTRIBUTES (decl)))
> + return decl;
> +
> /* Ensure DECL_ASSEMBLER_NAME will not set assembler name. */
> gcc_assert (DECL_ASSEMBLER_NAME_SET_P (decl));
>
> Index: testsuite/gcc.dg/lto/attr-weakref-1_0.c
> ===================================================================
> *** testsuite/gcc.dg/lto/attr-weakref-1_0.c (revision 0)
> --- testsuite/gcc.dg/lto/attr-weakref-1_0.c (revision 0)
> ***************
> *** 0 ****
> --- 1,29 ----
> + /* { dg-lto-do run } */
> + int first = 0;
> + void abort (void);
> + int second = 0;
> + void callmealias (void)
> + {
> + if (!first || !second)
> + abort ();
> + }
> + void callmefirst (void)
> + {
> + if (first)
> + abort();
> + first = 1;
> + }
> + void callmesecond (void)
> + {
> + if (!first)
> + abort();
> + if (second)
> + abort();
> + second = 1;
> + }
> + main()
> + {
> + c();
> + b();
> + return 0;
> + }
> Index: testsuite/gcc.dg/lto/attr-weakref-1_1.c
> ===================================================================
> *** testsuite/gcc.dg/lto/attr-weakref-1_1.c (revision 0)
> --- testsuite/gcc.dg/lto/attr-weakref-1_1.c (revision 0)
> ***************
> *** 0 ****
> --- 1,7 ----
> + extern void callmesecond();
> + static void callmealias() __attribute__((weakref ("callmesecond")));
> +
> + b()
> + {
> + callmealias();
> + }
> Index: testsuite/gcc.dg/lto/attr-weakref-1_2.c
> ===================================================================
> *** testsuite/gcc.dg/lto/attr-weakref-1_2.c (revision 0)
> --- testsuite/gcc.dg/lto/attr-weakref-1_2.c (revision 0)
> ***************
> *** 0 ****
> --- 1,7 ----
> + extern void callmefirst();
> + static void callmealias() __attribute__((weakref ("callmefirst")));
> +
> + c()
> + {
> + callmealias();
> + }
> Index: lto/lto-partition.c
> ===================================================================
> *** lto/lto-partition.c (revision 198824)
> --- lto/lto-partition.c (working copy)
> *************** get_symbol_class (symtab_node node)
> *** 59,64 ****
> --- 59,68 ----
> if (cnode && cnode->global.inlined_to)
> return SYMBOL_DUPLICATE;
>
> + /* Weakref aliases are always duplicated. */
> + if (lookup_attribute ("weakref", DECL_ATTRIBUTES (node->symbol.decl)))
> + return SYMBOL_DUPLICATE;
> +
> /* External declarations are external. */
> if (DECL_EXTERNAL (node->symbol.decl))
> return SYMBOL_EXTERNAL;
> *************** get_symbol_class (symtab_node node)
> *** 79,88 ****
> else if (!cgraph (node)->analyzed)
> return SYMBOL_EXTERNAL;
>
> - /* Weakref aliases are always duplicated. */
> - if (lookup_attribute ("weakref", DECL_ATTRIBUTES (node->symbol.decl)))
> - return SYMBOL_DUPLICATE;
> -
> /* Comdats are duplicated to every use unless they are keyed.
> Those do not need duplication. */
> if (DECL_COMDAT (node->symbol.decl)
> --- 83,88 ----
> *************** lto_balanced_map (void)
> *** 561,567 ****
>
> last_visited_node++;
>
> ! gcc_assert (node->analyzed);
>
> /* Compute boundary cost of callgraph edges. */
> for (edge = node->callees; edge; edge = edge->next_callee)
> --- 561,568 ----
>
> last_visited_node++;
>
> ! gcc_assert (node->analyzed
> ! || lookup_attribute ("weakref", DECL_ATTRIBUTES
> (node->symbol.decl)));
>
> /* Compute boundary cost of callgraph edges. */
> for (edge = node->callees; edge; edge = edge->next_callee)
> *************** privatize_symbol_name (symtab_node node)
> *** 768,774 ****
> {
> tree decl = node->symbol.decl;
> const char *name = IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (decl));
> - char *label;
>
> /* Our renaming machinery do not handle more than one change of assembler
> name.
> We should not need more than one anyway. */
> --- 769,774 ----
> *************** privatize_symbol_name (symtab_node node)
> *** 793,799 ****
> name);
> return;
> }
> - ASM_FORMAT_PRIVATE_NAME (label, name, DECL_UID (decl));
> change_decl_assembler_name (decl, clone_function_name (decl, "lto_priv"));
> if (node->symbol.lto_file_data)
> lto_record_renamed_decl (node->symbol.lto_file_data, name,
> --- 793,798 ----
> *************** rename_statics (lto_symtab_encoder_t enc
> *** 869,875 ****
> once this is fixed. */
> || DECL_EXTERNAL (node->symbol.decl)
> || !symtab_real_symbol_p (node))
> ! && !may_need_named_section_p (encoder, node))
> return;
>
> /* Now walk symbols sharing the same name and see if there are any
> conflicts.
> --- 868,875 ----
> once this is fixed. */
> || DECL_EXTERNAL (node->symbol.decl)
> || !symtab_real_symbol_p (node))
> ! && !may_need_named_section_p (encoder, node)
> ! && !lookup_attribute ("weakref", DECL_ATTRIBUTES
> (node->symbol.decl)))
> return;
>
> /* Now walk symbols sharing the same name and see if there are any
> conflicts.
> *************** rename_statics (lto_symtab_encoder_t enc
> *** 894,902 ****
> /* Assign every symbol in the set that shares the same ASM name an unique
> mangled name. */
> for (s = symtab_node_for_asm (name); s;)
> ! if (!s->symbol.externally_visible
> && ((symtab_real_symbol_p (s)
> ! && !DECL_EXTERNAL (node->symbol.decl)
> && !TREE_PUBLIC (node->symbol.decl))
> || may_need_named_section_p (encoder, s))
> && (!encoder
> --- 894,904 ----
> /* Assign every symbol in the set that shares the same ASM name an unique
> mangled name. */
> for (s = symtab_node_for_asm (name); s;)
> ! if ((!s->symbol.externally_visible
> ! || lookup_attribute ("weakref", DECL_ATTRIBUTES (node->symbol.decl)))
> && ((symtab_real_symbol_p (s)
> ! && (!DECL_EXTERNAL (node->symbol.decl)
> ! || lookup_attribute ("weakref", DECL_ATTRIBUTES
> (node->symbol.decl)))
> && !TREE_PUBLIC (node->symbol.decl))
> || may_need_named_section_p (encoder, s))
> && (!encoder
> Index: varpool.c
> ===================================================================
> *** varpool.c (revision 198824)
> --- varpool.c (working copy)
> *************** dump_varpool_node (FILE *f, struct varpo
> *** 86,91 ****
> --- 86,95 ----
> fprintf (f, " finalized");
> if (node->output)
> fprintf (f, " output");
> + if (TREE_READONLY (node->symbol.decl))
> + fprintf (f, " read-only");
> + if (const_value_known_p (node->symbol.decl))
> + fprintf (f, " const-value-known");
> fprintf (f, "\n");
> }
>
