| Message ID | 20140212062745.GA11693@kam.mff.cuni.cz |
|---|---|
| State | New |
| Headers | show |
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
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.
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
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.