public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* Warn about virtual table mismatches
@ 2014-02-12  3:54 Jan Hubicka
  2014-02-12  4:22 ` Jason Merrill
  0 siblings, 1 reply; 6+ messages in thread
From: Jan Hubicka @ 2014-02-12  3:54 UTC (permalink / raw)
  To: gcc-patches, jason

Hi,
this patch implements warning when we merge two virtual tables that do not
match.  It compares the size and also go into fields.  I had to implement my
own comparsion code, since the vtables may subtly differ - in RTTI and also I
need to do so during DECL merging (since we will forget about the decl then)

Here is an example of real ODR violation in bugzilla it finds.
../../dist/include/mozilla/a11y/DocAccessible.h:40:0: warning: type �struct DocAccessible� violates one definition rule [enabled by default]
 class DocAccessible : public HyperTextAccessibleWrap,
 ^
/aux/hubicka/firefox/accessible/src/generic/DocAccessible.h:40:0: note: a type with the same name but different virtual table is defined in another translation unit
 class DocAccessible : public HyperTextAccessibleWrap,
 ^
/aux/hubicka/firefox/accessible/src/generic/DocAccessible.cpp:1232:0: note: the first different method is �HandleAccEvent�
 DocAccessible::HandleAccEvent(AccEvent* aEvent)
 ^
/aux/hubicka/firefox/accessible/src/atk/AccessibleWrap.cpp:956:0: note: a conflicting method is �HandleAccEvent�
 AccessibleWrap::HandleAccEvent(AccEvent* aEvent)
 ^

The patch is not terribly pretty, but does the job.  I will wait for few
days for comments/suggestins and then plan to commit it.

	PR lto/59468
	* lto/lto-symtab.c (lto_symtab_prevailing_decl): Check
	virtual tables for ODR violations.
	* ipa-devirt.c (compare_virtual_tables): New function.
	* ipa-utils.h (compare_virtual_tables): Declare.
Index: lto/lto-symtab.c
===================================================================
--- lto/lto-symtab.c	(revision 207702)
+++ lto/lto-symtab.c	(working copy)
@@ -679,5 +679,13 @@ lto_symtab_prevailing_decl (tree decl)
   if (!ret)
     return decl;
 
+  /* Check and report ODR violations on virtual tables.  */
+  if (decl != ret->decl && DECL_VIRTUAL_P (decl))
+    {
+      compare_virtual_tables (ret->decl, decl);
+      /* We are done with checking and DECL will die after merigng.  */
+      DECL_VIRTUAL_P (decl) = 0;
+    }
+
   return ret->decl;
 }
Index: ipa-devirt.c
===================================================================
--- ipa-devirt.c	(revision 207702)
+++ ipa-devirt.c	(working copy)
@@ -1675,6 +1673,101 @@ update_type_inheritance_graph (void)
 }
 
 
+/* Compare two virtual tables, PREVAILING and VTABLE and output ODR
+   violation warings.  */
+
+void
+compare_virtual_tables (tree prevailing, tree vtable)
+{
+  tree init1 = DECL_INITIAL (prevailing), init2 = DECL_INITIAL (vtable);
+  if (init1 == init2)
+    return;
+  if (init2 == error_mark_node)
+    return;
+  /* Be sure to keep virtual table contents even for external
+     vtables when they are available.  */
+  if (init1 == error_mark_node || !init1)
+    {
+      DECL_INITIAL (prevailing) = DECL_INITIAL (vtable);
+      return;
+    }
+  if (!init2 && DECL_EXTERNAL (vtable))
+    return;
+  if (DECL_VIRTUAL_P (prevailing) && init1 && init2
+      && CONSTRUCTOR_NELTS (init1) == CONSTRUCTOR_NELTS (init2))
+    {
+      unsigned i;
+      tree field1, field2;
+      tree val1, val2;
+      bool matched = true;
+
+      FOR_EACH_CONSTRUCTOR_ELT (CONSTRUCTOR_ELTS (init1),
+				 i, field1, val1)
+	{
+	  gcc_assert (!field1);
+	  field2 = CONSTRUCTOR_ELT (init2, i)->index;
+	  val2 = CONSTRUCTOR_ELT (init2, i)->value;
+	  gcc_assert (!field2);
+	  if (val2 == val1)
+	    continue;
+	  STRIP_NOPS (val1);
+	  STRIP_NOPS (val2);
+	  /* Unwind
+	      val <addr_expr type <pointer_type>
+	       readonly constant
+	       arg 0 <mem_ref type <pointer_type __vtbl_ptr_type>
+		   readonly
+		   arg 0 <addr_expr type <pointer_type>
+		       arg 0 <var_decl _ZTCSd0_Si>> arg 1 <integer_cst 24>>> */
+
+	  while ((TREE_CODE (val1) == MEM_REF
+	          && TREE_CODE (val2) == MEM_REF
+		  && (TREE_OPERAND (val1, 1)
+		      == TREE_OPERAND (val2, 1)))
+		 || (TREE_CODE (val1) == ADDR_EXPR
+		     && TREE_CODE (val2) == ADDR_EXPR))
+	    {
+	      val1 = TREE_OPERAND (val1, 0);
+	      val2 = TREE_OPERAND (val2, 0);
+	    }
+	  if (val1 == val2)
+	    continue;
+	  if (TREE_CODE (val2) == ADDR_EXPR)
+	    {
+	      tree tmp = val1;
+	      val1 = val2;
+	      val2 = tmp;
+            }
+	  /* Allow combining RTTI and non-RTTI is OK.  */
+	  if (TREE_CODE (val1) == ADDR_EXPR
+	      && TREE_CODE (TREE_OPERAND (val1, 0)) == VAR_DECL
+	      && !DECL_VIRTUAL_P (TREE_OPERAND (val1, 0))
+	      && TREE_CODE (val2) == NOP_EXPR
+	      && integer_zerop (TREE_OPERAND (val2, 0)))
+	    continue;
+	  if (TREE_CODE (val1) == NOP_EXPR
+	      && TREE_CODE (val2) == NOP_EXPR
+	      && integer_zerop (TREE_OPERAND (val1, 0))
+	      && integer_zerop (TREE_OPERAND (val2, 0)))
+	    continue;
+	  if (DECL_P (val1) && DECL_P (val2)
+	      && DECL_ASSEMBLER_NAME (val1) == DECL_ASSEMBLER_NAME (val2))
+	    continue;
+	  matched = false;
+	  break;
+	}
+      if (matched)
+	return;
+    }
+  odr_violation_reported = true;
+  if (warning_at (DECL_SOURCE_LOCATION (TYPE_NAME (DECL_CONTEXT (vtable))), 0,
+		     "type %qD violates one definition rule  ",
+		     DECL_CONTEXT (vtable)))
+    inform (DECL_SOURCE_LOCATION (TYPE_NAME (DECL_CONTEXT (prevailing))),
+	    "a type with the same name but different virtual table is "
+	    "defined in another translation unit");
+}
+
 /* Return true if N looks like likely target of a polymorphic call.
    Rule out cxa_pure_virtual, noreturns, function declared cold and
    other obvious cases.  */
Index: ipa-utils.h
===================================================================
--- ipa-utils.h	(revision 207702)
+++ ipa-utils.h	(working copy)
@@ -92,6 +92,7 @@ bool get_polymorphic_call_info_from_inva
 					       tree, tree, HOST_WIDE_INT);
 tree vtable_pointer_value_to_binfo (tree t);
 bool vtable_pointer_value_to_vtable (tree, tree *, unsigned HOST_WIDE_INT *);
+void compare_virtual_tables (tree, tree);
 
 /* Return vector containing possible targets of polymorphic call E.
    If FINALP is non-NULL, store true if the list is complette. 

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: Warn about virtual table mismatches
  2014-02-12  3:54 Warn about virtual table mismatches Jan Hubicka
@ 2014-02-12  4:22 ` Jason Merrill
  2014-02-12  6:27   ` Jan Hubicka
  0 siblings, 1 reply; 6+ messages in thread
From: Jason Merrill @ 2014-02-12  4:22 UTC (permalink / raw)
  To: Jan Hubicka, gcc-patches

On 02/11/2014 07:54 PM, Jan Hubicka wrote:
> +	  /* Allow combining RTTI and non-RTTI is OK.  */

You mean combining -frtti and -fno-rtti compiles?  Yes, that's fine, 
though you need to prefer the -frtti version in case code from that 
translation unit uses the RTTI info.

> /aux/hubicka/firefox/accessible/src/generic/DocAccessible.cpp:1232:0: note: the first different method is �HandleAccEvent�

I don't see where this note would come from in the patch.

Jason

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: Warn about virtual table mismatches
  2014-02-12  4:22 ` Jason Merrill
