Message ID | 20150519173323.GA93618@kam.mff.cuni.cz |
---|---|
State | New |
Headers | show |
On 2015.05.19 at 19:33 +0200, Jan Hubicka wrote: > > Jason, > I just noticed that there are bogus ODR violation warnings during LTO-bootstrap > (that breaks -Werror builds). It was caused by my work-around for type_in_anonymous_namespace > for the issue discussed in: > https://gcc.gnu.org/ml/gcc-patches/2015-05/msg01245.html > (i.e. the TYPE_STUB_DECL disucssion). There are also many bogus ODR warnings when building LLVM with LTO: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66180
On 05/19/2015 01:33 PM, Jan Hubicka wrote: > I tracked down that those are implicit typedef created by create_implicit_typedef. > My patch made them no longer anonymous that in turn triggers the bogus diagnostics. > I do not think it is fully correct though - those types are not anonymous. Hmm? The types are anonymous: static struct { int moves_inserted; int copies_inserted; int insns_deleted; } stats; Here there is a variable named 'stats', but its type has no name. > (I also wonder we we need to introdce a type name "._134") and pass it all the way down > to LTO. Anonymous types do need to have some name, so that we can mangle them. But I don't know if they need to remain past free_lang_data. Jason
> On 05/19/2015 01:33 PM, Jan Hubicka wrote: > >I tracked down that those are implicit typedef created by create_implicit_typedef. > >My patch made them no longer anonymous that in turn triggers the bogus diagnostics. > >I do not think it is fully correct though - those types are not anonymous. > > Hmm? The types are anonymous: > > static struct > { > int moves_inserted; > int copies_inserted; > int insns_deleted; > } stats; > > Here there is a variable named 'stats', but its type has no name. Ah, sorry. I misread the declaration and thought it produce type stats. I suppose this cost me an afternoon yesterday :) Indeed this is anonymous type. I see it is anonymous even though it is not in any namespace, so it makes sense that I needed to make an exception to my hack looking for explicit namespace in the DECL_CONTEXT. > > >(I also wonder we we need to introdce a type name "._134") and pass it all the way down > >to LTO. > > Anonymous types do need to have some name, so that we can mangle > them. But I don't know if they need to remain past free_lang_data. I think they can be killed there, as a minor optimization. I will look into it. Thanks for the explanation. Honza > > Jason
> I bootstrapped/regtested on x86_64-linux the patch bellow. If it will work > for Firefox and Chrome I will go ahead with it at least temporarily. Really? This introduced a LTO failure in the gnat.dg testsuite: FAIL: gnat.dg/lto8.adb (internal compiler error) FAIL: gnat.dg/lto8.adb (test for excess errors) WARNING: gnat.dg/lto8.adb compilation failed to produce executable lto1: internal compiler error: in odr_types_equivalent_p, at ipa-devirt.c:1276 0x86a263 odr_types_equivalent_p /home/eric/svn/gcc/gcc/ipa-devirt.c:1276 0x86bf44 odr_types_equivalent_p(tree_node*, tree_node*) /home/eric/svn/gcc/gcc/ipa-devirt.c:1718 0x5c563a warn_type_compatibility_p /home/eric/svn/gcc/gcc/lto/lto-symtab.c:219 0x5c6103 lto_symtab_merge /home/eric/svn/gcc/gcc/lto/lto-symtab.c:336 0x5c6103 lto_symtab_merge_decls_2 /home/eric/svn/gcc/gcc/lto/lto-symtab.c:520 0x5c6103 lto_symtab_merge_decls_1 /home/eric/svn/gcc/gcc/lto/lto-symtab.c:671 0x5c6103 lto_symtab_merge_decls() /home/eric/svn/gcc/gcc/lto/lto-symtab.c:694 0x5bb9cc read_cgraph_and_symbols /home/eric/svn/gcc/gcc/lto/lto.c:2891 0x5bb9cc lto_main() /home/eric/svn/gcc/gcc/lto/lto.c:3277
> > I bootstrapped/regtested on x86_64-linux the patch bellow. If it will work > > for Firefox and Chrome I will go ahead with it at least temporarily. > > Really? This introduced a LTO failure in the gnat.dg testsuite: > > FAIL: gnat.dg/lto8.adb (internal compiler error) > FAIL: gnat.dg/lto8.adb (test for excess errors) > WARNING: gnat.dg/lto8.adb compilation failed to produce executable > > lto1: internal compiler error: in odr_types_equivalent_p, at ipa-devirt.c:1276 > 0x86a263 odr_types_equivalent_p > /home/eric/svn/gcc/gcc/ipa-devirt.c:1276 > 0x86bf44 odr_types_equivalent_p(tree_node*, tree_node*) > /home/eric/svn/gcc/gcc/ipa-devirt.c:1718 > 0x5c563a warn_type_compatibility_p > /home/eric/svn/gcc/gcc/lto/lto-symtab.c:219 Hmm, ICE here means that we think the global symbols are defined by a type in anonymous namespace. We really need to solve the problem of reliably identifying these https://gcc.gnu.org/ml/gcc-patches/2015-05/msg01245.html It is not at all that hard to do it, we just need to decide on the representation. I will take a look if I can improve type_in_anonymous_namepsace somehow. So Ada produces TYPE_DECL with DECL_ABSTRACT that do have TYPE_STUB_DECL with TREE_PUBLIC NULL I suppose. Honza > 0x5c6103 lto_symtab_merge > /home/eric/svn/gcc/gcc/lto/lto-symtab.c:336 > 0x5c6103 lto_symtab_merge_decls_2 > /home/eric/svn/gcc/gcc/lto/lto-symtab.c:520 > 0x5c6103 lto_symtab_merge_decls_1 > /home/eric/svn/gcc/gcc/lto/lto-symtab.c:671 > 0x5c6103 lto_symtab_merge_decls() > /home/eric/svn/gcc/gcc/lto/lto-symtab.c:694 > 0x5bb9cc read_cgraph_and_symbols > /home/eric/svn/gcc/gcc/lto/lto.c:2891 > 0x5bb9cc lto_main() > /home/eric/svn/gcc/gcc/lto/lto.c:3277 > > -- > Eric Botcazou
> I will take a look if I can improve type_in_anonymous_namepsace somehow. So > Ada produces TYPE_DECL with DECL_ABSTRACT that do have TYPE_STUB_DECL with > TREE_PUBLIC NULL I suppose. Do you mean DECL_ARTIFICIAL instead of DECL_ABSTRACT? If so, presumably, yes, why wouldn't it do that? That seems the natural description of an artificial private type with file scope. And I don't really understand why DECL_ARTIFICIAL is allowed to make such a difference in semantics here. That looks dangerous to me. Note that the problematic assertions: gcc_assert (!type_with_linkage_p (t1) || !type_in_anonymous_namespace_p (t1)); gcc_assert (!type_with_linkage_p (t2) || !type_in_anonymous_namespace_p (t2)); make the following code unreachable: if ((type_with_linkage_p (t1) && type_in_anonymous_namespace_p (t1)) || (type_with_linkage_p (t2) && type_in_anonymous_namespace_p (t2))) { /* We can not trip this when comparing ODR types, only when trying to match different ODR derivations from different declarations. So WARN should be always false. */ gcc_assert (!warn); return false; }
Index: ipa-devirt.c =================================================================== --- ipa-devirt.c (revision 223390) +++ ipa-devirt.c (working copy) @@ -269,6 +269,8 @@ type_in_anonymous_namespace_p (const_tre if (TYPE_STUB_DECL (t) && !TREE_PUBLIC (TYPE_STUB_DECL (t))) { + if (DECL_ARTIFICIAL (TYPE_NAME (t))) + return true; tree ctx = DECL_CONTEXT (TYPE_NAME (t)); while (ctx) { @@ -296,7 +298,7 @@ odr_type_p (const_tree t) to care, since it is used only for type merging. */ gcc_checking_assert (in_lto_p || flag_lto); - return (TYPE_NAME (t) + return (TYPE_NAME (t) && TREE_CODE (TYPE_NAME (t)) == TYPE_DECL && (DECL_ASSEMBLER_NAME_SET_P (TYPE_NAME (t)))); } @@ -2124,6 +2126,7 @@ get_odr_type (tree type, bool insert) } if (build_bases && TREE_CODE (type) == RECORD_TYPE && TYPE_BINFO (type) + && type_with_linkage_p (type) && type == TYPE_MAIN_VARIANT (type)) { tree binfo = TYPE_BINFO (type); @@ -2183,7 +2186,8 @@ register_odr_type (tree type) makes it possible that non-ODR type is main_odr_variant of ODR type. Things may get smoother if LTO FE set mangled name of those types same way as C++ FE does. */ - if (odr_type_p (main_odr_variant (TYPE_MAIN_VARIANT (type)))) + if (odr_type_p (main_odr_variant (TYPE_MAIN_VARIANT (type))) + && odr_type_p (TYPE_MAIN_VARIANT (type))) get_odr_type (TYPE_MAIN_VARIANT (type), true); if (TYPE_MAIN_VARIANT (type) != type && odr_type_p (main_odr_variant (type))) get_odr_type (type, true); @@ -2192,7 +2196,7 @@ register_odr_type (tree type) /* Return true if type is known to have no derivations. */ bool -type_known_to_have_no_deriavations_p (tree t) +type_known_to_have_no_derivations_p (tree t) { return (type_all_derivations_known_p (t) && (TYPE_FINAL_P (t) Index: ipa-utils.h =================================================================== --- ipa-utils.h (revision 223390) +++ ipa-utils.h (working copy) @@ -80,7 +80,7 @@ bool vtable_pointer_value_to_vtable (con tree subbinfo_with_vtable_at_offset (tree, unsigned HOST_WIDE_INT, tree); void compare_virtual_tables (varpool_node *, varpool_node *); bool type_all_derivations_known_p (const_tree); -bool type_known_to_have_no_deriavations_p (tree); +bool type_known_to_have_no_derivations_p (tree); bool contains_polymorphic_type_p (const_tree); void register_odr_type (tree); bool types_must_be_same_for_odr (tree, tree); Index: ipa-polymorphic-call.c =================================================================== --- ipa-polymorphic-call.c (revision 223390) +++ ipa-polymorphic-call.c (working copy) @@ -269,7 +269,8 @@ ipa_polymorphic_call_context::restrict_t types. Testing it here may help us to avoid speculation. */ if (otr_type && TREE_CODE (outer_type) == RECORD_TYPE && (!in_lto_p || odr_type_p (outer_type)) - && type_known_to_have_no_deriavations_p (outer_type)) + && type_with_linkage_p (outer_type) + && type_known_to_have_no_derivations_p (outer_type)) maybe_derived_type = false; /* Type can not contain itself on an non-zero offset. In that case @@ -393,7 +394,7 @@ ipa_polymorphic_call_context::restrict_t goto no_useful_type_info; cur_offset = new_offset; - type = subtype; + type = TYPE_MAIN_VARIANT (subtype); if (!speculative) { outer_type = type;