* False ODR violation positives on anonymous namespace types @ 2015-05-11 14:28 Jan Hubicka 2015-05-11 17:16 ` Jason Merrill 0 siblings, 1 reply; 13+ messages in thread From: Jan Hubicka @ 2015-05-11 14:28 UTC (permalink / raw) To: gcc-patches, jason Jason, I got my firefox tree building again and noticed that my patch to enable ODR merging on non-class types caused false positives: /aux/hubicka/trunk-install/include/c++/6.0.0/ext/new_allocator.h:66:26: warning: type â(anonymous namespace)::ObservationWithStack const&â violates one definition rule [-Wodr] typedef const _Tp& const_reference; ^ /aux/hubicka/trunk-install/include/c++/6.0.0/ext/alloc_traits.h:110:53: note: it is defined as a pointer to different type in another translation unit typedef const value_type& const_reference; ^ /aux/hubicka/firefox8/xpcom/build/MainThreadIOLogger.cpp:28:8: note: type âconst struct ObservationWithStackâ should match type âconst struct value_typeâ struct ObservationWithStack ^ /aux/hubicka/trunk-install/include/c++/6.0.0/ext/alloc_traits.h:103:53: note: the incompatible type is defined here typedef typename _Base_type::value_type value_type; ^ Here the obvious problem is that we try to merge type in anonymous namespace. This is because at LTO time odr_type_p returns true and type_in_anonymous_namespace returns false. odr_type_p basically checks that we computed mangled type name for it which is done in free_lang_data as follows: { /* We use DECL_ASSEMBLER_NAME to hold mangled type names for One Definition Rule merging. */ if (flag_lto_odr_type_mering && TREE_CODE (decl) == TYPE_DECL && DECL_NAME (decl) && decl == TYPE_NAME (TREE_TYPE (decl)) && !is_lang_specific (TREE_TYPE (decl)) /* Save some work. Names of builtin types are always derived from properties of its main variant. A special case are integer types where mangling do make differences between char/signed char/unsigned char etc. Storing name for these makes e.g. -fno-signed-char/-fsigned-char mismatches to be handled well. See cp/mangle.c:write_builtin_type for details. */ && (TREE_CODE (TREE_TYPE (decl)) != VOID_TYPE && TREE_CODE (TREE_TYPE (decl)) != BOOLEAN_TYPE && TREE_CODE (TREE_TYPE (decl)) != REAL_TYPE && TREE_CODE (TREE_TYPE (decl)) != FIXED_POINT_TYPE) && !TYPE_ARTIFICIAL (TREE_TYPE (decl)) && !variably_modified_type_p (TREE_TYPE (decl), NULL_TREE) && !type_in_anonymous_namespace_p (TREE_TYPE (decl))) return !DECL_ASSEMBLER_NAME_SET_P (decl); and obviously we are not intended to mangle tyhis type and it should be caught by type_in_anonymous_namespace_p check: bool type_in_anonymous_namespace_p (const_tree t) { /* TREE_PUBLIC of TYPE_STUB_DECL may not be properly set for bulitin types; those have CONTEXT NULL. */ if (!TYPE_CONTEXT (t)) return false; return (TYPE_STUB_DECL (t) && !TREE_PUBLIC (TYPE_STUB_DECL (t))); } We already discussed earlier that type_in_anonymous_namespace_p is not working on compund types, because these do not have TYPE_STUB_DECL. I tought those are !TYPE_NAME types. What is reason for !TYPE_NAME type with no TYPE_STUB_DECL? Is it always a compound type with typedef name? One possible solution is to avoid producing mangled names so these won't be odr_type_p at LTO time: Index: tree.c =================================================================== --- tree.c (revision 222991) +++ tree.c (working copy) @@ -5173,6 +5173,7 @@ need_assembler_name_p (tree decl) && TREE_CODE (TREE_TYPE (decl)) != REAL_TYPE && TREE_CODE (TREE_TYPE (decl)) != FIXED_POINT_TYPE) && !TYPE_ARTIFICIAL (TREE_TYPE (decl)) + && TYPE_STUB_DECL (TREE_TYPE (decl)) && !variably_modified_type_p (TREE_TYPE (decl), NULL_TREE) && !type_in_anonymous_namespace_p (TREE_TYPE (decl))) return !DECL_ASSEMBLER_NAME_SET_P (decl); which silence the warnings, but I think it also may cause false negatives. Ohter option is to make type_in_anonymous_namespace_p work. The types in question are anonymous because they are defined within anonymous template, would the following work in type_in_anonymous_namespace_p? + /* Types (such as pointer_type) defined within class types may not + have their own TYPE_STUB_DECL. Look for the outer type. */ + while (!TYPE_STUB_DECL (t) && TYPE_NAME (t) + && TREE_CODE (TYPE_NAME (t)) == TYPE_DECL + && DECL_CONTEXT (TYPE_NAME (t)) + && TYPE_P (DECL_CONTEXT (TYPE_NAME (t)))) + t = DECL_CONTEXT (TYPE_NAME (t)); Last option would be make anonymous_namespace_p to walk to compound type bases. I made variant of this in https://gcc.gnu.org/ml/gcc-patches/2015-05/msg00880.html (odr_or_derived_type_p, modifying type_in_anonymous_namespace_p to work same way is definitly easy) What would be a preffered fix for this? Honza ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: False ODR violation positives on anonymous namespace types 2015-05-11 14:28 False ODR violation positives on anonymous namespace types Jan Hubicka @ 2015-05-11 17:16 ` Jason Merrill 2015-05-11 17:46 ` Jan Hubicka 0 siblings, 1 reply; 13+ messages in thread From: Jason Merrill @ 2015-05-11 17:16 UTC (permalink / raw) To: Jan Hubicka, gcc-patches On 05/11/2015 09:28 AM, Jan Hubicka wrote: > We already discussed earlier that type_in_anonymous_namespace_p is not working > on compund types, because these do not have TYPE_STUB_DECL. I thought those are > !TYPE_NAME types. What is reason for !TYPE_NAME type with no TYPE_STUB_DECL? > Is it always a compound type with typedef name? Right. Typedef names have no linkage, so they aren't really ODR types; only classes and enums have linkage. Why do you want to check other types? Jason ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: False ODR violation positives on anonymous namespace types 2015-05-11 17:16 ` Jason Merrill @ 2015-05-11 17:46 ` Jan Hubicka 2015-05-11 17:54 ` Jason Merrill 0 siblings, 1 reply; 13+ messages in thread From: Jan Hubicka @ 2015-05-11 17:46 UTC (permalink / raw) To: Jason Merrill; +Cc: Jan Hubicka, gcc-patches > On 05/11/2015 09:28 AM, Jan Hubicka wrote: > >We already discussed earlier that type_in_anonymous_namespace_p is not working > >on compund types, because these do not have TYPE_STUB_DECL. I thought those are > >!TYPE_NAME types. What is reason for !TYPE_NAME type with no TYPE_STUB_DECL? > >Is it always a compound type with typedef name? > > Right. Typedef names have no linkage, so they aren't really ODR > types; only classes and enums have linkage. Why do you want to > check other types? Well, my main motivatoin to extend from RECORD_OR_UNION_TYPE_P was to handle enums. But other case I would like to deal with are integer types - i.e. preserve difference between char/signed char/unsigned char/short/int/long/wchar in cases where they structurally coincide. This makes us to output better diagnostics on ODR violations caused by -fsigned-char mismatches, but it also has some issues with fact that LTO forcingly merge char_type_node no matter whetehr it agrees or disagress across units by preloading https://gcc.gnu.org/ml/gcc-patches/2015-04/msg01538.html I can go with RECORD_OR_UNION_TYPE_P (..) || TREE_CODE (type) == ENUMERAL_TYPE with comment that those types have linkage and other not, but we lose some checks. Honza > > Jason ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: False ODR violation positives on anonymous namespace types 2015-05-11 17:46 ` Jan Hubicka @ 2015-05-11 17:54 ` Jason Merrill 2015-05-11 18:05 ` Jan Hubicka 0 siblings, 1 reply; 13+ messages in thread From: Jason Merrill @ 2015-05-11 17:54 UTC (permalink / raw) To: Jan Hubicka; +Cc: gcc-patches On 05/11/2015 12:46 PM, Jan Hubicka wrote: > Well, my main motivatoin to extend from RECORD_OR_UNION_TYPE_P was to handle > enums. But other case I would like to deal with are integer types - i.e. preserve > difference between char/signed char/unsigned char/short/int/long/wchar in cases > where they structurally coincide. In what context? Won't you get that from comparing e.g. the field types of two definitions of the same class? Jason ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: False ODR violation positives on anonymous namespace types 2015-05-11 17:54 ` Jason Merrill @ 2015-05-11 18:05 ` Jan Hubicka 2015-05-11 21:21 ` Jason Merrill 0 siblings, 1 reply; 13+ messages in thread From: Jan Hubicka @ 2015-05-11 18:05 UTC (permalink / raw) To: Jason Merrill; +Cc: Jan Hubicka, gcc-patches > On 05/11/2015 12:46 PM, Jan Hubicka wrote: > >Well, my main motivatoin to extend from RECORD_OR_UNION_TYPE_P was to handle > >enums. But other case I would like to deal with are integer types - i.e. preserve > >difference between char/signed char/unsigned char/short/int/long/wchar in cases > >where they structurally coincide. > > In what context? Won't you get that from comparing e.g. the field > types of two definitions of the same class? If one class define "int foo;" and other "long foo;" we currently do not complain about ODR on 32bit targets while I think we could. Other case was the ODR violations dragged by the signed/unsigned: char switch: $ cat t.C char a; $ ./xgcc -B ./ -O2 t.C -o t1.o -fno-signed-char -c -flto $ ./xgcc -B ./ -O2 t.C -o t2.o -fsigned-char -c -flto $ ./xgcc -B ./ -O2 t1.o t2.o -flto -fno-signed-char -flto <built-in>: warning: type ïcharï violates one definition rule [-Wodr] <built-in>: note: a type with different signedness is defined in another translation unit t.C:1:6: warning: type of ïaï does not match original declaration char a; ^ t.C:1:6: note: previously declared here char a; ^ I also want to use ODR for more fine grained TBAA on LTO and there the differences between integer types matter more. Honza > > Jason ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: False ODR violation positives on anonymous namespace types 2015-05-11 18:05 ` Jan Hubicka @ 2015-05-11 21:21 ` Jason Merrill 2015-05-11 21:52 ` Jan Hubicka 0 siblings, 1 reply; 13+ messages in thread From: Jason Merrill @ 2015-05-11 21:21 UTC (permalink / raw) To: Jan Hubicka; +Cc: gcc-patches On 05/11/2015 01:05 PM, Jan Hubicka wrote: >> On 05/11/2015 12:46 PM, Jan Hubicka wrote: >>> Well, my main motivatoin to extend from RECORD_OR_UNION_TYPE_P was to handle >>> enums. But other case I would like to deal with are integer types - i.e. preserve >>> difference between char/signed char/unsigned char/short/int/long/wchar in cases >>> where they structurally coincide. >> >> In what context? Won't you get that from comparing e.g. the field >> types of two definitions of the same class? > > If one class define "int foo;" and other "long foo;" we currently do not complain > about ODR on 32bit targets while I think we could. We certainly should. But that's a problem because foo is subject to the ODR. I don't see why you need to treat int as an ODR type to get checking for foo. Jason ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: False ODR violation positives on anonymous namespace types 2015-05-11 21:21 ` Jason Merrill @ 2015-05-11 21:52 ` Jan Hubicka 2015-05-12 13:51 ` Jason Merrill 0 siblings, 1 reply; 13+ messages in thread From: Jan Hubicka @ 2015-05-11 21:52 UTC (permalink / raw) To: Jason Merrill; +Cc: Jan Hubicka, gcc-patches > On 05/11/2015 01:05 PM, Jan Hubicka wrote: > >>On 05/11/2015 12:46 PM, Jan Hubicka wrote: > >>>Well, my main motivatoin to extend from RECORD_OR_UNION_TYPE_P was to handle > >>>enums. But other case I would like to deal with are integer types - i.e. preserve > >>>difference between char/signed char/unsigned char/short/int/long/wchar in cases > >>>where they structurally coincide. > >> > >>In what context? Won't you get that from comparing e.g. the field > >>types of two definitions of the same class? > > > >If one class define "int foo;" and other "long foo;" we currently do not complain > >about ODR on 32bit targets while I think we could. > > We certainly should. But that's a problem because foo is subject to > the ODR. I don't see why you need to treat int as an ODR type to > get checking for foo. What happens in LTO is that lto-symtab decide to merge the two declarations of foo. At this time it has chance to output the warning. For that it needs to be able to work out that these declarations are having different types, but because LTO merge canonical types on structural basis, types_compatible_p (long, int) will return true. The idea behind LTO canonical type computation is that it needs to safely work cross-language and if the two declarations came from C and fortran, the mismatch in type name is useless (see gimple_canonical_types_compatible_p) So what I want is to have odr_or_derived_type_p to return true on those types (because it sees they do have mangled name attached) and then call odr_types_equivalent_p which is the same comparer I use to output ODR waring about types and it will complain about mangled type names being different. This is already implemented in https://gcc.gnu.org/ml/gcc-patches/2015-05/msg00870.html Honza > > Jason ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: False ODR violation positives on anonymous namespace types 2015-05-11 21:52 ` Jan Hubicka @ 2015-05-12 13:51 ` Jason Merrill 2015-05-12 14:13 ` Jan Hubicka 0 siblings, 1 reply; 13+ messages in thread From: Jason Merrill @ 2015-05-12 13:51 UTC (permalink / raw) To: Jan Hubicka; +Cc: gcc-patches On 05/11/2015 04:39 PM, Jan Hubicka wrote: > What happens in LTO is that lto-symtab decide to merge the two declarations of > foo. At this time it has chance to output the warning. For that it needs to > be able to work out that these declarations are having different types, but > because LTO merge canonical types on structural basis, types_compatible_p > (long, int) will return true. OK. So I guess it makes sense to check built-in integer types as well, but compound types should be compared structurally. Jason ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: False ODR violation positives on anonymous namespace types 2015-05-12 13:51 ` Jason Merrill @ 2015-05-12 14:13 ` Jan Hubicka 2015-05-12 14:54 ` Jan Hubicka 0 siblings, 1 reply; 13+ messages in thread From: Jan Hubicka @ 2015-05-12 14:13 UTC (permalink / raw) To: Jason Merrill; +Cc: Jan Hubicka, gcc-patches > On 05/11/2015 04:39 PM, Jan Hubicka wrote: > >What happens in LTO is that lto-symtab decide to merge the two declarations of > >foo. At this time it has chance to output the warning. For that it needs to > >be able to work out that these declarations are having different types, but > >because LTO merge canonical types on structural basis, types_compatible_p > >(long, int) will return true. > > OK. So I guess it makes sense to check built-in integer types as > well, but compound types should be compared structurally. Yes, agreed. if (flag_lto_odr_type_mering && TREE_CODE (decl) == TYPE_DECL && DECL_NAME (decl) && decl == TYPE_NAME (TREE_TYPE (decl)) && !is_lang_specific (TREE_TYPE (decl)) /* Save some work. Names of builtin types are always derived from properties of its main variant. A special case are integer types where mangling do make differences between char/signed char/unsigned char etc. Storing name for these makes e.g. -fno-signed-char/-fsigned-char mismatches to be handled well. See cp/mangle.c:write_builtin_type for details. */ && (TREE_CODE (TREE_TYPE (decl)) != VOID_TYPE && TREE_CODE (TREE_TYPE (decl)) != BOOLEAN_TYPE && TREE_CODE (TREE_TYPE (decl)) != REAL_TYPE && TREE_CODE (TREE_TYPE (decl)) != FIXED_POINT_TYPE) && !TYPE_ARTIFICIAL (TREE_TYPE (decl)) && !variably_modified_type_p (TREE_TYPE (decl), NULL_TREE) && !type_in_anonymous_namespace_p (TREE_TYPE (decl))) I probably want to check /* Record, union and enumeration type have linkage that allows use to check type_in_anonymous_namespace_p. Compound types can be always compared structurally. To save some work we compare builtin types properties of its main variant. A special case are integer types where mangling do make differences between char/signed char/unsigned char etc. Storing name for these makes e.g. -fno-signed-char/-fsigned-char mismatches to be handled well. See cp/mangle.c:write_builtin_type for details. */ && ((RECORD_OR_UNION_TYPE_P (TREE_TYPE (decl)) || TREE_CODE (TREE_TYPE (decl)) == ENUMERATION_TIME && !type_in_anonymous_namespace_p (TREE_TYPE (decl))) || TREE_CODE (TREE_TYPE (decl)) == INTEGER_TYPE) I will test this. Thank you! Honza > > Jason ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: False ODR violation positives on anonymous namespace types 2015-05-12 14:13 ` Jan Hubicka @ 2015-05-12 14:54 ` Jan Hubicka 2015-05-12 16:07 ` Jason Merrill 0 siblings, 1 reply; 13+ messages in thread From: Jan Hubicka @ 2015-05-12 14:54 UTC (permalink / raw) To: Jan Hubicka; +Cc: Jason Merrill, gcc-patches > /* Record, union and enumeration type have linkage that allows use > to check type_in_anonymous_namespace_p. Compound types > can be always compared structurally. > To save some work we compare builtin types properties of its main > variant. A special case are integer types where mangling do make > differences between char/signed char/unsigned char etc. Storing name > for these makes e.g. -fno-signed-char/-fsigned-char mismatches to be > handled well. > > See cp/mangle.c:write_builtin_type for details. */ > && ((RECORD_OR_UNION_TYPE_P (TREE_TYPE (decl)) > || TREE_CODE (TREE_TYPE (decl)) == ENUMERATION_TIME > && !type_in_anonymous_namespace_p (TREE_TYPE (decl))) > || TREE_CODE (TREE_TYPE (decl)) == INTEGER_TYPE) I had quite some typos in the conditional. Here is variant of patch I am testing. I added predicate type_with_linkage_p to ipa-devirt and made type_in_anonymous_namespace_p to ICE on anything else - it is very easy to run into a trap with false negatives by calling it on something that does not have linkage defined. This is patch I am testing. Honza Index: tree.c =================================================================== --- tree.c (revision 223072) +++ tree.c (working copy) @@ -5160,27 +5160,33 @@ free_lang_data_in_type (tree type) static inline bool need_assembler_name_p (tree decl) { - /* We use DECL_ASSEMBLER_NAME to hold mangled type names for One Definition Rule - merging. */ + /* We use DECL_ASSEMBLER_NAME to hold mangled type names for One Definition + Rule merging. This makes type_odr_p to return true on those types during + LTO and by comparing the mangled name, we can say what types are intended + to be equivalent across compilation unit. + + We do not store names of type_in_anonymous_namespace_p. + + Record, union and enumeration type have linkage that allows use + to check type_in_anonymous_namespace_p. We do not mangle compound types + that always can be compared structurally. + + Similarly for builtin types, we compare properties of their main variant. + A special case are integer types where mangling do make differences + between char/signed char/unsigned char etc. Storing name for these makes + e.g. -fno-signed-char/-fsigned-char mismatches to be handled well. + See cp/mangle.c:write_builtin_type for details. */ + if (flag_lto_odr_type_mering && TREE_CODE (decl) == TYPE_DECL && DECL_NAME (decl) && decl == TYPE_NAME (TREE_TYPE (decl)) - && !is_lang_specific (TREE_TYPE (decl)) - /* Save some work. Names of builtin types are always derived from - properties of its main variant. A special case are integer types - where mangling do make differences between char/signed char/unsigned - char etc. Storing name for these makes e.g. - -fno-signed-char/-fsigned-char mismatches to be handled well. - - See cp/mangle.c:write_builtin_type for details. */ - && (TREE_CODE (TREE_TYPE (decl)) != VOID_TYPE - && TREE_CODE (TREE_TYPE (decl)) != BOOLEAN_TYPE - && TREE_CODE (TREE_TYPE (decl)) != REAL_TYPE - && TREE_CODE (TREE_TYPE (decl)) != FIXED_POINT_TYPE) && !TYPE_ARTIFICIAL (TREE_TYPE (decl)) - && !variably_modified_type_p (TREE_TYPE (decl), NULL_TREE) - && !type_in_anonymous_namespace_p (TREE_TYPE (decl))) + && (((RECORD_OR_UNION_TYPE_P (TREE_TYPE (decl)) + || TREE_CODE (TREE_TYPE (decl)) == ENUMERAL_TYPE) + && !type_in_anonymous_namespace_p (TREE_TYPE (decl))) + || TREE_CODE (TREE_TYPE (decl)) == INTEGER_TYPE) + && !variably_modified_type_p (TREE_TYPE (decl), NULL_TREE)) return !DECL_ASSEMBLER_NAME_SET_P (decl); /* Only FUNCTION_DECLs and VAR_DECLs are considered. */ if (TREE_CODE (decl) != FUNCTION_DECL @@ -12037,18 +12043,6 @@ obj_type_ref_class (tree ref) return TREE_TYPE (ref); } -/* Return true if T is in anonymous namespace. */ - -bool -type_in_anonymous_namespace_p (const_tree t) -{ - /* TREE_PUBLIC of TYPE_STUB_DECL may not be properly set for - bulitin types; those have CONTEXT NULL. */ - if (!TYPE_CONTEXT (t)) - return false; - return (TYPE_STUB_DECL (t) && !TREE_PUBLIC (TYPE_STUB_DECL (t))); -} - /* Lookup sub-BINFO of BINFO of TYPE at offset POS. */ static tree Index: ipa-utils.h =================================================================== --- ipa-utils.h (revision 223072) +++ ipa-utils.h (working copy) @@ -63,6 +63,7 @@ possible_polymorphic_call_targets (tree, void **cache_token = NULL, bool speuclative = false); odr_type get_odr_type (tree, bool insert = false); +bool odr_type_p (const_tree t); bool possible_polymorphic_call_target_p (tree ref, gimple stmt, struct cgraph_node *n); void dump_possible_polymorphic_call_targets (FILE *, tree, HOST_WIDE_INT, const ipa_polymorphic_call_context &); @@ -153,23 +157,6 @@ possible_polymorphic_call_target_p (stru context, n); } -/* Return true of T is type with One Definition Rule info attached. - It means that either it is anonymous type or it has assembler name - set. */ - -static inline bool -odr_type_p (const_tree t) -{ - if (type_in_anonymous_namespace_p (t)) - return true; - /* We do not have this information when not in LTO, but we do not need - to care, since it is used only for type merging. */ - gcc_assert (in_lto_p || flag_lto); - - return (TYPE_NAME (t) - && (DECL_ASSEMBLER_NAME_SET_P (TYPE_NAME (t)))); -} - /* Return true if BINFO corresponds to a type with virtual methods. Every type has several BINFOs. One is the BINFO associated by the type Index: ipa-devirt.c =================================================================== --- ipa-devirt.c (revision 223072) +++ ipa-devirt.c (working copy) @@ -245,6 +245,47 @@ struct GTY(()) odr_type_d bool rtti_broken; }; +/* Return true if T is a type with linkage defined. */ + +static bool +type_with_linkage_p (const_tree t) +{ + return (RECORD_OR_UNION_TYPE_P (t) + || TREE_CODE (t) == ENUMERAL_TYPE); +} + +/* Return true if T is in anonymous namespace. + This works only on those C++ types with linkage defined. */ + +bool +type_in_anonymous_namespace_p (const_tree t) +{ + gcc_assert (type_with_linkage_p (t)); + /* TREE_PUBLIC of TYPE_STUB_DECL may not be properly set for + backend produced types (such as va_arg_type); those have CONTEXT NULL + and never are considered anonymoius. */ + if (!TYPE_CONTEXT (t)) + return false; + return (TYPE_STUB_DECL (t) && !TREE_PUBLIC (TYPE_STUB_DECL (t))); +} + +/* Return true of T is type with One Definition Rule info attached. + It means that either it is anonymous type or it has assembler name + set. */ + +bool +odr_type_p (const_tree t) +{ + if (type_with_linkage_p (t) && type_in_anonymous_namespace_p (t)) + return true; + /* We do not have this information when not in LTO, but we do not need + to care, since it is used only for type merging. */ + gcc_checking_assert (in_lto_p || flag_lto); + + return (TYPE_NAME (t) + && (DECL_ASSEMBLER_NAME_SET_P (TYPE_NAME (t)))); +} + /* Return TRUE if all derived types of T are known and thus we may consider the walk of derived type complete. @@ -341,8 +382,7 @@ main_odr_variant (const_tree t) static bool can_be_name_hashed_p (tree t) { - return (!in_lto_p || type_in_anonymous_namespace_p (t) - || (TYPE_NAME (t) && DECL_ASSEMBLER_NAME_SET_P (TYPE_NAME (t)))); + return (!in_lto_p || odr_type_p (t)); } /* Hash type by its ODR name. */ @@ -358,7 +398,7 @@ hash_odr_name (const_tree t) return htab_hash_pointer (t); /* Anonymous types are unique. */ - if (type_in_anonymous_namespace_p (t)) + if (type_with_linkage_p (t) && type_in_anonymous_namespace_p (t)) return htab_hash_pointer (t); gcc_checking_assert (TYPE_NAME (t) @@ -381,7 +421,7 @@ can_be_vtable_hashed_p (tree t) if (TYPE_MAIN_VARIANT (t) != t) return false; /* Anonymous namespace types are always handled by name hash. */ - if (type_in_anonymous_namespace_p (t)) + if (type_with_linkage_p (t) && type_in_anonymous_namespace_p (t)) return false; return (TREE_CODE (t) == RECORD_TYPE && TYPE_BINFO (t) && BINFO_VTABLE (TYPE_BINFO (t))); @@ -455,8 +495,8 @@ types_same_for_odr (const_tree type1, co /* Check for anonymous namespaces. Those have !TREE_PUBLIC on the corresponding TYPE_STUB_DECL. */ - if (type_in_anonymous_namespace_p (type1) - || type_in_anonymous_namespace_p (type2)) + if ((type_with_linkage_p (type1) && type_in_anonymous_namespace_p (type1)) + || (type_with_linkage_p (type2) && type_in_anonymous_namespace_p (type2))) return false; @@ -565,8 +658,8 @@ odr_name_hasher::equal (const odr_type_d return false; /* Check for anonymous namespaces. Those have !TREE_PUBLIC on the corresponding TYPE_STUB_DECL. */ - if (type_in_anonymous_namespace_p (t1) - || type_in_anonymous_namespace_p (t2)) + if ((type_with_linkage_p (t1) && type_in_anonymous_namespace_p (t1)) + || (type_with_linkage_p (t2) && type_in_anonymous_namespace_p (t2))) return false; gcc_checking_assert (DECL_ASSEMBLER_NAME (TYPE_NAME (t1))); gcc_checking_assert (DECL_ASSEMBLER_NAME (TYPE_NAME (t2))); @@ -642,7 +735,6 @@ static bool odr_subtypes_equivalent_p (tree t1, tree t2, hash_set<type_pair,pair_traits> *visited) { - bool an1, an2; /* This can happen in incomplete types that should be handled earlier. */ gcc_assert (t1 && t2); @@ -653,9 +745,8 @@ odr_subtypes_equivalent_p (tree t1, tree return true; /* Anonymous namespace types must match exactly. */ - an1 = type_in_anonymous_namespace_p (t1); - an2 = type_in_anonymous_namespace_p (t2); - if (an1 != an2 || an1) + if ((type_with_linkage_p (t1) && type_in_anonymous_namespace_p (t1)) + || (type_with_linkage_p (t2) && type_in_anonymous_namespace_p (t2))) return false; /* For ODR types be sure to compare their names. @@ -1019,10 +1122,10 @@ warn_types_mismatch (tree t1, tree t2) } /* It is a quite common bug to reference anonymous namespace type in non-anonymous namespace class. */ - if (type_in_anonymous_namespace_p (t1) - || type_in_anonymous_namespace_p (t2)) + if ((type_with_linkage_p (t1) && type_in_anonymous_namespace_p (t1)) + || (type_with_linkage_p (t2) && type_in_anonymous_namespace_p (t2))) { - if (!type_in_anonymous_namespace_p (t1)) + if (type_with_linkage_p (t1) && !type_in_anonymous_namespace_p (t1)) { tree tmp = t1;; t1 = t2; @@ -1166,8 +1269,8 @@ odr_types_equivalent_p (tree t1, tree t2 /* Check first for the obvious case of pointer identity. */ if (t1 == t2) return true; - gcc_assert (!type_in_anonymous_namespace_p (t1)); - gcc_assert (!type_in_anonymous_namespace_p (t2)); + 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)); /* Can't be the same type if the types don't have the same code. */ if (TREE_CODE (t1) != TREE_CODE (t2)) @@ -1976,7 +2089,10 @@ get_odr_type (tree type, bool insert) val->type = type; val->bases = vNULL; val->derived_types = vNULL; - val->anonymous_namespace = type_in_anonymous_namespace_p (type); + if (type_with_linkage_p (type)) + val->anonymous_namespace = type_in_anonymous_namespace_p (type); + else + val->anonymous_namespace = 0; build_bases = COMPLETE_TYPE_P (val->type); insert_to_odr_array = true; if (slot) ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: False ODR violation positives on anonymous namespace types 2015-05-12 14:54 ` Jan Hubicka @ 2015-05-12 16:07 ` Jason Merrill 2015-05-12 16:27 ` Jan Hubicka 0 siblings, 1 reply; 13+ messages in thread From: Jason Merrill @ 2015-05-12 16:07 UTC (permalink / raw) To: Jan Hubicka; +Cc: gcc-patches Looks good. Jason ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: False ODR violation positives on anonymous namespace types 2015-05-12 16:07 ` Jason Merrill @ 2015-05-12 16:27 ` Jan Hubicka 2015-05-12 18:40 ` Jan Hubicka 0 siblings, 1 reply; 13+ messages in thread From: Jan Hubicka @ 2015-05-12 16:27 UTC (permalink / raw) To: Jason Merrill; +Cc: Jan Hubicka, gcc-patches > Looks good. Thank you! Testing looks good, too, no false positives on Firefox. I suppose the next challenge is to make type_with_linkage_p to return false on types that was not built by C++ FE as discussed in https://gcc.gnu.org/ml/gcc-patches/2015-05/msg01023.html What would be the preferred solution? Honza > > Jason ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: False ODR violation positives on anonymous namespace types 2015-05-12 16:27 ` Jan Hubicka @ 2015-05-12 18:40 ` Jan Hubicka 0 siblings, 0 replies; 13+ messages in thread From: Jan Hubicka @ 2015-05-12 18:40 UTC (permalink / raw) To: Jan Hubicka; +Cc: Jason Merrill, gcc-patches Hi, here is patch I LTO-bootstrapped and regtested on x86_64-linux and I am going to commit shortly. Thank you for useful discussion - I did not really realize before that types may or may not have linkage. This caused surprises indeed ;) Honza * ipa-devirt.c (type_with_linkage_p): New function. (type_in_anonymous_namespace_p): Move here from tree.c; assert that type has linkage. (odr_type_p): Move here from ipa-utils.h; use type_with_linkage_p. (can_be_name_hashed_p): Simplify. (hash_odr_name): Check that type has linkage before checking if it is anonymous. (types_same_for_odr): Likewise. (odr_name_hasher::equal): Likewise. (odr_subtypes_equivalent_p): Likewise. (warn_types_mismatch): Likewise. (get_odr_type): Likewise. (odr_types_equivalent_p): Fix checking of TYPE_MAIN_VARIANT. * ipa-utils.h (odr_type_p): Move offline. * tree.c (need_assembler_name_p): Fix handling of types without linkages. (type_in_anonymous_namespace_p): Move to ipa-devirt.c Index: ipa-devirt.c =================================================================== --- ipa-devirt.c (revision 223020) +++ ipa-devirt.c (working copy) @@ -245,6 +245,47 @@ struct GTY(()) odr_type_d bool rtti_broken; }; +/* Return true if T is a type with linkage defined. */ + +static bool +type_with_linkage_p (const_tree t) +{ + return (RECORD_OR_UNION_TYPE_P (t) + || TREE_CODE (t) == ENUMERAL_TYPE); +} + +/* Return true if T is in anonymous namespace. + This works only on those C++ types with linkage defined. */ + +bool +type_in_anonymous_namespace_p (const_tree t) +{ + gcc_assert (type_with_linkage_p (t)); + /* TREE_PUBLIC of TYPE_STUB_DECL may not be properly set for + backend produced types (such as va_arg_type); those have CONTEXT NULL + and never are considered anonymoius. */ + if (!TYPE_CONTEXT (t)) + return false; + return (TYPE_STUB_DECL (t) && !TREE_PUBLIC (TYPE_STUB_DECL (t))); +} + +/* Return true of T is type with One Definition Rule info attached. + It means that either it is anonymous type or it has assembler name + set. */ + +bool +odr_type_p (const_tree t) +{ + if (type_with_linkage_p (t) && type_in_anonymous_namespace_p (t)) + return true; + /* We do not have this information when not in LTO, but we do not need + to care, since it is used only for type merging. */ + gcc_checking_assert (in_lto_p || flag_lto); + + return (TYPE_NAME (t) + && (DECL_ASSEMBLER_NAME_SET_P (TYPE_NAME (t)))); +} + /* Return TRUE if all derived types of T are known and thus we may consider the walk of derived type complete. @@ -341,8 +382,7 @@ main_odr_variant (const_tree t) static bool can_be_name_hashed_p (tree t) { - return (!in_lto_p || type_in_anonymous_namespace_p (t) - || (TYPE_NAME (t) && DECL_ASSEMBLER_NAME_SET_P (TYPE_NAME (t)))); + return (!in_lto_p || odr_type_p (t)); } /* Hash type by its ODR name. */ @@ -358,7 +398,7 @@ hash_odr_name (const_tree t) return htab_hash_pointer (t); /* Anonymous types are unique. */ - if (type_in_anonymous_namespace_p (t)) + if (type_with_linkage_p (t) && type_in_anonymous_namespace_p (t)) return htab_hash_pointer (t); gcc_checking_assert (TYPE_NAME (t) @@ -381,7 +421,7 @@ can_be_vtable_hashed_p (tree t) if (TYPE_MAIN_VARIANT (t) != t) return false; /* Anonymous namespace types are always handled by name hash. */ - if (type_in_anonymous_namespace_p (t)) + if (type_with_linkage_p (t) && type_in_anonymous_namespace_p (t)) return false; return (TREE_CODE (t) == RECORD_TYPE && TYPE_BINFO (t) && BINFO_VTABLE (TYPE_BINFO (t))); @@ -455,8 +495,8 @@ types_same_for_odr (const_tree type1, co /* Check for anonymous namespaces. Those have !TREE_PUBLIC on the corresponding TYPE_STUB_DECL. */ - if (type_in_anonymous_namespace_p (type1) - || type_in_anonymous_namespace_p (type2)) + if ((type_with_linkage_p (type1) && type_in_anonymous_namespace_p (type1)) + || (type_with_linkage_p (type2) && type_in_anonymous_namespace_p (type2))) return false; @@ -565,8 +605,8 @@ odr_name_hasher::equal (const odr_type_d return false; /* Check for anonymous namespaces. Those have !TREE_PUBLIC on the corresponding TYPE_STUB_DECL. */ - if (type_in_anonymous_namespace_p (t1) - || type_in_anonymous_namespace_p (t2)) + if ((type_with_linkage_p (t1) && type_in_anonymous_namespace_p (t1)) + || (type_with_linkage_p (t2) && type_in_anonymous_namespace_p (t2))) return false; gcc_checking_assert (DECL_ASSEMBLER_NAME (TYPE_NAME (t1))); gcc_checking_assert (DECL_ASSEMBLER_NAME (TYPE_NAME (t2))); @@ -642,7 +682,6 @@ static bool odr_subtypes_equivalent_p (tree t1, tree t2, hash_set<type_pair,pair_traits> *visited) { - bool an1, an2; /* This can happen in incomplete types that should be handled earlier. */ gcc_assert (t1 && t2); @@ -653,9 +692,8 @@ odr_subtypes_equivalent_p (tree t1, tree return true; /* Anonymous namespace types must match exactly. */ - an1 = type_in_anonymous_namespace_p (t1); - an2 = type_in_anonymous_namespace_p (t2); - if (an1 != an2 || an1) + if ((type_with_linkage_p (t1) && type_in_anonymous_namespace_p (t1)) + || (type_with_linkage_p (t2) && type_in_anonymous_namespace_p (t2))) return false; /* For ODR types be sure to compare their names. @@ -1019,10 +1057,10 @@ warn_types_mismatch (tree t1, tree t2) } /* It is a quite common bug to reference anonymous namespace type in non-anonymous namespace class. */ - if (type_in_anonymous_namespace_p (t1) - || type_in_anonymous_namespace_p (t2)) + if ((type_with_linkage_p (t1) && type_in_anonymous_namespace_p (t1)) + || (type_with_linkage_p (t2) && type_in_anonymous_namespace_p (t2))) { - if (!type_in_anonymous_namespace_p (t1)) + if (type_with_linkage_p (t1) && !type_in_anonymous_namespace_p (t1)) { tree tmp = t1;; t1 = t2; @@ -1166,8 +1204,8 @@ odr_types_equivalent_p (tree t1, tree t2 /* Check first for the obvious case of pointer identity. */ if (t1 == t2) return true; - gcc_assert (!type_in_anonymous_namespace_p (t1)); - gcc_assert (!type_in_anonymous_namespace_p (t2)); + 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)); /* Can't be the same type if the types don't have the same code. */ if (TREE_CODE (t1) != TREE_CODE (t2)) @@ -1498,43 +1536,53 @@ odr_types_equivalent_p (tree t1, tree t2 return false; } if ((TYPE_MAIN_VARIANT (t1) == t1 || TYPE_MAIN_VARIANT (t2) == t2) + && COMPLETE_TYPE_P (TYPE_MAIN_VARIANT (t1)) + && COMPLETE_TYPE_P (TYPE_MAIN_VARIANT (t2)) + && odr_type_p (TYPE_MAIN_VARIANT (t1)) + && odr_type_p (TYPE_MAIN_VARIANT (t2)) && (TYPE_METHODS (TYPE_MAIN_VARIANT (t1)) != TYPE_METHODS (TYPE_MAIN_VARIANT (t2)))) { - for (f1 = TYPE_METHODS (TYPE_MAIN_VARIANT (t1)), - f2 = TYPE_METHODS (TYPE_MAIN_VARIANT (t2)); - f1 && f2 ; f1 = DECL_CHAIN (f1), f2 = DECL_CHAIN (f2)) - { - if (DECL_ASSEMBLER_NAME (f1) != DECL_ASSEMBLER_NAME (f2)) - { - warn_odr (t1, t2, f1, f2, warn, warned, - G_("a different method of same type " - "is defined in another translation unit")); - return false; - } - if (DECL_VIRTUAL_P (f1) != DECL_VIRTUAL_P (f2)) - { - warn_odr (t1, t2, f1, f2, warn, warned, - G_("s definition that differs by virtual " - "keyword in another translation unit")); - return false; - } - if (DECL_VINDEX (f1) != DECL_VINDEX (f2)) - { - warn_odr (t1, t2, f1, f2, warn, warned, - G_("virtual table layout differs in another " - "translation unit")); - return false; - } - if (odr_subtypes_equivalent_p (TREE_TYPE (f1), TREE_TYPE (f2), visited)) - { - warn_odr (t1, t2, f1, f2, warn, warned, - G_("method with incompatible type is defined " - "in another translation unit")); - return false; - } - } - if (f1 || f2) + /* Currently free_lang_data sets TYPE_METHODS to error_mark_node + if it is non-NULL so this loop will never realy execute. */ + if (TYPE_METHODS (TYPE_MAIN_VARIANT (t1)) != error_mark_node + && TYPE_METHODS (TYPE_MAIN_VARIANT (t2)) != error_mark_node) + for (f1 = TYPE_METHODS (TYPE_MAIN_VARIANT (t1)), + f2 = TYPE_METHODS (TYPE_MAIN_VARIANT (t2)); + f1 && f2 ; f1 = DECL_CHAIN (f1), f2 = DECL_CHAIN (f2)) + { + if (DECL_ASSEMBLER_NAME (f1) != DECL_ASSEMBLER_NAME (f2)) + { + warn_odr (t1, t2, f1, f2, warn, warned, + G_("a different method of same type " + "is defined in another " + "translation unit")); + return false; + } + if (DECL_VIRTUAL_P (f1) != DECL_VIRTUAL_P (f2)) + { + warn_odr (t1, t2, f1, f2, warn, warned, + G_("s definition that differs by virtual " + "keyword in another translation unit")); + return false; + } + if (DECL_VINDEX (f1) != DECL_VINDEX (f2)) + { + warn_odr (t1, t2, f1, f2, warn, warned, + G_("virtual table layout differs " + "in another translation unit")); + return false; + } + if (odr_subtypes_equivalent_p (TREE_TYPE (f1), + TREE_TYPE (f2), visited)) + { + warn_odr (t1, t2, f1, f2, warn, warned, + G_("method with incompatible type is " + "defined in another translation unit")); + return false; + } + } + if ((f1 == NULL) != (f2 == NULL)) { warn_odr (t1, t2, NULL, NULL, warn, warned, G_("a type with different number of methods " @@ -1976,7 +2024,10 @@ get_odr_type (tree type, bool insert) val->type = type; val->bases = vNULL; val->derived_types = vNULL; - val->anonymous_namespace = type_in_anonymous_namespace_p (type); + if (type_with_linkage_p (type)) + val->anonymous_namespace = type_in_anonymous_namespace_p (type); + else + val->anonymous_namespace = 0; build_bases = COMPLETE_TYPE_P (val->type); insert_to_odr_array = true; if (slot) Index: ipa-utils.h =================================================================== --- ipa-utils.h (revision 223020) +++ ipa-utils.h (working copy) @@ -63,6 +63,7 @@ possible_polymorphic_call_targets (tree, void **cache_token = NULL, bool speuclative = false); odr_type get_odr_type (tree, bool insert = false); +bool odr_type_p (const_tree t); bool possible_polymorphic_call_target_p (tree ref, gimple stmt, struct cgraph_node *n); void dump_possible_polymorphic_call_targets (FILE *, tree, HOST_WIDE_INT, const ipa_polymorphic_call_context &); @@ -153,23 +154,6 @@ possible_polymorphic_call_target_p (stru context, n); } -/* Return true of T is type with One Definition Rule info attached. - It means that either it is anonymous type or it has assembler name - set. */ - -static inline bool -odr_type_p (const_tree t) -{ - if (type_in_anonymous_namespace_p (t)) - return true; - /* We do not have this information when not in LTO, but we do not need - to care, since it is used only for type merging. */ - gcc_assert (in_lto_p || flag_lto); - - return (TYPE_NAME (t) - && (DECL_ASSEMBLER_NAME_SET_P (TYPE_NAME (t)))); -} - /* Return true if BINFO corresponds to a type with virtual methods. Every type has several BINFOs. One is the BINFO associated by the type Index: tree.c =================================================================== --- tree.c (revision 223021) +++ tree.c (working copy) @@ -5160,27 +5160,33 @@ free_lang_data_in_type (tree type) static inline bool need_assembler_name_p (tree decl) { - /* We use DECL_ASSEMBLER_NAME to hold mangled type names for One Definition Rule - merging. */ + /* We use DECL_ASSEMBLER_NAME to hold mangled type names for One Definition + Rule merging. This makes type_odr_p to return true on those types during + LTO and by comparing the mangled name, we can say what types are intended + to be equivalent across compilation unit. + + We do not store names of type_in_anonymous_namespace_p. + + Record, union and enumeration type have linkage that allows use + to check type_in_anonymous_namespace_p. We do not mangle compound types + that always can be compared structurally. + + Similarly for builtin types, we compare properties of their main variant. + A special case are integer types where mangling do make differences + between char/signed char/unsigned char etc. Storing name for these makes + e.g. -fno-signed-char/-fsigned-char mismatches to be handled well. + See cp/mangle.c:write_builtin_type for details. */ + if (flag_lto_odr_type_mering && TREE_CODE (decl) == TYPE_DECL && DECL_NAME (decl) && decl == TYPE_NAME (TREE_TYPE (decl)) - && !is_lang_specific (TREE_TYPE (decl)) - /* Save some work. Names of builtin types are always derived from - properties of its main variant. A special case are integer types - where mangling do make differences between char/signed char/unsigned - char etc. Storing name for these makes e.g. - -fno-signed-char/-fsigned-char mismatches to be handled well. - - See cp/mangle.c:write_builtin_type for details. */ - && (TREE_CODE (TREE_TYPE (decl)) != VOID_TYPE - && TREE_CODE (TREE_TYPE (decl)) != BOOLEAN_TYPE - && TREE_CODE (TREE_TYPE (decl)) != REAL_TYPE - && TREE_CODE (TREE_TYPE (decl)) != FIXED_POINT_TYPE) && !TYPE_ARTIFICIAL (TREE_TYPE (decl)) - && !variably_modified_type_p (TREE_TYPE (decl), NULL_TREE) - && !type_in_anonymous_namespace_p (TREE_TYPE (decl))) + && (((RECORD_OR_UNION_TYPE_P (TREE_TYPE (decl)) + || TREE_CODE (TREE_TYPE (decl)) == ENUMERAL_TYPE) + && !type_in_anonymous_namespace_p (TREE_TYPE (decl))) + || TREE_CODE (TREE_TYPE (decl)) == INTEGER_TYPE) + && !variably_modified_type_p (TREE_TYPE (decl), NULL_TREE)) return !DECL_ASSEMBLER_NAME_SET_P (decl); /* Only FUNCTION_DECLs and VAR_DECLs are considered. */ if (TREE_CODE (decl) != FUNCTION_DECL @@ -12037,18 +12043,6 @@ obj_type_ref_class (tree ref) return TREE_TYPE (ref); } -/* Return true if T is in anonymous namespace. */ - -bool -type_in_anonymous_namespace_p (const_tree t) -{ - /* TREE_PUBLIC of TYPE_STUB_DECL may not be properly set for - bulitin types; those have CONTEXT NULL. */ - if (!TYPE_CONTEXT (t)) - return false; - return (TYPE_STUB_DECL (t) && !TREE_PUBLIC (TYPE_STUB_DECL (t))); -} - /* Lookup sub-BINFO of BINFO of TYPE at offset POS. */ static tree ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2015-05-12 18:10 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-05-11 14:28 False ODR violation positives on anonymous namespace types Jan Hubicka 2015-05-11 17:16 ` Jason Merrill 2015-05-11 17:46 ` Jan Hubicka 2015-05-11 17:54 ` Jason Merrill 2015-05-11 18:05 ` Jan Hubicka 2015-05-11 21:21 ` Jason Merrill 2015-05-11 21:52 ` Jan Hubicka 2015-05-12 13:51 ` Jason Merrill 2015-05-12 14:13 ` Jan Hubicka 2015-05-12 14:54 ` Jan Hubicka 2015-05-12 16:07 ` Jason Merrill 2015-05-12 16:27 ` Jan Hubicka 2015-05-12 18:40 ` Jan Hubicka
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).