diff mbox

Type comparing TLC

Message ID 20150218161708.GD20180@kam.mff.cuni.cz
State New
Headers show

Commit Message

Jan Hubicka Feb. 18, 2015, 4:17 p.m. UTC
Hi,
looking across the ODR violation messages in libreoffice and Chromium I found
some false positives and some confused messages.  This patch fixes them. In partiuclar
 - I introduced nasty vtable corruption when breaking out my type merging patches,
   so we ended up creating separate entries for each copy of type without BINFO :(
 - C++ now allows to use enum that has no fields defined. Those needs to match enums
   with fields from other unit
 - class and vtable layout diffing got confused by presence of extra vptr pointers
   and bases. Fixed thus.

Bootstrapped/regtested x86_64-linux, comitted.

Honza

	* ipa-devirt.c (odr_subtypes_equivalent_p): Fix formating.
	(compare_virtual_tables): Be smarter about skipping typeinfos;
	do sane output on virtual table table mismatch.
	(warn_odr): Be ready for forward declarations of enums;
	output sane info on base mismatch and virtual table mismatch.
	(add_type_duplicate): Fix code choosing prevailing type; do not ICE
	when only one type is polymorphic.
	(get_odr_type): Fix hashtable corruption.
	(dump_odr_type): Dump mangled names.

Comments

Thomas Schwinge Feb. 20, 2015, 1:15 p.m. UTC | #1
Hi!

On Wed, 18 Feb 2015 17:17:08 +0100, Jan Hubicka <hubicka@ucw.cz> wrote:
> looking across the ODR violation messages in libreoffice and Chromium I found
> some false positives and some confused messages.  This patch fixes them. In partiuclar
>  - I introduced nasty vtable corruption when breaking out my type merging patches,
>    so we ended up creating separate entries for each copy of type without BINFO :(
>  - C++ now allows to use enum that has no fields defined. Those needs to match enums
>    with fields from other unit
>  - class and vtable layout diffing got confused by presence of extra vptr pointers
>    and bases. Fixed thus.
> 
> Bootstrapped/regtested x86_64-linux, comitted.

> 	* ipa-devirt.c (odr_subtypes_equivalent_p): Fix formating.
> 	(compare_virtual_tables): Be smarter about skipping typeinfos;
> 	do sane output on virtual table table mismatch.
> 	(warn_odr): Be ready for forward declarations of enums;
> 	output sane info on base mismatch and virtual table mismatch.
> 	(add_type_duplicate): Fix code choosing prevailing type; do not ICE
> 	when only one type is polymorphic.
> 	(get_odr_type): Fix hashtable corruption.
> 	(dump_odr_type): Dump mangled names.

I find this commit, r220790, cause the following regression in an
offloading-enabled configuration:

    [-PASS:-]{+FAIL:+} libgomp.c++/target-3.C (test for excess errors)
    PASS: libgomp.c++/target-3.C execution test

    [...]/build-gcc/gcc/xgcc -B[...]/build-gcc/gcc/ [...]/source-gcc/libgomp/testsuite/libgomp.c++/target-3.C -B[...]/build-gcc/x86_64-unknown-linux-gnu/./libgomp/ -B[...]/build-gcc/x86_64-unknown-linux-gnu/./libgomp/.libs -I[...]/build-gcc/x86_64-unknown-linux-gnu/./libgomp -I[...]/source-gcc/libgomp/testsuite/../../include -I[...]/source-gcc/libgomp/testsuite/.. -I/usr/local/cuda-5.5/targets/x86_64-linux/include -fmessage-length=0 -fno-diagnostics-show-caret -fdiagnostics-color=never -B[...]/install/offload-nvptx-none/libexec/gcc/x86_64-unknown-linux-gnu/5.0.0 -B[...]/install/offload-nvptx-none/bin -B[...]/install/offload-x86_64-intelmicemul-linux-gnu/libexec/gcc/x86_64-unknown-linux-gnu/5.0.0 -B[...]/install/offload-x86_64-intelmicemul-linux-gnu/bin -fopenmp -nostdinc++ -I[...]/build-gcc/x86_64-unknown-linux-gnu/libstdc++-v3/include/x86_64-unknown-linux-gnu -I[...]/build-gcc/x86_64-unknown-linux-gnu/libstdc++-v3/include -I[...]/source-gcc/libstdc++-v3/libsupc++ -I[...]/source-gcc/libstdc++-v3/include/backward -I[...]/source-gcc/libstdc++-v3/testsuite/util -B[...]/build-gcc/x86_64-unknown-linux-gnu/./libgomp/../libstdc++-v3/src/.libs -L[...]/build-gcc/x86_64-unknown-linux-gnu/./libgomp/.libs -L[...]/build-gcc/x86_64-unknown-linux-gnu/./libgomp/../libstdc++-v3/src/.libs -lstdc++ -lm -o ./target-3.exe
    [...]/source-gcc/libgomp/testsuite/libgomp.c++/../libgomp.c/target-2.c:27:13: warning: type 'struct .omp_data_s.7' violates one definition rule [-Wodr]
         #pragma omp parallel for reduction(+:s)
                 ^
    [...]/source-gcc/libgomp/testsuite/libgomp.c++/../libgomp.c/target-2.c:27:13: note: a different type is defined in another translation unit
         #pragma omp parallel for reduction(+:s)
                 ^
    [...]/source-gcc/libgomp/testsuite/libgomp.c++/../libgomp.c/target-2.c:22:17: note: the first difference of corresponding definitions is field 'b.0'
       double b[3 * x], c[3 * x], d[3 * x], e[3 * x];
                     ^
    [...]/source-gcc/libgomp/testsuite/libgomp.c++/../libgomp.c/target-2.c:22:17: note: a field of same name but different type is defined in another translation unit
       double b[3 * x], c[3 * x], d[3 * x], e[3 * x];
                     ^
    [...]/source-gcc/libgomp/testsuite/libgomp.c++/../libgomp.c/target-2.c:42:13: warning: type 'struct .omp_data_s.20' violates one definition rule [-Wodr]
         #pragma omp parallel for reduction(+:s)
                 ^
    [...]


Grüße,
 Thomas
Jakub Jelinek Feb. 20, 2015, 1:27 p.m. UTC | #2
On Fri, Feb 20, 2015 at 02:15:24PM +0100, Thomas Schwinge wrote:
> > 	* ipa-devirt.c (odr_subtypes_equivalent_p): Fix formating.
> > 	(compare_virtual_tables): Be smarter about skipping typeinfos;
> > 	do sane output on virtual table table mismatch.
> > 	(warn_odr): Be ready for forward declarations of enums;
> > 	output sane info on base mismatch and virtual table mismatch.
> > 	(add_type_duplicate): Fix code choosing prevailing type; do not ICE
> > 	when only one type is polymorphic.
> > 	(get_odr_type): Fix hashtable corruption.
> > 	(dump_odr_type): Dump mangled names.
> 
> I find this commit, r220790, cause the following regression in an
> offloading-enabled configuration:

I'd think that we shouldn't report ODR violations for types with
DECL_ARTIFICIAL (or just DECL_NAMELESS?) TYPE_DECLs.
Especially the DECL_NAMELESS ones have names just for debugging purposes.
> 
>     [-PASS:-]{+FAIL:+} libgomp.c++/target-3.C (test for excess errors)
>     PASS: libgomp.c++/target-3.C execution test
> 
>     [...]/build-gcc/gcc/xgcc -B[...]/build-gcc/gcc/ [...]/source-gcc/libgomp/testsuite/libgomp.c++/target-3.C -B[...]/build-gcc/x86_64-unknown-linux-gnu/./libgomp/ -B[...]/build-gcc/x86_64-unknown-linux-gnu/./libgomp/.libs -I[...]/build-gcc/x86_64-unknown-linux-gnu/./libgomp -I[...]/source-gcc/libgomp/testsuite/../../include -I[...]/source-gcc/libgomp/testsuite/.. -I/usr/local/cuda-5.5/targets/x86_64-linux/include -fmessage-length=0 -fno-diagnostics-show-caret -fdiagnostics-color=never -B[...]/install/offload-nvptx-none/libexec/gcc/x86_64-unknown-linux-gnu/5.0.0 -B[...]/install/offload-nvptx-none/bin -B[...]/install/offload-x86_64-intelmicemul-linux-gnu/libexec/gcc/x86_64-unknown-linux-gnu/5.0.0 -B[...]/install/offload-x86_64-intelmicemul-linux-gnu/bin -fopenmp -nostdinc++ -I[...]/build-gcc/x86_64-unknown-linux-gnu/libstdc++-v3/include/x86_64-unknown-linux-gnu -I[...]/build-gcc/x86_64-unknown-linux-gnu/libstdc++-v3/include -I[...]/source-gcc/libstdc++-v3/libsupc++ -I[...]/source-gcc/libstdc++-v3/include/backward -I[...]/source-gcc/libstdc++-v3/testsuite/util -B[...]/build-gcc/x86_64-unknown-linux-gnu/./libgomp/../libstdc++-v3/src/.libs -L[...]/build-gcc/x86_64-unknown-linux-gnu/./libgomp/.libs -L[...]/build-gcc/x86_64-unknown-linux-gnu/./libgomp/../libstdc++-v3/src/.libs -lstdc++ -lm -o ./target-3.exe
>     [...]/source-gcc/libgomp/testsuite/libgomp.c++/../libgomp.c/target-2.c:27:13: warning: type 'struct .omp_data_s.7' violates one definition rule [-Wodr]
>          #pragma omp parallel for reduction(+:s)
>                  ^
>     [...]/source-gcc/libgomp/testsuite/libgomp.c++/../libgomp.c/target-2.c:27:13: note: a different type is defined in another translation unit
>          #pragma omp parallel for reduction(+:s)
>                  ^
>     [...]/source-gcc/libgomp/testsuite/libgomp.c++/../libgomp.c/target-2.c:22:17: note: the first difference of corresponding definitions is field 'b.0'
>        double b[3 * x], c[3 * x], d[3 * x], e[3 * x];
>                      ^
>     [...]/source-gcc/libgomp/testsuite/libgomp.c++/../libgomp.c/target-2.c:22:17: note: a field of same name but different type is defined in another translation unit
>        double b[3 * x], c[3 * x], d[3 * x], e[3 * x];
>                      ^
>     [...]/source-gcc/libgomp/testsuite/libgomp.c++/../libgomp.c/target-2.c:42:13: warning: type 'struct .omp_data_s.20' violates one definition rule [-Wodr]
>          #pragma omp parallel for reduction(+:s)
>                  ^
>     [...]

	Jakub
Jan Hubicka Feb. 20, 2015, 6:49 p.m. UTC | #3
> On Fri, Feb 20, 2015 at 02:15:24PM +0100, Thomas Schwinge wrote:
> > > 	* ipa-devirt.c (odr_subtypes_equivalent_p): Fix formating.
> > > 	(compare_virtual_tables): Be smarter about skipping typeinfos;
> > > 	do sane output on virtual table table mismatch.
> > > 	(warn_odr): Be ready for forward declarations of enums;
> > > 	output sane info on base mismatch and virtual table mismatch.
> > > 	(add_type_duplicate): Fix code choosing prevailing type; do not ICE
> > > 	when only one type is polymorphic.
> > > 	(get_odr_type): Fix hashtable corruption.
> > > 	(dump_odr_type): Dump mangled names.
> > 
> > I find this commit, r220790, cause the following regression in an
> > offloading-enabled configuration:
> 
> I'd think that we shouldn't report ODR violations for types with
> DECL_ARTIFICIAL (or just DECL_NAMELESS?) TYPE_DECLs.
> Especially the DECL_NAMELESS ones have names just for debugging purposes.

Yes, I was considering to do the same because of warning on types of typeinfos
on Firefox.  Firefox links together -fsigned-char and -funsigned-char that is
an ODR violation but reporting this about typeinfo itself is confusing.

I will disable warning on those.  Should it be DECL_ARTIFICIAL only or both flags?
I am not really familiar about what types can be DECL_NAMELESS or DECL_ARTIFICIAL.

Honza
Ilya Verbin March 10, 2015, 12:37 p.m. UTC | #4
Hi!

On Fri, Feb 20, 2015 at 19:49:03 +0100, Jan Hubicka wrote:
> > On Fri, Feb 20, 2015 at 02:15:24PM +0100, Thomas Schwinge wrote:
> > > > 	* ipa-devirt.c (odr_subtypes_equivalent_p): Fix formating.
> > > > 	(compare_virtual_tables): Be smarter about skipping typeinfos;
> > > > 	do sane output on virtual table table mismatch.
> > > > 	(warn_odr): Be ready for forward declarations of enums;
> > > > 	output sane info on base mismatch and virtual table mismatch.
> > > > 	(add_type_duplicate): Fix code choosing prevailing type; do not ICE
> > > > 	when only one type is polymorphic.
> > > > 	(get_odr_type): Fix hashtable corruption.
> > > > 	(dump_odr_type): Dump mangled names.
> > > 
> > > I find this commit, r220790, cause the following regression in an
> > > offloading-enabled configuration:
> > 
> > I'd think that we shouldn't report ODR violations for types with
> > DECL_ARTIFICIAL (or just DECL_NAMELESS?) TYPE_DECLs.
> > Especially the DECL_NAMELESS ones have names just for debugging purposes.
> 
> Yes, I was considering to do the same because of warning on types of typeinfos
> on Firefox.  Firefox links together -fsigned-char and -funsigned-char that is
> an ODR violation but reporting this about typeinfo itself is confusing.
> 
> I will disable warning on those.  Should it be DECL_ARTIFICIAL only or both flags?
> I am not really familiar about what types can be DECL_NAMELESS or DECL_ARTIFICIAL.

Any plans to fix this in GCC 5?

Thanks,
  -- Ilya
Jan Hubicka March 10, 2015, 6:12 p.m. UTC | #5
> Hi!
> 
> On Fri, Feb 20, 2015 at 19:49:03 +0100, Jan Hubicka wrote:
> > > On Fri, Feb 20, 2015 at 02:15:24PM +0100, Thomas Schwinge wrote:
> > > > > 	* ipa-devirt.c (odr_subtypes_equivalent_p): Fix formating.
> > > > > 	(compare_virtual_tables): Be smarter about skipping typeinfos;
> > > > > 	do sane output on virtual table table mismatch.
> > > > > 	(warn_odr): Be ready for forward declarations of enums;
> > > > > 	output sane info on base mismatch and virtual table mismatch.
> > > > > 	(add_type_duplicate): Fix code choosing prevailing type; do not ICE
> > > > > 	when only one type is polymorphic.
> > > > > 	(get_odr_type): Fix hashtable corruption.
> > > > > 	(dump_odr_type): Dump mangled names.
> > > > 
> > > > I find this commit, r220790, cause the following regression in an
> > > > offloading-enabled configuration:
> > > 
> > > I'd think that we shouldn't report ODR violations for types with
> > > DECL_ARTIFICIAL (or just DECL_NAMELESS?) TYPE_DECLs.
> > > Especially the DECL_NAMELESS ones have names just for debugging purposes.
> > 
> > Yes, I was considering to do the same because of warning on types of typeinfos
> > on Firefox.  Firefox links together -fsigned-char and -funsigned-char that is
> > an ODR violation but reporting this about typeinfo itself is confusing.
> > 
> > I will disable warning on those.  Should it be DECL_ARTIFICIAL only or both flags?
> > I am not really familiar about what types can be DECL_NAMELESS or DECL_ARTIFICIAL.
> 
> Any plans to fix this in GCC 5?

yes, only yesterday I got chromium LTO building up and running and it does
produce some ODR warnings that are either confusing or wrong (among many
correct ones).  I plan to check them and fix these issues soon.

Honza
> 
> Thanks,
>   -- Ilya
diff mbox

Patch

Index: ipa-devirt.c
===================================================================
--- ipa-devirt.c	(revision 220741)
+++ ipa-devirt.c	(working copy)
@@ -551,7 +551,8 @@  set_type_binfo (tree type, tree binfo)
 /* Compare T2 and T2 based on name or structure.  */
 
 static bool
-odr_subtypes_equivalent_p (tree t1, tree t2, hash_set<type_pair,pair_traits> *visited)
+odr_subtypes_equivalent_p (tree t1, tree t2,
+			   hash_set<type_pair,pair_traits> *visited)
 {
   bool an1, an2;
 
@@ -618,7 +619,8 @@  compare_virtual_tables (varpool_node *pr
 	  prevailing = vtable;
 	  vtable = tmp;
 	}
-      if (warning_at (DECL_SOURCE_LOCATION (TYPE_NAME (DECL_CONTEXT (vtable->decl))),
+      if (warning_at (DECL_SOURCE_LOCATION
+			(TYPE_NAME (DECL_CONTEXT (vtable->decl))),
 		      OPT_Wodr,
 		      "virtual table of type %qD violates one definition rule",
 		      DECL_CONTEXT (vtable->decl)))
@@ -633,39 +635,118 @@  compare_virtual_tables (varpool_node *pr
     {
       struct ipa_ref *ref1, *ref2;
       bool end1, end2;
+
       end1 = !prevailing->iterate_reference (n1, ref1);
       end2 = !vtable->iterate_reference (n2, ref2);
-      if (end1 && end2)
-	return;
-      if (!end1 && !end2
-	  && DECL_ASSEMBLER_NAME (ref1->referred->decl)
-	     != DECL_ASSEMBLER_NAME (ref2->referred->decl)
-	  && !n2
-	  && !DECL_VIRTUAL_P (ref2->referred->decl)
-	  && DECL_VIRTUAL_P (ref1->referred->decl))
+
+      /* !DECL_VIRTUAL_P means RTTI entry;
+	 We warn when RTTI is lost because non-RTTI previals; we silently
+	 accept the other case.  */
+      while (!end2
+	     && (end1
+	         || (DECL_ASSEMBLER_NAME (ref1->referred->decl)
+		     != DECL_ASSEMBLER_NAME (ref2->referred->decl)
+	             && DECL_VIRTUAL_P (ref1->referred->decl)))
+	     && !DECL_VIRTUAL_P (ref2->referred->decl))
 	{
-	  if (warning_at (DECL_SOURCE_LOCATION (TYPE_NAME (DECL_CONTEXT (vtable->decl))), 0,
+	  if (warning_at (DECL_SOURCE_LOCATION
+			    (TYPE_NAME (DECL_CONTEXT (vtable->decl))), 0,
 			  "virtual table of type %qD contains RTTI information",
 			  DECL_CONTEXT (vtable->decl)))
 	    {
-	      inform (DECL_SOURCE_LOCATION (TYPE_NAME (DECL_CONTEXT (prevailing->decl))),
-		      "but is prevailed by one without from other translation unit");
-	      inform (DECL_SOURCE_LOCATION (TYPE_NAME (DECL_CONTEXT (prevailing->decl))),
+	      inform (DECL_SOURCE_LOCATION
+			(TYPE_NAME (DECL_CONTEXT (prevailing->decl))),
+		      "but is prevailed by one without from other translation "
+		      "unit");
+	      inform (DECL_SOURCE_LOCATION
+			(TYPE_NAME (DECL_CONTEXT (prevailing->decl))),
 		      "RTTI will not work on this type");
 	    }
 	  n2++;
           end2 = !vtable->iterate_reference (n2, ref2);
 	}
-      if (!end1 && !end2
-	  && DECL_ASSEMBLER_NAME (ref1->referred->decl)
-	     != DECL_ASSEMBLER_NAME (ref2->referred->decl)
-	  && !n1
-	  && !DECL_VIRTUAL_P (ref1->referred->decl)
-	  && DECL_VIRTUAL_P (ref2->referred->decl))
+      while (!end1
+	     && (end2
+	         || (DECL_ASSEMBLER_NAME (ref2->referred->decl)
+		     != DECL_ASSEMBLER_NAME (ref1->referred->decl)
+	             && DECL_VIRTUAL_P (ref2->referred->decl)))
+	     && !DECL_VIRTUAL_P (ref1->referred->decl))
 	{
 	  n1++;
           end1 = !vtable->iterate_reference (n1, ref1);
 	}
+
+      /* Finished?  */
+      if (end1 && end2)
+	{
+	  /* Extra paranoia; compare the sizes.  We do not have information
+	     about virtual inheritance offsets, so just be sure that these
+	     match.  
+	     Do this as very last check so the not very informative error
+	     is not output too often.  */
+	  if (DECL_SIZE (prevailing->decl) != DECL_SIZE (vtable->decl))
+	    {
+	      if (warning_at (DECL_SOURCE_LOCATION
+				(TYPE_NAME (DECL_CONTEXT (vtable->decl))), 0,
+			      "virtual table of type %qD violates "
+			      "one definition rule  ",
+			      DECL_CONTEXT (vtable->decl)))
+		{
+		  inform (DECL_SOURCE_LOCATION 
+			    (TYPE_NAME (DECL_CONTEXT (prevailing->decl))),
+			  "the conflicting type defined in another translation "
+			  "unit has virtual table of different size");
+		}
+	    }
+	  return;
+	}
+
+      if (!end1 && !end2)
+	{
+	  if (DECL_ASSEMBLER_NAME (ref1->referred->decl)
+	      == DECL_ASSEMBLER_NAME (ref2->referred->decl))
+	    continue;
+
+	  /* If the loops above stopped on non-virtual pointer, we have
+	     mismatch in RTTI information mangling.  */
+	  if (!DECL_VIRTUAL_P (ref1->referred->decl)
+	      && !DECL_VIRTUAL_P (ref2->referred->decl))
+	    {
+	      if (warning_at (DECL_SOURCE_LOCATION
+				(TYPE_NAME (DECL_CONTEXT (vtable->decl))), 0,
+			      "virtual table of type %qD violates "
+			      "one definition rule  ",
+			      DECL_CONTEXT (vtable->decl)))
+		{
+		  inform (DECL_SOURCE_LOCATION 
+			    (TYPE_NAME (DECL_CONTEXT (prevailing->decl))),
+			  "the conflicting type defined in another translation "
+			  "unit virtual table with different RTTI information");
+		}
+	      return;
+	    }
+	  /* At this point both REF1 and REF2 points either to virtual table
+	     or virtual method.  If one points to virtual table and other to
+	     method we can complain the same way as if one table was shorter
+	     than other pointing out the extra method.  */
+	  gcc_assert (DECL_VIRTUAL_P (ref1->referred->decl)
+		      && (TREE_CODE (ref1->referred->decl) == FUNCTION_DECL
+		          || TREE_CODE (ref1->referred->decl) == VAR_DECL));
+	  gcc_assert (DECL_VIRTUAL_P (ref2->referred->decl)
+		      && (TREE_CODE (ref2->referred->decl) == FUNCTION_DECL
+		          || TREE_CODE (ref2->referred->decl) == VAR_DECL));
+	  if (TREE_CODE (ref1->referred->decl)
+	      != TREE_CODE (ref2->referred->decl))
+	    {
+	      if (TREE_CODE (ref1->referred->decl) == VAR_DECL)
+		end1 = true;
+	      else if (TREE_CODE (ref2->referred->decl) == VAR_DECL)
+		end2 = true;
+	    }
+	}
+
+      /* Complain about size mismatch.  Either we have too many virutal
+ 	 functions or too many virtual table pointers.  */
       if (end1 || end2)
 	{
 	  if (end1)
@@ -681,37 +762,56 @@  compare_virtual_tables (varpool_node *pr
 			  "one definition rule",
 			  DECL_CONTEXT (vtable->decl)))
 	    {
-	      inform (DECL_SOURCE_LOCATION
-		       (TYPE_NAME (DECL_CONTEXT (prevailing->decl))),
-		      "the conflicting type defined in another translation "
-		      "unit");
-	      inform (DECL_SOURCE_LOCATION
-		        (TYPE_NAME (DECL_CONTEXT (ref1->referring->decl))),
-		      "contains additional virtual method %qD",
-		      ref1->referred->decl);
+	      if (TREE_CODE (ref1->referring->decl) == FUNCTION_DECL)
+		{
+		  inform (DECL_SOURCE_LOCATION
+			   (TYPE_NAME (DECL_CONTEXT (prevailing->decl))),
+			  "the conflicting type defined in another translation "
+			  "unit");
+		  inform (DECL_SOURCE_LOCATION
+			    (TYPE_NAME (DECL_CONTEXT (ref1->referring->decl))),
+			  "contains additional virtual method %qD",
+			  ref1->referred->decl);
+		}
+	      else
+		{
+		  inform (DECL_SOURCE_LOCATION
+			   (TYPE_NAME (DECL_CONTEXT (prevailing->decl))),
+			  "the conflicting type defined in another translation "
+			  "unit has virtual table table with more entries");
+		}
 	    }
 	  return;
 	}
-      if (DECL_ASSEMBLER_NAME (ref1->referred->decl)
-	  != DECL_ASSEMBLER_NAME (ref2->referred->decl))
+
+      /* And in the last case we have either mistmatch in between two virtual
+	 methods or two virtual table pointers.  */
+      if (warning_at (DECL_SOURCE_LOCATION
+			(TYPE_NAME (DECL_CONTEXT (vtable->decl))), 0,
+		      "virtual table of type %qD violates "
+		      "one definition rule  ",
+		      DECL_CONTEXT (vtable->decl)))
 	{
-	  if (warning_at (DECL_SOURCE_LOCATION
-			    (TYPE_NAME (DECL_CONTEXT (vtable->decl))), 0,
-			  "virtual table of type %qD violates "
-			  "one definition rule  ",
-			  DECL_CONTEXT (vtable->decl)))
+	  if (TREE_CODE (ref1->referred->decl) == FUNCTION_DECL)
 	    {
 	      inform (DECL_SOURCE_LOCATION 
 			(TYPE_NAME (DECL_CONTEXT (prevailing->decl))),
 		      "the conflicting type defined in another translation "
 		      "unit");
+	      gcc_assert (TREE_CODE (ref2->referred->decl)
+			  == FUNCTION_DECL);
 	      inform (DECL_SOURCE_LOCATION (ref1->referred->decl),
 		      "virtual method %qD", ref1->referred->decl);
 	      inform (DECL_SOURCE_LOCATION (ref2->referred->decl),
 		      "ought to match virtual method %qD but does not",
 		      ref2->referred->decl);
-	      return;
 	    }
+	  else
+	    inform (DECL_SOURCE_LOCATION 
+		      (TYPE_NAME (DECL_CONTEXT (prevailing->decl))),
+		    "the conflicting type defined in another translation "
+		    "unit has virtual table table with different contents");
+	  return;
 	}
     }
 }
@@ -727,6 +827,8 @@  warn_odr (tree t1, tree t2, tree st1, tr
 	  bool warn, bool *warned, const char *reason)
 {
   tree decl2 = TYPE_NAME (t2);
+  if (warned)
+    *warned = false;
 
   if (!warn)
     return;
@@ -840,7 +942,8 @@  odr_types_equivalent_p (tree t1, tree t2
       return false;
     }
 
-  if (TREE_CODE (t1) == ENUMERAL_TYPE)
+  if (TREE_CODE (t1) == ENUMERAL_TYPE
+      && TYPE_VALUES (t1) && TYPE_VALUES (t2))
     {
       tree v1, v2;
       for (v1 = TYPE_VALUES (t1), v2 = TYPE_VALUES (t2);
@@ -1056,8 +1159,20 @@  odr_types_equivalent_p (tree t1, tree t2
 		  f2 = TREE_CHAIN (f2);
 		if (!f1 || !f2)
 		  break;
+		if (DECL_VIRTUAL_P (f1) != DECL_VIRTUAL_P (f2))
+		  {
+		    warn_odr (t1, t2, NULL, NULL, warn, warned,
+			      G_("a type with different virtual table pointers"
+			         " is defined in another translation unit"));
+		    return false;
+		  }
 		if (DECL_ARTIFICIAL (f1) != DECL_ARTIFICIAL (f2))
-		  break;
+		  {
+		    warn_odr (t1, t2, NULL, NULL, warn, warned,
+			      G_("a type with different bases is defined "
+				 "in another translation unit"));
+		    return false;
+		  }
 		if (DECL_NAME (f1) != DECL_NAME (f2)
 		    && !DECL_ARTIFICIAL (f1))
 		  {
@@ -1066,10 +1181,11 @@  odr_types_equivalent_p (tree t1, tree t2
 				 "in another translation unit"));
 		    return false;
 		  }
-		if (!odr_subtypes_equivalent_p (TREE_TYPE (f1), TREE_TYPE (f2), visited))
+		if (!odr_subtypes_equivalent_p (TREE_TYPE (f1),
+						TREE_TYPE (f2), visited))
 		  {
-		    /* Do not warn about artificial fields and just go into generic
-		       field mismatch warning.  */
+		    /* Do not warn about artificial fields and just go into
+ 		       generic field mismatch warning.  */
 		    if (DECL_ARTIFICIAL (f1))
 		      break;
 
@@ -1082,11 +1198,11 @@  odr_types_equivalent_p (tree t1, tree t2
 		  }
 		if (!gimple_compare_field_offset (f1, f2))
 		  {
-		    /* Do not warn about artificial fields and just go into generic
-		       field mismatch warning.  */
+		    /* Do not warn about artificial fields and just go into
+		       generic field mismatch warning.  */
 		    if (DECL_ARTIFICIAL (f1))
 		      break;
-		    warn_odr (t1, t2, t1, t2, warn, warned,
+		    warn_odr (t1, t2, f1, f2, warn, warned,
 			      G_("fields has different layout "
 				 "in another translation unit"));
 		    return false;
@@ -1099,18 +1215,18 @@  odr_types_equivalent_p (tree t1, tree t2
 	       are not the same.  */
 	    if (f1 || f2)
 	      {
-		if (f1 && DECL_ARTIFICIAL (f1))
-		  f1 = NULL;
-		if (f2 && DECL_ARTIFICIAL (f2))
-		  f2 = NULL;
-		if (f1 || f2)
-		  warn_odr (t1, t2, f1, f2, warn, warned,
-			    G_("a type with different number of fields "
-			       "is defined in another translation unit"));
-		/* Ideally we should never get this generic message.  */
+		if ((f1 && DECL_VIRTUAL_P (f1)) || (f2 && DECL_VIRTUAL_P (f2)))
+		  warn_odr (t1, t2, NULL, NULL, warn, warned,
+			    G_("a type with different virtual table pointers"
+			       " is defined in another translation unit"));
+		if ((f1 && DECL_ARTIFICIAL (f1))
+		    || (f2 && DECL_ARTIFICIAL (f2)))
+		  warn_odr (t1, t2, NULL, NULL, warn, warned,
+			    G_("a type with different bases is defined "
+			       "in another translation unit"));
 		else
 		  warn_odr (t1, t2, f1, f2, warn, warned,
-			    G_("a type with different memory representation "
+			    G_("a type with different number of fields "
 			       "is defined in another translation unit"));
 		
 		return false;
@@ -1207,12 +1323,24 @@  add_type_duplicate (odr_type val, tree t
     val->types_set = new hash_set<tree>;
 
   /* Always prefer complete type to be the leader.  */
-  if ((!COMPLETE_TYPE_P (val->type) || !TYPE_BINFO (val->type))
-      && (COMPLETE_TYPE_P (type) && TYPE_BINFO (val->type)))
+
+  if (!COMPLETE_TYPE_P (val->type) && COMPLETE_TYPE_P (type))
+    build_bases = true;
+  else if (COMPLETE_TYPE_P (val->type) && !COMPLETE_TYPE_P (type))
+    ;
+  else if (TREE_CODE (val->type) == ENUMERAL_TYPE
+	   && TREE_CODE (type) == ENUMERAL_TYPE
+	   && !TYPE_VALUES (val->type) && TYPE_VALUES (type))
+    build_bases = true;
+  else if (TREE_CODE (val->type) == RECORD_TYPE
+	   && TREE_CODE (type) == RECORD_TYPE
+	   && TYPE_BINFO (type) && !TYPE_BINFO (val->type))
+    build_bases = true;
+
+  if (build_bases)
     {
       tree tmp = type;
 
-      build_bases = true;
       type = val->type;
       val->type = tmp;
     }
@@ -1303,11 +1431,14 @@  add_type_duplicate (odr_type val, tree t
 		if (base_mismatch)
 		  {
 		    if (!warned && !val->odr_violated)
-		      warn_odr (type, val->type, NULL, NULL,
-				!warned, &warned,
-				"a type with the same name but different base "
-				"type is defined in another translation unit");
-		    warn_types_mismatch (type1, type2);
+		      {
+			warn_odr (type, val->type, NULL, NULL,
+				  !warned, &warned,
+				  "a type with the same name but different base "
+				  "type is defined in another translation unit");
+			if (warned)
+			  warn_types_mismatch (type1, type2);
+		      }
 		    break;
 		  }
 		if (BINFO_OFFSET (base1) != BINFO_OFFSET (base2))
@@ -1320,6 +1451,18 @@  add_type_duplicate (odr_type val, tree t
 				"layout is defined in another translation unit");
 		    break;
 		  }
+		/* One base is polymorphic and the other not.
+		   This ought to be diagnosed earlier, but do not ICE in the
+	 	   checking bellow.  */
+		if (!TYPE_BINFO (type1) != !TYPE_BINFO (type2)
+		    || (TYPE_BINFO (type1)
+			&& polymorphic_type_binfo_p (TYPE_BINFO (type1))
+		           != polymorphic_type_binfo_p (TYPE_BINFO (type2))))
+		  {
+		    gcc_assert (val->odr_violated);
+		    base_mismatch = true;
+		    break;
+		  }
 	      }
 #ifdef ENABLE_CHECKING
 	  /* Sanity check that all bases will be build same way again.  */
@@ -1468,6 +1611,7 @@  get_odr_type (tree type, bool insert)
       val->anonymous_namespace = type_in_anonymous_namespace_p (type);
       build_bases = COMPLETE_TYPE_P (val->type);
       insert_to_odr_array = true;
+      *slot = val;
     }
 
   if (build_bases && TREE_CODE (type) == RECORD_TYPE && TYPE_BINFO (type)
@@ -1479,7 +1623,6 @@  get_odr_type (tree type, bool insert)
       gcc_assert (BINFO_TYPE (TYPE_BINFO (val->type)) = type);
   
       val->all_derivations_known = type_all_derivations_known_p (type);
-      *slot = val;
       for (i = 0; i < BINFO_N_BASE_BINFOS (binfo); i++)
 	/* For now record only polymorphic types. other are
 	   pointless for devirtualization and we can not precisely
@@ -1557,6 +1700,10 @@  dump_odr_type (FILE *f, odr_type t, int
       fprintf (f, "%*s defined at: %s:%i\n", indent * 2, "",
 	       DECL_SOURCE_FILE (TYPE_NAME (t->type)),
 	       DECL_SOURCE_LINE (TYPE_NAME (t->type)));
+      if (DECL_ASSEMBLER_NAME_SET_P (TYPE_NAME (t->type)))
+        fprintf (f, "%*s mangled name: %s\n", indent * 2, "",
+		 IDENTIFIER_POINTER
+		   (DECL_ASSEMBLER_NAME (TYPE_NAME (t->type))));
     }
   if (t->bases.length ())
     {