| 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;
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). I simply added a loop that for type that looks anonymous by if (TYPE_STUB_DECL (t) && !TREE_PUBLIC (TYPE_STUB_DECL (t))) it walks up the context and looks for actual anonymous NAMESPACE_DECL. Now I however run into type merging issues as follows: for following type: ../../gcc/postreload-gcse.c:107:1: error: type �struct <anon>� violates one definition rule [-Werror=odr] { ^ ../../gcc/postreload.c:721:3: note: a different type is defined in another translation unit { ^ ../../gcc/postreload-gcse.c:108:7: note: the first difference of corresponding definitions is field �moves_inserted� int moves_inserted; ^ ../../gcc/postreload.c:722:51: note: a field with different name is defined in another translation unit struct reg_use reg_use[RELOAD_COMBINE_MAX_USES]; ^ (gdb) p debug_tree (t1->type_common.name->decl_with_vis.assembler_name) <identifier_node 0x7ffff5ecac08 5._134> So the problem is that at compile time we think the type is ODR type but it does not really have name: <record_type 0x7ffff233e348 BLK size <integer_cst 0x7ffff6cce138 type <integer_type 0x7ffff6adb150 bitsizetype> constant 96> unit size <integer_cst 0x7ffff6987a08 type <integer_type 0x7ffff6adb0a8 sizetype> constant 12> align 32 symtab 0 alias set -1 canonical type 0x7ffff46d2bd0 fields <field_decl 0x7ffff27178e8 moves_inserted type <integer_type 0x7ffff6adb690 int public SI size <integer_cst 0x7ffff6ad7df8 constant 32> unit size <integer_cst 0x7ffff6ad7e10 constant 4> align 32 symtab 0 alias set -1 canonical type 0x7ffff6adb690 precision 32 min <integer_cst 0x7ffff6ad7db0 -2147483648> max <integer_cst 0x7ffff6ad7dc8 2147483647> pointer_to_this <pointer_type 0x7ffff6af27e0> reference_to_this <reference_type 0x7ffff6148dc8>> nonlocal SI file ../../gcc/postreload-gcse.c line 108 col 7 size <integer_cst 0x7ffff6ad7df8 32> unit size <integer_cst 0x7ffff6ad7e10 4> align 32 offset_align 128 offset <integer_cst 0x7ffff6ad7be8 constant 0> bit offset <integer_cst 0x7ffff6ad7c30 constant 0> context <record_type 0x7ffff233e150> chain <field_decl 0x7ffff2717850 copies_inserted type <integer_type 0x7ffff6adb690 int> nonlocal SI file ../../gcc/postreload-gcse.c line 109 col 7 size <integer_cst 0x7ffff6ad7df8 32> unit size <integer_cst 0x7ffff6ad7e10 4> align 32 offset_align 128 offset <integer_cst 0x7ffff6ad7be8 0> bit offset <integer_cst 0x7ffff6ad7df8 32> context <record_type 0x7ffff233e150> chain <field_decl 0x7ffff27177b8 insns_deleted>>> context <translation_unit_decl 0x7ffff25c2f78 D.1019967> chain <type_decl 0x7ffff2717688 D.1023523>> <type_decl 0x7ffff2717688 D.1023523 type <record_type 0x7ffff233e150 BLK size <integer_cst 0x7ffff6cce138 constant 96> unit size <integer_cst 0x7ffff6987a08 constant 12> align 32 symtab 0 alias set -1 canonical type 0x7ffff46d2bd0 fields <field_decl 0x7ffff27178e8 moves_inserted type <integer_type 0x7ffff6adb690 int> nonlocal SI file ../../gcc/postreload-gcse.c line 108 col 7 size <integer_cst 0x7ffff6ad7df8 constant 32> unit size <integer_cst 0x7ffff6ad7e10 constant 4> align 32 offset_align 128 offset <integer_cst 0x7ffff6ad7be8 constant 0> bit offset <integer_cst 0x7ffff6ad7c30 constant 0> context <record_type 0x7ffff233e150> chain <field_decl 0x7ffff2717850 copies_inserted>> context <translation_unit_decl 0x7ffff25c2f78 D.1019967> pointer_to_this <pointer_type 0x7ffff233e3f0> chain <type_decl 0x7ffff2717688 D.1023523>> VOID file ../../gcc/postreload-gcse.c line 107 col 1 align 8 context <translation_unit_decl 0x7ffff25c2f78 D.1019967>> 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. (I also wonder we we need to introdce a type name "._134") and pass it all the way down to LTO. I tried to make type_with_linkage_p to return false on those, but that causes problem witht fact that polymorphic call analysis expects all class types to have linkage and working ODR equivalency on these. Is there a way to associate them with the real named type they correspond to and arrange them to have same name mangling? (and perhaps the mangler to ICE on attempt to try to use local name like this?) 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. Honza * ipa-devirt.c (type_in_anonymous_namespace_p): Return true or implicit declarations. (odr_type_p): Check that TYPE_NAME is TYPE_DECL before looking into it. (get_odr_type): Check type has linkage before adding bases. (register_odr_type): Check that type has linkage before adding it. (type_known_to_have_no_deriavations_p): Rename to .. (type_known_to_have_no_derivations_p): This one. * ipa-utils.h (type_known_to_have_no_deriavations_p): Rename to .. (type_known_to_have_no_derivations_p): This one. * ipa-polymorphic-call.c (ipa_polymorphic_call_context::restrict_to_inner_type): Check that type has linkage.