Re: Warn about virtual table mismatches
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
Re: Warn about virtual table mismatches
On Wed, Feb 12, 2014 at 7:27 AM, Jan Hubicka hubi...@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
Re: Warn about virtual table mismatches
On 12 February 2014 07:27:59 Jan Hubicka hubi...@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
+
Warn about virtual table mismatches
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
Re: Warn about virtual table mismatches
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
Re: Warn about virtual table mismatches
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);
+