@ 2014-02-12  6:27   ` Jan Hubicka
  2014-02-12  8:01     ` Jason Merrill
                       ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Jan Hubicka @ 2014-02-12  6:27 UTC (permalink / raw)
  To: Jason Merrill; +Cc: Jan Hubicka, gcc-patches

> On 02/11/2014 07:54 PM, Jan Hubicka wrote:
> >+	  /* Allow combining RTTI and non-RTTI is OK.  */
> 
> You mean combining -frtti and -fno-rtti compiles?  Yes, that's fine,
> though you need to prefer the -frtti version in case code from that
> translation unit uses the RTTI info.

Is there some mechanism that linker will do so? At the moment we just chose variant
that would be selected by linker.  I can make the choice, but what happens with non-LTO?
> 
> >/aux/hubicka/firefox/accessible/src/generic/DocAccessible.cpp:1232:0: note: the first different method is �HandleAccEvent�
> 
> I don't see where this note would come from in the patch.
> 
Sorry, diffed old tree

Index: ipa-devirt.c
===================================================================
--- ipa-devirt.c	(revision 207702)
+++ ipa-devirt.c	(working copy)
@@ -1675,6 +1675,132 @@
 }
 
 
+/* Compare two virtual tables, PREVAILING and VTABLE and output ODR
+   violation warings.  */
+
+void
+compare_virtual_tables (tree prevailing, tree vtable)
+{
+  tree init1 = DECL_INITIAL (prevailing), init2 = DECL_INITIAL (vtable);
+  tree val1 = NULL, val2 = NULL;
+  if (!DECL_VIRTUAL_P (prevailing))
+    {
+      odr_violation_reported = true;
+      if (warning_at (DECL_SOURCE_LOCATION (prevailing), 0,
+			 "declaration %D conflict with a virtual table",
+			 prevailing))
+	inform (DECL_SOURCE_LOCATION (TYPE_NAME (DECL_CONTEXT (vtable))),
+		"a type defining the virtual table in another translation unit");
+      return;
+    }
+  if (init1 == init2 || init2 == error_mark_node)
+    return;
+  /* Be sure to keep virtual table contents even for external
+     vtables when they are available.  */
+  gcc_assert (init1 && init1 != error_mark_node);
+  if (!init2 && DECL_EXTERNAL (vtable))
+    return;
+  if (init1 && init2
+      && CONSTRUCTOR_NELTS (init1) == CONSTRUCTOR_NELTS (init2))
+    {
+      unsigned i;
+      tree field1, field2;
+      bool matched = true;
+
+      FOR_EACH_CONSTRUCTOR_ELT (CONSTRUCTOR_ELTS (init1),
+				 i, field1, val1)
+	{
+	  gcc_assert (!field1);
+	  field2 = CONSTRUCTOR_ELT (init2, i)->index;
+	  val2 = CONSTRUCTOR_ELT (init2, i)->value;
+	  gcc_assert (!field2);
+	  if (val2 == val1)
+	    continue;
+	  if (TREE_CODE (val1) == NOP_EXPR)
+	    val1 = TREE_OPERAND (val1, 0);
+	  if (TREE_CODE (val2) == NOP_EXPR)
+	    val2 = TREE_OPERAND (val2, 0);
+	  /* Unwind
+	      val <addr_expr type <pointer_type>
+	       readonly constant
+	       arg 0 <mem_ref type <pointer_type __vtbl_ptr_type>
+		   readonly
+		   arg 0 <addr_expr type <pointer_type>
+		       arg 0 <var_decl _ZTCSd0_Si>> arg 1 <integer_cst 24>>> */
+
+	  while (TREE_CODE (val1) == TREE_CODE (val2)
+		 && (((TREE_CODE (val1) == MEM_REF
+	               || TREE_CODE (val1) == POINTER_PLUS_EXPR)
+		      && (TREE_OPERAND (val1, 1))
+		        == TREE_OPERAND (val2, 1))
+		     || TREE_CODE (val1) == ADDR_EXPR))
+	    {
+	      val1 = TREE_OPERAND (val1, 0);
+	      val2 = TREE_OPERAND (val2, 0);
+	      if (TREE_CODE (val1) == NOP_EXPR)
+		val1 = TREE_OPERAND (val1, 0);
+	      if (TREE_CODE (val2) == NOP_EXPR)
+		val2 = TREE_OPERAND (val2, 0);
+	    }
+	  if (val1 == val2)
+	    continue;
+	  if (TREE_CODE (val2) == ADDR_EXPR)
+	    {
+	      tree tmp = val1;
+	      val1 = val2;
+	      val2 = tmp;
+            }
+	  /* Combining RTTI and non-RTTI vtables is OK.
+	     ??? Perhaps we can alsa (optionally) warn here?  */
+	  if (TREE_CODE (val1) == ADDR_EXPR
+	      && TREE_CODE (TREE_OPERAND (val1, 0)) == VAR_DECL
+	      && !DECL_VIRTUAL_P (TREE_OPERAND (val1, 0))
+	      && integer_zerop (val2))
+	    continue;
+	  /* For some reason zeros gets NOP_EXPR wrappers.  */
+	  if (integer_zerop (val1)
+	      && integer_zerop (val2))
+	    continue;
+	  /* Compare assembler names; this function is run during
+	     declaration merging.  */
+	  if (DECL_P (val1) && DECL_P (val2)
+	      && DECL_ASSEMBLER_NAME (val1) == DECL_ASSEMBLER_NAME (val2))
+	    continue;
+	  matched = false;
+	  break;
+	}
+      if (!matched)
+	{
+	  odr_violation_reported = true;
+	  if (warning_at (DECL_SOURCE_LOCATION (TYPE_NAME (DECL_CONTEXT (vtable))), 0,
+			     "type %qD violates one definition rule",
+			     DECL_CONTEXT (vtable)))
+	    {
+	      inform (DECL_SOURCE_LOCATION (TYPE_NAME (DECL_CONTEXT (prevailing))),
+		      "a type with the same name but different virtual table is "
+		      "defined in another translation unit");
+	      /* See if we can be more informative.  */
+	      if (val1 && val2 && TREE_CODE (val1) == FUNCTION_DECL
+		  && TREE_CODE (val1) == FUNCTION_DECL
+		  && !DECL_ARTIFICIAL (val1) && !DECL_ARTIFICIAL (val2))
+		{
+		  inform (DECL_SOURCE_LOCATION (val1),
+			  "the first different method is %qD", val1);
+		  inform (DECL_SOURCE_LOCATION (val2),
+			  "a conflicting method is %qD", val2);
+		}
+	    }
+	}
+      return;
+    }
+  if (warning_at (DECL_SOURCE_LOCATION (TYPE_NAME (DECL_CONTEXT (vtable))), 0,
+		     "type %qD violates one definition rule",
+		     DECL_CONTEXT (vtable)))
+    inform (DECL_SOURCE_LOCATION (TYPE_NAME (DECL_CONTEXT (prevailing))),
+	    "a type with the same name but number of virtual methods is "
+	    "defined in another translation unit");
+}
+
 /* Return true if N looks like likely target of a polymorphic call.
    Rule out cxa_pure_virtual, noreturns, function declared cold and
    other obvious cases.  */
