Re: Warn about virtual table mismatches

[Richard Biener <richard dot guenther at gmail dot com>]

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.