False ODR violation positives on anonymous namespace types

False ODR violation positives on anonymous namespace types

[Jan Hubicka]

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

Re: False ODR violation positives on anonymous namespace types

[Jason Merrill]

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

Re: False ODR violation positives on anonymous namespace types

[Jan Hubicka]

> 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

Re: False ODR violation positives on anonymous namespace types

[Jason Merrill]

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

Re: False ODR violation positives on anonymous namespace types

[Jan Hubicka]

> 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

Re: False ODR violation positives on anonymous namespace types

[Jason Merrill]

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

Re: False ODR violation positives on anonymous namespace types

[Jan Hubicka]

> 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

Re: False ODR violation positives on anonymous namespace types

[Jason Merrill]

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

Re: False ODR violation positives on anonymous namespace types

[Jan Hubicka]

> 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

Re: False ODR violation positives on anonymous namespace types

[Jan Hubicka]

>       /* 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)

Re: False ODR violation positives on anonymous namespace types

[Jason Merrill]

Looks good.

Jason

Re: False ODR violation positives on anonymous namespace types

[Jan Hubicka]

> 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

Re: False ODR violation positives on anonymous namespace types

[Jan Hubicka]

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