Index: lto/lto-symtab.c
===================================================================
--- lto/lto-symtab.c	(revision 207701)
+++ lto/lto-symtab.c	(working copy)
@@ -679,5 +679,14 @@
   if (!ret)
     return decl;
 
+  /* Check and report ODR violations on virtual tables.  */
+  if (decl != ret->decl
+      && TREE_CODE (decl) == VAR_DECL && DECL_VIRTUAL_P (decl))
+    {
+      compare_virtual_tables (ret->decl, decl);
+      /* We are done with checking and DECL will die after merigng.  */
+      DECL_VIRTUAL_P (decl) = 0;
+    }
+
   return ret->decl;
 }
Index: ipa-utils.h
===================================================================
--- ipa-utils.h	(revision 207702)
+++ ipa-utils.h	(working copy)
@@ -92,6 +92,7 @@
 					       tree, tree, HOST_WIDE_INT);
 tree vtable_pointer_value_to_binfo (tree t);
 bool vtable_pointer_value_to_vtable (tree, tree *, unsigned HOST_WIDE_INT *);
+void compare_virtual_tables (tree, tree);
 
 /* Return vector containing possible targets of polymorphic call E.
    If FINALP is non-NULL, store true if the list is complette. 

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: Warn about virtual table mismatches
  2014-02-12  6:27   ` Jan Hubicka
@ 2014-02-12  8:01     ` Jason Merrill
  2014-02-12 11:21     ` Richard Biener
  2014-02-12 18:09     ` Bernhard Reutner-Fischer
  2 siblings, 0 replies; 6+ messages in thread
From: Jason Merrill @ 2014-02-12  8:01 UTC (permalink / raw)
  To: Jan Hubicka; +Cc: gcc-patches

On 02/11/2014 10:27 PM, Jan Hubicka wrote:
>> On 02/11/2014 07:54 PM, Jan Hubicka wrote:
>>> +	  /* Allow combining RTTI and non-RTTI is OK.  */
>>
>> You mean combining -frtti and -fno-rtti compiles?  Yes, that's fine,
>> though you need to prefer the -frtti version in case code from that
>> translation unit uses the RTTI info.
>
> Is there some mechanism that linker will do so? At the moment we just chose variant
> that would be selected by linker.  I can make the choice, but what happens with non-LTO?

Hmm, the linker might well make the wrong choice.  Might be worth 
warning about this as well.

> +	    "a type with the same name but number of virtual methods is "

"but different number"

Jason

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: Warn about virtual table mismatches
  2014-02-12  6:27   ` Jan Hubicka
  2014-02-12  8:01     ` Jason Merrill
@ 2014-02-12 11:21     ` Richard Biener
  2014-02-12 18:09     ` Bernhard Reutner-Fischer
  2 siblings, 0 replies; 6+ messages in thread
From: Richard Biener @ 2014-02-12 11:21 UTC (permalink / raw)
  To: Jan Hubicka; +Cc: Jason Merrill, GCC Patches

On Wed, Feb 12, 2014 at 7:27 AM, Jan Hubicka <hubicka@ucw.cz> wrote:
>> On 02/11/2014 07:54 PM, Jan Hubicka wrote:
>> >+      /* Allow combining RTTI and non-RTTI is OK.  */
>>
>> You mean combining -frtti and -fno-rtti compiles?  Yes, that's fine,
>> though you need to prefer the -frtti version in case code from that
>> translation unit uses the RTTI info.
>
> Is there some mechanism that linker will do so? At the moment we just chose variant
> that would be selected by linker.  I can make the choice, but what happens with non-LTO?
>>
>> >/aux/hubicka/firefox/accessible/src/generic/DocAccessible.cpp:1232:0: note: the first different method is �HandleAccEvent�
>>
>> I don't see where this note would come from in the patch.
>>
> Sorry, diffed old tree
>
> Index: ipa-devirt.c
> ===================================================================
> --- ipa-devirt.c        (revision 207702)
> +++ ipa-devirt.c        (working copy)
> @@ -1675,6 +1675,132 @@
>  }
>
>
> +/* Compare two virtual tables, PREVAILING and VTABLE and output ODR
> +   violation warings.  */
> +
> +void
> +compare_virtual_tables (tree prevailing, tree vtable)
> +{
> +  tree init1 = DECL_INITIAL (prevailing), init2 = DECL_INITIAL (vtable);
> +  tree val1 = NULL, val2 = NULL;
> +  if (!DECL_VIRTUAL_P (prevailing))
> +    {
> +      odr_violation_reported = true;
> +      if (warning_at (DECL_SOURCE_LOCATION (prevailing), 0,
> +                        "declaration %D conflict with a virtual table",
> +                        prevailing))
> +       inform (DECL_SOURCE_LOCATION (TYPE_NAME (DECL_CONTEXT (vtable))),
> +               "a type defining the virtual table in another translation unit");
> +      return;
> +    }
> +  if (init1 == init2 || init2 == error_mark_node)
> +    return;
> +  /* Be sure to keep virtual table contents even for external
> +     vtables when they are available.  */
> +  gcc_assert (init1 && init1 != error_mark_node);
> +  if (!init2 && DECL_EXTERNAL (vtable))
> +    return;
> +  if (init1 && init2
> +      && CONSTRUCTOR_NELTS (init1) == CONSTRUCTOR_NELTS (init2))
> +    {
> +      unsigned i;
> +      tree field1, field2;
> +      bool matched = true;
> +
> +      FOR_EACH_CONSTRUCTOR_ELT (CONSTRUCTOR_ELTS (init1),
> +                                i, field1, val1)
> +       {
> +         gcc_assert (!field1);
> +         field2 = CONSTRUCTOR_ELT (init2, i)->index;
> +         val2 = CONSTRUCTOR_ELT (init2, i)->value;
> +         gcc_assert (!field2);
> +         if (val2 == val1)
> +           continue;
> +         if (TREE_CODE (val1) == NOP_EXPR)
> +           val1 = TREE_OPERAND (val1, 0);
> +         if (TREE_CODE (val2) == NOP_EXPR)
> +           val2 = TREE_OPERAND (val2, 0);
> +         /* Unwind
> +             val <addr_expr type <pointer_type>
> +              readonly constant
> +              arg 0 <mem_ref type <pointer_type __vtbl_ptr_type>
> +                  readonly
> +                  arg 0 <addr_expr type <pointer_type>
> +                      arg 0 <var_decl _ZTCSd0_Si>> arg 1 <integer_cst 24>>> */
> +
> +         while (TREE_CODE (val1) == TREE_CODE (val2)
> +                && (((TREE_CODE (val1) == MEM_REF
> +                      || TREE_CODE (val1) == POINTER_PLUS_EXPR)
> +                     && (TREE_OPERAND (val1, 1))
> +                       == TREE_OPERAND (val2, 1))
> +                    || TREE_CODE (val1) == ADDR_EXPR))
> +           {
> +             val1 = TREE_OPERAND (val1, 0);
> +             val2 = TREE_OPERAND (val2, 0);
> +             if (TREE_CODE (val1) == NOP_EXPR)
> +               val1 = TREE_OPERAND (val1, 0);
> +             if (TREE_CODE (val2) == NOP_EXPR)
> +               val2 = TREE_OPERAND (val2, 0);
> +           }
> +         if (val1 == val2)
> +           continue;
> +         if (TREE_CODE (val2) == ADDR_EXPR)
> +           {
> +             tree tmp = val1;
> +             val1 = val2;
> +             val2 = tmp;
> +            }
> +         /* Combining RTTI and non-RTTI vtables is OK.
> +            ??? Perhaps we can alsa (optionally) warn here?  */
> +         if (TREE_CODE (val1) == ADDR_EXPR
> +             && TREE_CODE (TREE_OPERAND (val1, 0)) == VAR_DECL
> +             && !DECL_VIRTUAL_P (TREE_OPERAND (val1, 0))
> +             && integer_zerop (val2))
> +           continue;
> +         /* For some reason zeros gets NOP_EXPR wrappers.  */
> +         if (integer_zerop (val1)
> +             && integer_zerop (val2))
> +           continue;
> +         /* Compare assembler names; this function is run during
> +            declaration merging.  */
> +         if (DECL_P (val1) && DECL_P (val2)
> +             && DECL_ASSEMBLER_NAME (val1) == DECL_ASSEMBLER_NAME (val2))
> +           continue;
> +         matched = false;
> +         break;
> +       }
> +      if (!matched)
> +       {
> +         odr_violation_reported = true;
> +         if (warning_at (DECL_SOURCE_LOCATION (TYPE_NAME (DECL_CONTEXT (vtable))), 0,

Please don't add new warnings that cannot be turned off.  Maybe
add -Wodr?  (also use that on the two warnings emitted in lto-symtab.c).

Thanks,
Richard.

> +                            "type %qD violates one definition rule",
> +                            DECL_CONTEXT (vtable)))
> +           {
> +             inform (DECL_SOURCE_LOCATION (TYPE_NAME (DECL_CONTEXT (prevailing))),
> +                     "a type with the same name but different virtual table is "
> +                     "defined in another translation unit");
> +             /* See if we can be more informative.  */
> +             if (val1 && val2 && TREE_CODE (val1) == FUNCTION_DECL
> +                 && TREE_CODE (val1) == FUNCTION_DECL
> +                 && !DECL_ARTIFICIAL (val1) && !DECL_ARTIFICIAL (val2))
> +               {
> +                 inform (DECL_SOURCE_LOCATION (val1),
> +                         "the first different method is %qD", val1);
> +                 inform (DECL_SOURCE_LOCATION (val2),
> +                         "a conflicting method is %qD", val2);
> +               }
> +           }
> +       }
> +      return;
> +    }
> +  if (warning_at (DECL_SOURCE_LOCATION (TYPE_NAME (DECL_CONTEXT (vtable))), 0,
> +                    "type %qD violates one definition rule",
> +                    DECL_CONTEXT (vtable)))
> +    inform (DECL_SOURCE_LOCATION (TYPE_NAME (DECL_CONTEXT (prevailing))),
> +           "a type with the same name but number of virtual methods is "
> +           "defined in another translation unit");
> +}
> +
>  /* Return true if N looks like likely target of a polymorphic call.
>     Rule out cxa_pure_virtual, noreturns, function declared cold and
>     other obvious cases.  */
> Index: lto/lto-symtab.c
> ===================================================================
> --- lto/lto-symtab.c    (revision 207701)
> +++ lto/lto-symtab.c    (working copy)
> @@ -679,5 +679,14 @@
>    if (!ret)
>      return decl;
>
> +  /* Check and report ODR violations on virtual tables.  */
> +  if (decl != ret->decl
> +      && TREE_CODE (decl) == VAR_DECL && DECL_VIRTUAL_P (decl))
> +    {
> +      compare_virtual_tables (ret->decl, decl);
> +      /* We are done with checking and DECL will die after merigng.  */
> +      DECL_VIRTUAL_P (decl) = 0;
> +    }
> +
>    return ret->decl;
>  }
> Index: ipa-utils.h
> ===================================================================
> --- ipa-utils.h (revision 207702)
> +++ ipa-utils.h (working copy)
> @@ -92,6 +92,7 @@
>                                                tree, tree, HOST_WIDE_INT);
>  tree vtable_pointer_value_to_binfo (tree t);
>  bool vtable_pointer_value_to_vtable (tree, tree *, unsigned HOST_WIDE_INT *);
> +void compare_virtual_tables (tree, tree);
>
>  /* Return vector containing possible targets of polymorphic call E.
>     If FINALP is non-NULL, store true if the list is complette.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: Warn about virtual table mismatches
  2014-02-12  6:27   ` Jan Hubicka
  2014-02-12  8:01     ` Jason Merrill
  2014-02-12 11:21     ` Richard Biener
@ 2014-02-12 18:09     ` Bernhard Reutner-Fischer
  2 siblings, 0 replies; 6+ messages in thread
From: Bernhard Reutner-Fischer @ 2014-02-12 18:09 UTC (permalink / raw)
  To: Jan Hubicka, Jason Merrill; +Cc: gcc-patches

On 12 February 2014 07:27:59 Jan Hubicka <hubicka@ucw.cz> wrote:

> > On 02/11/2014 07:54 PM, Jan Hubicka wrote:
> > >+	  /* Allow combining RTTI and non-RTTI is OK.  */
> > You mean combining -frtti and -fno-rtti compiles?  Yes, that's fine,
> > though you need to prefer the -frtti version in case code from that
> > translation unit uses the RTTI info.
>
> Is there some mechanism that linker will do so? At the moment we just chose 
> variant
> that would be selected by linker.  I can make the choice, but what happens 
> with non-LTO?
> > >/aux/hubicka/firefox/accessible/src/generic/DocAccessible.cpp:1232:0: 
> note: the first different method is �HandleAccEvent�
> > I don't see where this note would come from in the patch.
> > Sorry, diffed old tree
>
> Index: ipa-devirt.c
> ===================================================================
> --- ipa-devirt.c	(revision 207702)
> +++ ipa-devirt.c	(working copy)
> @@ -1675,6 +1675,132 @@
>  }
>
>
> +/* Compare two virtual tables, PREVAILING and VTABLE and output ODR
> +   violation warings.  */
> +
> +void
> +compare_virtual_tables (tree prevailing, tree vtable)
> +{
> +  tree init1 = DECL_INITIAL (prevailing), init2 = DECL_INITIAL (vtable);
> +  tree val1 = NULL, val2 = NULL;
> +  if (!DECL_VIRTUAL_P (prevailing))
> +    {
> +      odr_violation_reported = true;
> +      if (warning_at (DECL_SOURCE_LOCATION (prevailing), 0,
> +			 "declaration %D conflict with a virtual table",
> +			 prevailing))
> +	inform (DECL_SOURCE_LOCATION (TYPE_NAME (DECL_CONTEXT (vtable))),
> +		"a type defining the virtual table in another translation unit");
> +      return;
> +    }
> +  if (init1 == init2 || init2 == error_mark_node)
> +    return;
> +  /* Be sure to keep virtual table contents even for external
> +     vtables when they are available.  */
> +  gcc_assert (init1 && init1 != error_mark_node);
> +  if (!init2 && DECL_EXTERNAL (vtable))
> +    return;
> +  if (init1 && init2
> +      && CONSTRUCTOR_NELTS (init1) == CONSTRUCTOR_NELTS (init2))
> +    {
> +      unsigned i;
> +      tree field1, field2;
> +      bool matched = true;
> +
> +      FOR_EACH_CONSTRUCTOR_ELT (CONSTRUCTOR_ELTS (init1),
> +				 i, field1, val1)
> +	{
> +	  gcc_assert (!field1);
> +	  field2 = CONSTRUCTOR_ELT (init2, i)->index;
> +	  val2 = CONSTRUCTOR_ELT (init2, i)->value;
> +	  gcc_assert (!field2);
> +	  if (val2 == val1)
> +	    continue;
> +	  if (TREE_CODE (val1) == NOP_EXPR)
> +	    val1 = TREE_OPERAND (val1, 0);
> +	  if (TREE_CODE (val2) == NOP_EXPR)
> +	    val2 = TREE_OPERAND (val2, 0);
> +	  /* Unwind
> +	      val <addr_expr type <pointer_type>
> +	       readonly constant
> +	       arg 0 <mem_ref type <pointer_type __vtbl_ptr_type>
> +		   readonly
> +		   arg 0 <addr_expr type <pointer_type>
> +		       arg 0 <var_decl _ZTCSd0_Si>> arg 1 <integer_cst 24>>> */
> +
> +	  while (TREE_CODE (val1) == TREE_CODE (val2)
> +		 && (((TREE_CODE (val1) == MEM_REF
> +	               || TREE_CODE (val1) == POINTER_PLUS_EXPR)
> +		      && (TREE_OPERAND (val1, 1))
> +		        == TREE_OPERAND (val2, 1))
> +		     || TREE_CODE (val1) == ADDR_EXPR))
> +	    {
> +	      val1 = TREE_OPERAND (val1, 0);
> +	      val2 = TREE_OPERAND (val2, 0);
> +	      if (TREE_CODE (val1) == NOP_EXPR)
> +		val1 = TREE_OPERAND (val1, 0);
> +	      if (TREE_CODE (val2) == NOP_EXPR)
> +		val2 = TREE_OPERAND (val2, 0);
> +	    }
> +	  if (val1 == val2)
> +	    continue;
> +	  if (TREE_CODE (val2) == ADDR_EXPR)
> +	    {
> +	      tree tmp = val1;
> +	      val1 = val2;
> +	      val2 = tmp;
> +            }
> +	  /* Combining RTTI and non-RTTI vtables is OK.
> +	     ??? Perhaps we can alsa (optionally) warn here?  */
> +	  if (TREE_CODE (val1) == ADDR_EXPR
> +	      && TREE_CODE (TREE_OPERAND (val1, 0)) == VAR_DECL
> +	      && !DECL_VIRTUAL_P (TREE_OPERAND (val1, 0))
> +	      && integer_zerop (val2))
> +	    continue;
> +	  /* For some reason zeros gets NOP_EXPR wrappers.  */
> +	  if (integer_zerop (val1)
> +	      && integer_zerop (val2))
> +	    continue;
> +	  /* Compare assembler names; this function is run during
> +	     declaration merging.  */
> +	  if (DECL_P (val1) && DECL_P (val2)
> +	      && DECL_ASSEMBLER_NAME (val1) == DECL_ASSEMBLER_NAME (val2))
> +	    continue;
> +	  matched = false;
> +	  break;
> +	}
> +      if (!matched)
> +	{
> +	  odr_violation_reported = true;
> +	  if (warning_at (DECL_SOURCE_LOCATION (TYPE_NAME (DECL_CONTEXT 
> (vtable))), 0,
> +			     "type %qD violates one definition rule",
> +			     DECL_CONTEXT (vtable)))
> +	    {
> +	      inform (DECL_SOURCE_LOCATION (TYPE_NAME (DECL_CONTEXT (prevailing))),
> +		      "a type with the same name but different virtual table is "
> +		      "defined in another translation unit");
> +	      /* See if we can be more informative.  */
> +	      if (val1 && val2 && TREE_CODE (val1) == FUNCTION_DECL
> +		  && TREE_CODE (val1) == FUNCTION_DECL

I suppose this second val1 ought to be val2.

> +		  && !DECL_ARTIFICIAL (val1) && !DECL_ARTIFICIAL (val2))
> +		{
> +		  inform (DECL_SOURCE_LOCATION (val1),
> +			  "the first different method is %qD", val1);
> +		  inform (DECL_SOURCE_LOCATION (val2),
> +			  "a conflicting method is %qD", val2);
> +		}
> +	    }
> +	}
> +      return;
> +    }
> +  if (warning_at (DECL_SOURCE_LOCATION (TYPE_NAME (DECL_CONTEXT (vtable))), 0,
> +		     "type %qD violates one definition rule",
> +		     DECL_CONTEXT (vtable)))
> +    inform (DECL_SOURCE_LOCATION (TYPE_NAME (DECL_CONTEXT (prevailing))),
> +	    "a type with the same name but number of virtual methods is "
> +	    "defined in another translation unit");
> +}
> +
>  /* Return true if N looks like likely target of a polymorphic call.
>     Rule out cxa_pure_virtual, noreturns, function declared cold and
>     other obvious cases.  */
> Index: lto/lto-symtab.c
> ===================================================================
> --- lto/lto-symtab.c	(revision 207701)
> +++ lto/lto-symtab.c	(working copy)
> @@ -679,5 +679,14 @@
>    if (!ret)
>      return decl;
>
> +  /* Check and report ODR violations on virtual tables.  */
> +  if (decl != ret->decl
> +      && TREE_CODE (decl) == VAR_DECL && DECL_VIRTUAL_P (decl))
> +    {
> +      compare_virtual_tables (ret->decl, decl);
> +      /* We are done with checking and DECL will die after merigng.  */

merging

Thanks,
> +      DECL_VIRTUAL_P (decl) = 0;
> +    }
> +
>    return ret->decl;
>  }
> Index: ipa-utils.h
> ===================================================================
> --- ipa-utils.h	(revision 207702)
> +++ ipa-utils.h	(working copy)
> @@ -92,6 +92,7 @@
>  					       tree, tree, HOST_WIDE_INT);
>  tree vtable_pointer_value_to_binfo (tree t);
>  bool vtable_pointer_value_to_vtable (tree, tree *, unsigned HOST_WIDE_INT *);
> +void compare_virtual_tables (tree, tree);
>
>  /* Return vector containing possible targets of polymorphic call E.
>     If FINALP is non-NULL, store true if the list is complette.



Sent with AquaMail for Android
http://www.aqua-mail.com


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2014-02-12 18:09 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-02-12  3:54 Warn about virtual table mismatches Jan Hubicka
2014-02-12  4:22 ` Jason Merrill
2014-02-12  6:27   ` Jan Hubicka
2014-02-12  8:01     ` Jason Merrill
2014-02-12 11:21     ` Richard Biener
2014-02-12 18:09     ` Bernhard Reutner-Fischer

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).