| Message ID | 20181121000454.xszsneee7irfywt5@kam.mff.cuni.cz |
|---|---|
| State | New |
| Headers | show |
| Series | Stream TREE_TYPE of TYPE_DECLs again | expand |
> PR lto/87957 > * tree.c (fld_decl_context): Break out from ... > (free_lang_data_in_decl): ... here; free TREE_PUBLIC, TREE_PRIVATE > DECL_ARTIFICIAL of TYPE_DECL; do not free TREE_TYPE of TYPE_DECL. > (fld_incomplete_type_of): Build copy of TYP_DECL. > * ipa-devirt.c (free_enum_values): Rename to ... > (free_odr_warning_data): ... this one; free also duplicated TYPE_DECLs > and TREE_TYPEs of TYPE_DECLs. Hi, and here are the testcases from the PR which I finally managed to annotate so they are accepted :) PR lto/87957 * g++.dg/lto/odr-1_0.C: Extend by mismatched enum. * g++.dg/lto/odr-1_1.C: Extend by mismatched enum. * g++.dg/lto/odr-2_0.C: New. * g++.dg/lto/odr-2_0.C: New. * g++.dg/lto/odr-3_1.C: New. * g++.dg/lto/odr-3_1.C: New. Index: g++.dg/lto/odr-1_0.C =================================================================== --- g++.dg/lto/odr-1_0.C (revision 266333) +++ g++.dg/lto/odr-1_0.C (working copy) @@ -1,8 +1,11 @@ // PR c++/82414 // { dg-lto-do link } +enum vals {aa,cc}; // { dg-lto-warning "6: type 'vals' violates the C\\+\\+ One Definition Rule" } struct a { // { dg-lto-warning "8: type 'struct a' violates the C\\+\\+ One Definition Rule" } struct b *ptr; // { dg-lto-message "13: the first difference of corresponding definitions is field 'ptr'" } + enum vals vals; }; -void test(struct a *) +void test(struct a *a) { + a->vals = cc; } Index: g++.dg/lto/odr-1_1.C =================================================================== --- g++.dg/lto/odr-1_1.C (revision 266333) +++ g++.dg/lto/odr-1_1.C (working copy) @@ -1,12 +1,16 @@ namespace { - struct b; + struct b; // { dg-lto-message "type 'struct b' defined in anonymous namespace can not match across the translation unit boundary" } } -struct a { - struct b *ptr; -}; +enum vals {aa,bb,cc}; // { dg-lto-message "an enum with different value name is defined in another translation unit" } +struct a { // { dg-lto-message "a different type is defined in another translation unit" } + struct b *ptr; // { dg-lto-message "a field of same name but different type is defined in another translation unit" } + enum vals vals; +} a; void test(struct a *); int main(void) { - test (0); + test (&a); + if (a.vals==aa) + return 1; } Index: g++.dg/lto/odr-2_0.C =================================================================== --- g++.dg/lto/odr-2_0.C (nonexistent) +++ g++.dg/lto/odr-2_0.C (working copy) @@ -0,0 +1,8 @@ +// { dg-lto-do link } +// { dg-lto-options { { -O0 -flto } } +enum a {} b; // { dg-lto-warning "6: type 'a' violates the C\\+\\+ One Definition Rule" } +int +main(void) +{ + return 0; +} Index: g++.dg/lto/odr-2_1.C =================================================================== --- g++.dg/lto/odr-2_1.C (nonexistent) +++ g++.dg/lto/odr-2_1.C (working copy) @@ -0,0 +1,4 @@ +class a { // { dg-lto-message "a different type is defined in another translation unit" } + int *b() const; +}; +int *a::b() const { return 0; } Index: g++.dg/lto/odr-3_0.C =================================================================== --- g++.dg/lto/odr-3_0.C (nonexistent) +++ g++.dg/lto/odr-3_0.C (working copy) @@ -0,0 +1,12 @@ +// { dg-lto-do link } +// { dg-lto-options { -O0 -flto } } + +typedef struct { + int a; // { dg-lto-message "the first difference of corresponding definitions is field 'a'" } +} YYSTYPE; // { dg-lto-warning "3: warning: type ‘struct YYSTYPE’ violates the C\\+\\+ One Definition Rule" } +union yyalloc { // { dg-lto-warning "7: type ‘union yyalloc’ violates the C\\+\\+ One Definition Rule" } + short yyss; + YYSTYPE yyvs; // { dg-lto-message "the first difference of corresponding definitions is field ‘yyvs’" } +}; +void b() { yyalloc c; } + Index: g++.dg/lto/odr-3_1.C =================================================================== --- g++.dg/lto/odr-3_1.C (nonexistent) +++ g++.dg/lto/odr-3_1.C (working copy) @@ -0,0 +1,9 @@ +typedef struct YYSTYPE { // { dg-lto-message ":16 a different type is defined in another translation unit" } +} YYSTYPE; +union yyalloc { + short yyss; + YYSTYPE yyvs; // { dg-lto-message "the first difference of corresponding definitions is field ‘yyvs’" } + +}; +void a() { yyalloc b; } +
On Wed, 21 Nov 2018, Jan Hubicka wrote: > Hi, > this patch recovers location infomration in the ODR warnings. > Because location info is not attached to types but corresponding > TYPE_DECLs, we need to prevent TYPE_DECLs to be merged when > corresponding types are not merged. > > To achieve this I no longer clear TREE_TYPE of TYPE_DECLs which > puts them back to the same SCC as the type itself. While making > incomplete type variant we need to produce copy of TYPE_DECL. Becuase > it is possible that TYPE_DECL was not processed by free lang data > we can not do copy_node but build it from scratch (because > the toplevel loops possibly processed all decls). This is not hard > to do, but made me notice few extra flags that are streamed for > TYPE_DECLs and free_lang_data is not seeing them. > > I have also extended ipa-devirt to get rid of the duplicated decls > once ODR warnings are done to save ltrans streaming (it actually > added about 10% of ltrans data w/o this change) > > I have checked that the patch does not increase number of type > duplicates for cc1 (24), I will also re-do testing for Firefox > which may uncover some extra flags/attributes to care about. > > lto-bootstrapped/regtested x86_64-linux OK? OK if you put a comment ... > Honza > > PR lto/87957 > * tree.c (fld_decl_context): Break out from ... > (free_lang_data_in_decl): ... here; free TREE_PUBLIC, TREE_PRIVATE > DECL_ARTIFICIAL of TYPE_DECL; do not free TREE_TYPE of TYPE_DECL. > (fld_incomplete_type_of): Build copy of TYP_DECL. > * ipa-devirt.c (free_enum_values): Rename to ... > (free_odr_warning_data): ... this one; free also duplicated TYPE_DECLs > and TREE_TYPEs of TYPE_DECLs. > > Index: tree.c > =================================================================== > --- tree.c (revision 266325) > +++ tree.c (working copy) > @@ -5206,6 +5206,24 @@ fld_process_array_type (tree t, tree t2, > return array; > } > > +/* Return CTX after removal of contexts that are not relevant */ > + > +static tree > +fld_decl_context (tree ctx) > +{ > + /* Variably modified types are needed for tree_is_indexable to decide > + whether the type needs to go to local or global section. > + This code is semi-broken but for now it is easiest to keep contexts > + as expected. */ > + if (ctx && TYPE_P (ctx) > + && !variably_modified_type_p (ctx, NULL_TREE)) > + { > + while (ctx && TYPE_P (ctx)) > + ctx = TYPE_CONTEXT (ctx); > + } > + return ctx; > +} > + > /* For T being aggregate type try to turn it into a incomplete variant. > Return T if no simplification is possible. */ > > @@ -5267,6 +5285,27 @@ fld_incomplete_type_of (tree t, struct f > } > else > TYPE_VALUES (copy) = NULL; > + > + /* Build copy of TYPE_DECL in TYPE_NAME if necessary. > + This is needed for ODR violation warnings to come out right (we > + want duplicate TYPE_DECLs whenever the type is duplicated because > + of ODR violation. Because lang data in the TYPE_DECL may not > + have been freed yet, rebuild it from scratch and copy relevant > + fields. */ > + TYPE_NAME (copy) = fld_simplified_type_name (copy); > + tree name = TYPE_NAME (copy); > + > + if (name && TREE_CODE (name) == TYPE_DECL) > + { > + gcc_checking_assert (TREE_TYPE (name) == t); > + tree name2 = build_decl (DECL_SOURCE_LOCATION (name), TYPE_DECL, > + DECL_NAME (name), copy); > + SET_DECL_ASSEMBLER_NAME (name2, DECL_ASSEMBLER_NAME (name)); > + SET_DECL_ALIGN (name2, 0); > + DECL_CONTEXT (name2) = fld_decl_context > + (DECL_CONTEXT (name)); > + TYPE_NAME (copy) = name2; > + } > } > return copy; > } > @@ -5649,12 +5688,13 @@ free_lang_data_in_decl (tree decl, struc > { > DECL_VISIBILITY (decl) = VISIBILITY_DEFAULT; > DECL_VISIBILITY_SPECIFIED (decl) = 0; > - /* TREE_PUBLIC is used to tell if type is anonymous. */ > + TREE_PUBLIC (decl) = 0; > + TREE_PRIVATE (decl) = 0; > + DECL_ARTIFICIAL (decl) = 0; > TYPE_DECL_SUPPRESS_DEBUG (decl) = 0; > DECL_INITIAL (decl) = NULL_TREE; > DECL_ORIGINAL_TYPE (decl) = NULL_TREE; > DECL_MODE (decl) = VOIDmode; > - TREE_TYPE (decl) = void_type_node; > SET_DECL_ALIGN (decl, 0); > } > else if (TREE_CODE (decl) == FIELD_DECL) > @@ -5688,20 +5728,7 @@ free_lang_data_in_decl (tree decl, struc > if (TREE_CODE (decl) != FIELD_DECL > && ((TREE_CODE (decl) != VAR_DECL && TREE_CODE (decl) != FUNCTION_DECL) > || !DECL_VIRTUAL_P (decl))) > - { > - tree ctx = DECL_CONTEXT (decl); > - /* Variably modified types are needed for tree_is_indexable to decide > - whether the type needs to go to local or global section. > - This code is semi-broken but for now it is easiest to keep contexts > - as expected. */ > - if (ctx && TYPE_P (ctx) > - && !variably_modified_type_p (ctx, NULL_TREE)) > - { > - while (ctx && TYPE_P (ctx)) > - ctx = TYPE_CONTEXT (ctx); > - DECL_CONTEXT (decl) = ctx; > - } > - } > + DECL_CONTEXT (decl) = fld_decl_context (DECL_CONTEXT (decl)); > } > > > Index: ipa-devirt.c > =================================================================== > --- ipa-devirt.c (revision 266325) > +++ ipa-devirt.c (working copy) > @@ -2289,27 +2289,39 @@ dump_type_inheritance_graph (FILE *f) > "%i duplicates overall\n", num_all_types, num_types, num_duplicates); > } > > -/* Save some WPA->ltrans streaming by freeing enum values. */ > +/* Save some WPA->ltrans streaming by freeing stuff needed only for good > + ODR warnings. */ > > static void > -free_enum_values () > +free_odr_warning_data () > { > - static bool enum_values_freed = false; > - if (enum_values_freed || !flag_wpa || !odr_types_ptr) > + static bool odr_data_freed = false; > + > + if (odr_data_freed || !flag_wpa || !odr_types_ptr) > return; > - enum_values_freed = true; > - unsigned int i; > - for (i = 0; i < odr_types.length (); i++) > + > + odr_data_freed = true; > + > + for (unsigned int i = 0; i < odr_types.length (); i++) > if (odr_types[i]) > { > - if (TREE_CODE (odr_types[i]->type) == ENUMERAL_TYPE) > - TYPE_VALUES (odr_types[i]->type) = NULL; > + tree t = odr_types[i]->type; > + > + if (TREE_CODE (t) == ENUMERAL_TYPE) > + TYPE_VALUES (t) = NULL; > + TREE_TYPE (TYPE_NAME (t)) = void_type_node; ... here explaining the late "free-lang-data". Richard. > + > if (odr_types[i]->types) > for (unsigned int j = 0; j < odr_types[i]->types->length (); j++) > - if (TREE_CODE ((*odr_types[i]->types)[j]) == ENUMERAL_TYPE) > - TYPE_VALUES ((*odr_types[i]->types)[j]) = NULL; > + { > + tree td = (*odr_types[i]->types)[j]; > + > + if (TREE_CODE (td) == ENUMERAL_TYPE) > + TYPE_VALUES (td) = NULL; > + TYPE_NAME (td) = TYPE_NAME (t); > + } > } > - enum_values_freed = true; > + odr_data_freed = true; > } > > /* Initialize IPA devirt and build inheritance tree graph. */ > @@ -2323,7 +2335,7 @@ build_type_inheritance_graph (void) > > if (odr_hash) > { > - free_enum_values (); > + free_odr_warning_data (); > return; > } > timevar_push (TV_IPA_INHERITANCE); > @@ -2370,7 +2382,7 @@ build_type_inheritance_graph (void) > dump_type_inheritance_graph (inheritance_dump_file); > dump_end (TDI_inheritance, inheritance_dump_file); > } > - free_enum_values (); > + free_odr_warning_data (); > timevar_pop (TV_IPA_INHERITANCE); > } > > >
> > OK if you put a comment ... I have adde comments to both free_lang_data referring that some fields are freed late and comment to the new freeing pass. While testing I noticed stupid bug in need_assembler_name_p which in case TYPE_DECL does not satisfy the elaborate conditional for type to be ODR it falls into "return true" rather than false. Fixing that uncovered bug in -fno-odr-type-merging path of ipa-devirt where vtable hash was no longer initialized. Fixed thus. lto-bootstrapped/regtested x86_64-linux, comitted. PR lto/87957 * tree.c (fld_decl_context): Break out from ... (free_lang_data_in_decl): ... here; free TREE_PUBLIC, TREE_PRIVATE DECL_ARTIFICIAL of TYPE_DECL; do not free TREE_TYPE of TYPE_DECL. (fld_incomplete_type_of): Build copy of TYP_DECL. * ipa-devirt.c (free_enum_values): Rename to ... (free_odr_warning_data): ... this one; free also duplicated TYPE_DECLs and TREE_TYPEs of TYPE_DECLs. (get_odr_type): Initialize odr_vtable_hash if needed. Index: ipa-devirt.c =================================================================== --- ipa-devirt.c (revision 266334) +++ ipa-devirt.c (working copy) @@ -2025,6 +2025,8 @@ get_odr_type (tree type, bool insert) if ((!slot || !*slot) && in_lto_p && can_be_vtable_hashed_p (type)) { hash = hash_odr_vtable (type); + if (!odr_vtable_hash) + odr_vtable_hash = new odr_vtable_hash_type (23); vtable_slot = odr_vtable_hash->find_slot_with_hash (type, hash, insert ? INSERT : NO_INSERT); } @@ -2289,27 +2291,43 @@ dump_type_inheritance_graph (FILE *f) "%i duplicates overall\n", num_all_types, num_types, num_duplicates); } -/* Save some WPA->ltrans streaming by freeing enum values. */ +/* Save some WPA->ltrans streaming by freeing stuff needed only for good + ODR warnings. + We free TYPE_VALUES of enums and also make TYPE_DECLs to not point back + to the type (which is needed to keep them in the same SCC and preserve + location information to output warnings) and subsequently we make all + TYPE_DECLS of same assembler name equivalent. */ static void -free_enum_values () +free_odr_warning_data () { - static bool enum_values_freed = false; - if (enum_values_freed || !flag_wpa || !odr_types_ptr) + static bool odr_data_freed = false; + + if (odr_data_freed || !flag_wpa || !odr_types_ptr) return; - enum_values_freed = true; - unsigned int i; - for (i = 0; i < odr_types.length (); i++) + + odr_data_freed = true; + + for (unsigned int i = 0; i < odr_types.length (); i++) if (odr_types[i]) { - if (TREE_CODE (odr_types[i]->type) == ENUMERAL_TYPE) - TYPE_VALUES (odr_types[i]->type) = NULL; + tree t = odr_types[i]->type; + + if (TREE_CODE (t) == ENUMERAL_TYPE) + TYPE_VALUES (t) = NULL; + TREE_TYPE (TYPE_NAME (t)) = void_type_node; + if (odr_types[i]->types) for (unsigned int j = 0; j < odr_types[i]->types->length (); j++) - if (TREE_CODE ((*odr_types[i]->types)[j]) == ENUMERAL_TYPE) - TYPE_VALUES ((*odr_types[i]->types)[j]) = NULL; + { + tree td = (*odr_types[i]->types)[j]; + + if (TREE_CODE (td) == ENUMERAL_TYPE) + TYPE_VALUES (td) = NULL; + TYPE_NAME (td) = TYPE_NAME (t); + } } - enum_values_freed = true; + odr_data_freed = true; } /* Initialize IPA devirt and build inheritance tree graph. */ @@ -2323,7 +2341,7 @@ build_type_inheritance_graph (void) if (odr_hash) { - free_enum_values (); + free_odr_warning_data (); return; } timevar_push (TV_IPA_INHERITANCE); @@ -2370,7 +2388,7 @@ build_type_inheritance_graph (void) dump_type_inheritance_graph (inheritance_dump_file); dump_end (TDI_inheritance, inheritance_dump_file); } - free_enum_values (); + free_odr_warning_data (); timevar_pop (TV_IPA_INHERITANCE); } Index: tree.c =================================================================== --- tree.c (revision 266325) +++ tree.c (working copy) @@ -5206,6 +5206,24 @@ fld_process_array_type (tree t, tree t2, return array; } +/* Return CTX after removal of contexts that are not relevant */ + +static tree +fld_decl_context (tree ctx) +{ + /* Variably modified types are needed for tree_is_indexable to decide + whether the type needs to go to local or global section. + This code is semi-broken but for now it is easiest to keep contexts + as expected. */ + if (ctx && TYPE_P (ctx) + && !variably_modified_type_p (ctx, NULL_TREE)) + { + while (ctx && TYPE_P (ctx)) + ctx = TYPE_CONTEXT (ctx); + } + return ctx; +} + /* For T being aggregate type try to turn it into a incomplete variant. Return T if no simplification is possible. */ @@ -5267,6 +5285,28 @@ fld_incomplete_type_of (tree t, struct f } else TYPE_VALUES (copy) = NULL; + + /* Build copy of TYPE_DECL in TYPE_NAME if necessary. + This is needed for ODR violation warnings to come out right (we + want duplicate TYPE_DECLs whenever the type is duplicated because + of ODR violation. Because lang data in the TYPE_DECL may not + have been freed yet, rebuild it from scratch and copy relevant + fields. */ + TYPE_NAME (copy) = fld_simplified_type_name (copy); + tree name = TYPE_NAME (copy); + + if (name && TREE_CODE (name) == TYPE_DECL) + { + gcc_checking_assert (TREE_TYPE (name) == t); + tree name2 = build_decl (DECL_SOURCE_LOCATION (name), TYPE_DECL, + DECL_NAME (name), copy); + if (DECL_ASSEMBLER_NAME_SET_P (name)) + SET_DECL_ASSEMBLER_NAME (name2, DECL_ASSEMBLER_NAME (name)); + SET_DECL_ALIGN (name2, 0); + DECL_CONTEXT (name2) = fld_decl_context + (DECL_CONTEXT (name)); + TYPE_NAME (copy) = name2; + } } return copy; } @@ -5423,7 +5463,8 @@ free_lang_data_in_type (tree type, struc if (TREE_CODE (type) == ENUMERAL_TYPE) { /* Type values are used only for C++ ODR checking. Drop them - for all type variants and non-ODR types. */ + for all type variants and non-ODR types. + For ODR types the data is freed in free_odr_warning_data. */ if (TYPE_MAIN_VARIANT (type) != type || !type_with_linkage_p (type)) TYPE_VALUES (type) = NULL; @@ -5455,11 +5496,7 @@ free_lang_data_in_type (tree type, struc TYPE_CONTEXT (type) = ctx; } - /* Drop TYPE_DECLs in TYPE_NAME in favor of the identifier in the - TYPE_DECL if the type doesn't have linkage. - this must match fld_ */ - if (type != TYPE_MAIN_VARIANT (type) || ! type_with_linkage_p (type)) - TYPE_STUB_DECL (type) = NULL; + TYPE_STUB_DECL (type) = NULL; TYPE_NAME (type) = fld_simplified_type_name (type); } @@ -5486,16 +5523,19 @@ need_assembler_name_p (tree decl) e.g. -fno-signed-char/-fsigned-char mismatches to be handled well. See cp/mangle.c:write_builtin_type for details. */ - if (flag_lto_odr_type_mering - && TREE_CODE (decl) == TYPE_DECL - && DECL_NAME (decl) - && decl == TYPE_NAME (TREE_TYPE (decl)) - && TYPE_MAIN_VARIANT (TREE_TYPE (decl)) == TREE_TYPE (decl) - && !TYPE_ARTIFICIAL (TREE_TYPE (decl)) - && (type_with_linkage_p (TREE_TYPE (decl)) - || TREE_CODE (TREE_TYPE (decl)) == INTEGER_TYPE) - && !variably_modified_type_p (TREE_TYPE (decl), NULL_TREE)) - return !DECL_ASSEMBLER_NAME_SET_P (decl); + if (TREE_CODE (decl) == TYPE_DECL) + { + if (flag_lto_odr_type_mering + && DECL_NAME (decl) + && decl == TYPE_NAME (TREE_TYPE (decl)) + && TYPE_MAIN_VARIANT (TREE_TYPE (decl)) == TREE_TYPE (decl) + && !TYPE_ARTIFICIAL (TREE_TYPE (decl)) + && (type_with_linkage_p (TREE_TYPE (decl)) + || TREE_CODE (TREE_TYPE (decl)) == INTEGER_TYPE) + && !variably_modified_type_p (TREE_TYPE (decl), NULL_TREE)) + return !DECL_ASSEMBLER_NAME_SET_P (decl); + return false; + } /* Only FUNCTION_DECLs and VAR_DECLs are considered. */ if (!VAR_OR_FUNCTION_DECL_P (decl)) return false; @@ -5649,13 +5689,15 @@ free_lang_data_in_decl (tree decl, struc { DECL_VISIBILITY (decl) = VISIBILITY_DEFAULT; DECL_VISIBILITY_SPECIFIED (decl) = 0; - /* TREE_PUBLIC is used to tell if type is anonymous. */ + TREE_PUBLIC (decl) = 0; + TREE_PRIVATE (decl) = 0; + DECL_ARTIFICIAL (decl) = 0; TYPE_DECL_SUPPRESS_DEBUG (decl) = 0; DECL_INITIAL (decl) = NULL_TREE; DECL_ORIGINAL_TYPE (decl) = NULL_TREE; DECL_MODE (decl) = VOIDmode; - TREE_TYPE (decl) = void_type_node; SET_DECL_ALIGN (decl, 0); + /* TREE_TYPE is cleared at WPA time in free_odr_warning_data. */ } else if (TREE_CODE (decl) == FIELD_DECL) { @@ -5688,20 +5730,7 @@ free_lang_data_in_decl (tree decl, struc if (TREE_CODE (decl) != FIELD_DECL && ((TREE_CODE (decl) != VAR_DECL && TREE_CODE (decl) != FUNCTION_DECL) || !DECL_VIRTUAL_P (decl))) - { - tree ctx = DECL_CONTEXT (decl); - /* Variably modified types are needed for tree_is_indexable to decide - whether the type needs to go to local or global section. - This code is semi-broken but for now it is easiest to keep contexts - as expected. */ - if (ctx && TYPE_P (ctx) - && !variably_modified_type_p (ctx, NULL_TREE)) - { - while (ctx && TYPE_P (ctx)) - ctx = TYPE_CONTEXT (ctx); - DECL_CONTEXT (decl) = ctx; - } - } + DECL_CONTEXT (decl) = fld_decl_context (DECL_CONTEXT (decl)); }
* g++.dg/lto/odr-2_0.C: Remove extra brace
diff --git a/gcc/testsuite/g++.dg/lto/odr-2_0.C b/gcc/testsuite/g++.dg/lto/odr-2_0.C
index 222fa2c1db..3ebb49efa2 100644
--- a/gcc/testsuite/g++.dg/lto/odr-2_0.C
+++ b/gcc/testsuite/g++.dg/lto/odr-2_0.C
@@ -1,5 +1,5 @@
// { dg-lto-do link }
-// { dg-lto-options { { -O0 -flto } }
+// { dg-lto-options { -O0 -flto } }
enum a {} b; // { dg-lto-warning "6: type 'a' violates the C\\+\\+ One Definition Rule" }
int
main(void)
> * g++.dg/lto/odr-2_0.C: Remove extra brace > > diff --git a/gcc/testsuite/g++.dg/lto/odr-2_0.C b/gcc/testsuite/g++.dg/lto/odr-2_0.C > index 222fa2c1db..3ebb49efa2 100644 > --- a/gcc/testsuite/g++.dg/lto/odr-2_0.C > +++ b/gcc/testsuite/g++.dg/lto/odr-2_0.C > @@ -1,5 +1,5 @@ > // { dg-lto-do link } > -// { dg-lto-options { { -O0 -flto } } > +// { dg-lto-options { -O0 -flto } } Doesn't this make the testcase to be run twice, once with -O0 and second time with -flto rather than running it once with -O0 -flto? Honza > enum a {} b; // { dg-lto-warning "6: type 'a' violates the C\\+\\+ One Definition Rule" } > int > main(void) > -- > 2.19.1 > > -- > Andreas Schwab, SUSE Labs, schwab@suse.de > GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7 > "And now for something completely different."
On Nov 22 2018, Jan Hubicka <hubicka@ucw.cz> wrote: >> * g++.dg/lto/odr-2_0.C: Remove extra brace >> >> diff --git a/gcc/testsuite/g++.dg/lto/odr-2_0.C b/gcc/testsuite/g++.dg/lto/odr-2_0.C >> index 222fa2c1db..3ebb49efa2 100644 >> --- a/gcc/testsuite/g++.dg/lto/odr-2_0.C >> +++ b/gcc/testsuite/g++.dg/lto/odr-2_0.C >> @@ -1,5 +1,5 @@ >> // { dg-lto-do link } >> -// { dg-lto-options { { -O0 -flto } } >> +// { dg-lto-options { -O0 -flto } } > > Doesn't this make the testcase to be run twice, once with -O0 and second > time with -flto rather than running it once with -O0 -flto? It matches what odr-3_0.C does. Andreas.
On November 22, 2018 5:30:14 PM GMT+01:00, Jan Hubicka <hubicka@ucw.cz> wrote: >> * g++.dg/lto/odr-2_0.C: Remove extra brace >> >> diff --git a/gcc/testsuite/g++.dg/lto/odr-2_0.C >b/gcc/testsuite/g++.dg/lto/odr-2_0.C >> index 222fa2c1db..3ebb49efa2 100644 >> --- a/gcc/testsuite/g++.dg/lto/odr-2_0.C >> +++ b/gcc/testsuite/g++.dg/lto/odr-2_0.C >> @@ -1,5 +1,5 @@ >> // { dg-lto-do link } >> -// { dg-lto-options { { -O0 -flto } } >> +// { dg-lto-options { -O0 -flto } } > >Doesn't this make the testcase to be run twice, once with -O0 and >second >time with -flto rather than running it once with -O0 -flto? Yes. >Honza > >> enum a {} b; // { dg-lto-warning "6: type 'a' violates the C\\+\\+ >One Definition Rule" } >> int >> main(void) >> -- >> 2.19.1 >> >> -- >> Andreas Schwab, SUSE Labs, schwab@suse.de >> GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA >B9D7 >> "And now for something completely different."
> On November 22, 2018 5:30:14 PM GMT+01:00, Jan Hubicka <hubicka@ucw.cz> wrote: > >> * g++.dg/lto/odr-2_0.C: Remove extra brace > >> > >> diff --git a/gcc/testsuite/g++.dg/lto/odr-2_0.C > >b/gcc/testsuite/g++.dg/lto/odr-2_0.C > >> index 222fa2c1db..3ebb49efa2 100644 > >> --- a/gcc/testsuite/g++.dg/lto/odr-2_0.C > >> +++ b/gcc/testsuite/g++.dg/lto/odr-2_0.C > >> @@ -1,5 +1,5 @@ > >> // { dg-lto-do link } > >> -// { dg-lto-options { { -O0 -flto } } > >> +// { dg-lto-options { -O0 -flto } } > > > >Doesn't this make the testcase to be run twice, once with -O0 and > >second > >time with -flto rather than running it once with -O0 -flto? > > Yes. Actually it would be useful to have ODR tested with optimization on because streaming is somewhat optimization level specific. I will rework the testcases today so they do not need dg-lto-options Honza
On Thu, 22 Nov 2018 at 19:14, Jan Hubicka <hubicka@ucw.cz> wrote: > > > On November 22, 2018 5:30:14 PM GMT+01:00, Jan Hubicka <hubicka@ucw.cz> wrote: > > >> * g++.dg/lto/odr-2_0.C: Remove extra brace > > >> > > >> diff --git a/gcc/testsuite/g++.dg/lto/odr-2_0.C > > >b/gcc/testsuite/g++.dg/lto/odr-2_0.C > > >> index 222fa2c1db..3ebb49efa2 100644 > > >> --- a/gcc/testsuite/g++.dg/lto/odr-2_0.C > > >> +++ b/gcc/testsuite/g++.dg/lto/odr-2_0.C > > >> @@ -1,5 +1,5 @@ > > >> // { dg-lto-do link } > > >> -// { dg-lto-options { { -O0 -flto } } > > >> +// { dg-lto-options { -O0 -flto } } > > > > > >Doesn't this make the testcase to be run twice, once with -O0 and > > >second > > >time with -flto rather than running it once with -O0 -flto? > > > > Yes. > > Actually it would be useful to have ODR tested with optimization on > because streaming is somewhat optimization level specific. I will rework > the testcases today so they do not need dg-lto-options > At least the extra { or missing } causes Tcl errors: ERROR: tcl error sourcing /gcc/testsuite/g++.dg/lto/lto.exp. ERROR: unmatched open brace in list while executing "foreach op $tmp { set cmd [lindex $op 0] verbose "cmd is $cmd" if { [string match "dg-skip-if" $cmd] || [string match "dg-require-*" $cmd] } { ..." (procedure "lto-get-options-main" line 26) > Honza
Hi,
I have tweaked the testcases as follows. They now work with -O2
and additionally test better for locations of individual warnings.
The output is as follows:
../../gcc/testsuite/g++.dg/lto/odr-3_0.C:5:3: warning: type ‘struct YYSTYPE’ violates the C++ One Definition Rule [-Wodr]
5 | } YYSTYPE; // { dg-lto-message "violates the C\\+\\+ One Definition Rule" 2 }
| ^
../../gcc/testsuite/g++.dg/lto/odr-3_1.C:1:16: note: a different type is defined in another translation unit
1 | typedef struct YYSTYPE { // { dg-lto-message "type" 2 }
| ^
../../gcc/testsuite/g++.dg/lto/odr-3_0.C:4:7: note: the first difference of corresponding definitions is field ‘a’
4 | int a; // { dg-lto-message "the first difference of corresponding definitions is field 'a'" }
| ^
../../gcc/testsuite/g++.dg/lto/odr-3_1.C:1:16: note: a type with different number of fields is defined in another translation unit
1 | typedef struct YYSTYPE { // { dg-lto-message "type" 2 }
| ^
../../gcc/testsuite/g++.dg/lto/odr-3_0.C:9:7: warning: type ‘union yyalloc’ violates the C++ One Definition Rule [-Wodr]
9 | union yyalloc { // { dg-lto-warning "7: type 'union yyalloc' violates the C\\+\\+ One Definition Rule" }
| ^
../../gcc/testsuite/g++.dg/lto/odr-3_1.C:6:7: note: a different type is defined in another translation unit
6 | union yyalloc { // { dg-lto-message "type" 2 }
| ^
../../gcc/testsuite/g++.dg/lto/odr-3_0.C:11:11: note: the first difference of corresponding definitions is field ‘yyvs’
11 | YYSTYPE yyvs; // { dg-lto-message "the first difference of corresponding definitions is field 'yyvs'" }
| ^
../../gcc/testsuite/g++.dg/lto/odr-3_1.C:11:11: note: a field of same name but different type is defined in another translation unit
11 | YYSTYPE yyvs; // { dg-lto-message "field of same name but different type is defined in another translation unit" }
| ^
../../gcc/testsuite/g++.dg/lto/odr-3_0.C:5:3: note: type ‘struct YYSTYPE’ itself violates the C++ One Definition Rule
5 | } YYSTYPE; // { dg-lto-message "violates the C\\+\\+ One Definition Rule" 2 }
| ^
../../gcc/testsuite/g++.dg/lto/odr-3_0.C:13:16: warning: ‘a’ violates the C++ One Definition Rule [-Wodr]
13 | extern yyalloc a; // { dg-lto-warning "16: 'a' violates the C\\+\\+ One Definition Rule" }
| ^
../../gcc/testsuite/g++.dg/lto/odr-3_1.C:6:7: note: type ‘union yyalloc’ itself violates the C++ One Definition Rule
6 | union yyalloc { // { dg-lto-message "type" 2 }
| ^
../../gcc/testsuite/g++.dg/lto/odr-3_0.C:9:7: note: the incompatible type is defined here
9 | union yyalloc { // { dg-lto-warning "7: type 'union yyalloc' violates the C\\+\\+ One Definition Rule" }
| ^
../../gcc/testsuite/g++.dg/lto/odr-3_1.C:15:9: note: ‘a’ was previously declared here
15 | yyalloc a; // { dg-lto-message "'a' was previously declared here" }
| ^
../../gcc/testsuite/g++.dg/lto/odr-3_1.C:15:9: note: code may be misoptimized unless -fno-strict-aliasing is used
which I hope is informative enough. One problem is that I not know how
to pattern match warning and note or multiple notes pointing to single
location, so I have added somewhat generic dg-message patterns. Help
would be apprechiated.
I think ODR warnings should be in reasonable shape and it would be nice
to have more testsuite coverage. I find it very time consuming to write
the pattern matching, hope I will improve with time :)
The reason for verbosity is that they are often very cryptic to analyze
when difference comes from some ifdefs or other things not obvious in
the sources.
Honza
* g++.dg/lto/odr-2_0.C: Drop dg-lto-options.
* g++.dg/lto/odr-3_0.C: Likewise; harden for optimizing compilatoin.
Index: g++.dg/lto/odr-2_0.C
===================================================================
--- g++.dg/lto/odr-2_0.C (revision 266382)
+++ g++.dg/lto/odr-2_0.C (working copy)
@@ -1,5 +1,4 @@
// { dg-lto-do link }
-// { dg-lto-options { -O0 -flto } }
enum a {} b; // { dg-lto-warning "6: type 'a' violates the C\\+\\+ One Definition Rule" }
int
main(void)
Index: g++.dg/lto/odr-3_0.C
===================================================================
--- g++.dg/lto/odr-3_0.C (revision 266382)
+++ g++.dg/lto/odr-3_0.C (working copy)
@@ -1,12 +1,32 @@
// { dg-lto-do link }
-// { dg-lto-options { -O0 -flto } }
typedef struct {
int a; // { dg-lto-message "the first difference of corresponding definitions is field 'a'" }
-} YYSTYPE; // { dg-lto-warning "3: warning: type ‘struct YYSTYPE’ violates the C\\+\\+ One Definition Rule" }
-union yyalloc { // { dg-lto-warning "7: type ‘union yyalloc’ violates the C\\+\\+ One Definition Rule" }
+} YYSTYPE; // { dg-lto-message "violates the C\\+\\+ One Definition Rule" 2 }
+ // Here we get warning and a note:
+ // warning: type 'struct YYSTYPE' violates the C++ One Definition Rule
+ // note: type 'struct YYSTYPE' itself violates the C++ One Definition Rule
+union yyalloc { // { dg-lto-warning "7: type 'union yyalloc' violates the C\\+\\+ One Definition Rule" }
short yyss;
- YYSTYPE yyvs; // { dg-lto-message "the first difference of corresponding definitions is field ‘yyvs’" }
+ YYSTYPE yyvs; // { dg-lto-message "the first difference of corresponding definitions is field 'yyvs'" }
};
-void b() { yyalloc c; }
+extern yyalloc a; // { dg-lto-warning "16: 'a' violates the C\\+\\+ One Definition Rule" }
+int
+main (void) {return a.yyss;}
+/* Match warnings as follows:
+ odr-3_0.C:5:3: warning: type 'struct YYSTYPE' violates the C++ One Definition Rule [-Wodr]
+ odr-3_1.C:1:16: note: a different type is defined in another translation unit^
+ odr-3_0.C:4:7: note: the first difference of corresponding definitions is field 'a'
+ odr-3_1.C:1:16: note: a type with different number of fields is defined in another translation unit
+ odr-3_0.C:9:7: warning: type 'union yyalloc' violates the C++ One Definition Rule [-Wodr]
+ odr-3_1.C:6:7: note: a different type is defined in another translation unit
+ odr-3_0.C:11:11: note: the first difference of corresponding definitions is field 'yyvs'
+ odr-3_1.C:11:11: note: a field of same name but different type is defined in another translation unit
+ odr-3_0.C:5:3: note: type 'struct YYSTYPE' itself violates the C++ One Definition Rule
+ odr-3_0.C:13:16: warning: 'a' violates the C++ One Definition Rule [-Wodr]
+ odr-3_1.C:6:7: note: type 'union yyalloc' itself violates the C++ One Definition Rule
+ odr-3_0.C:9:7: note: the incompatible type is defined here
+ odr-3_1.C:15:9: note: 'a' was previously declared here
+ odr-3_1.C:15:9: note: code may be misoptimized unless -fno-strict-aliasing is used
+*/
Index: g++.dg/lto/odr-3_1.C
===================================================================
--- g++.dg/lto/odr-3_1.C (revision 266382)
+++ g++.dg/lto/odr-3_1.C (working copy)
@@ -1,9 +1,16 @@
-typedef struct YYSTYPE { // { dg-lto-message ":16 a different type is defined in another translation unit" }
+typedef struct YYSTYPE { // { dg-lto-message "type" 2 }
+ // We get two notes here:
+ // note: a different type is defined in another translation unit
+ // note: a type with different number of fields is defined in another translation unit
} YYSTYPE;
-union yyalloc {
+union yyalloc { // { dg-lto-message "type" 2 }
+ // We get here three notes:
+ // note: a different type is defined in another translation unit
+ // note: type 'union yyalloc' itself violates the C++ One Definition Rule
short yyss;
- YYSTYPE yyvs; // { dg-lto-message "the first difference of corresponding definitions is field ‘yyvs’" }
+ YYSTYPE yyvs; // { dg-lto-message "field of same name but different type is defined in another translation unit" }
-};
-void a() { yyalloc b; }
+};
+
+yyalloc a; // { dg-lto-message "'a' was previously declared here" }
Honza, One nit. On 21 November 2018 19:08:43 CET, Jan Hubicka <hubicka@ucw.cz> wrote: >> >Index: ipa-devirt.c >=================================================================== >--- ipa-devirt.c (revision 266334) >+++ ipa-devirt.c (working copy) >@@ -2289,27 +2291,43 @@ dump_type_inheritance_graph (FILE *f) > "%i duplicates overall\n", num_all_types, num_types, >num_duplicates); > } > >-/* Save some WPA->ltrans streaming by freeing enum values. */ >+/* Save some WPA->ltrans streaming by freeing stuff needed only for >good >+ ODR warnings. >+ We free TYPE_VALUES of enums and also make TYPE_DECLs to not point >back >+ to the type (which is needed to keep them in the same SCC and >preserve >+ location information to output warnings) and subsequently we make >all >+ TYPE_DECLS of same assembler name equivalent. */ > > static void >-free_enum_values () >+free_odr_warning_data () > { >- static bool enum_values_freed = false; >- if (enum_values_freed || !flag_wpa || !odr_types_ptr) >+ static bool odr_data_freed = false; >+ >+ if (odr_data_freed || !flag_wpa || !odr_types_ptr) > return; >- enum_values_freed = true; >- unsigned int i; >- for (i = 0; i < odr_types.length (); i++) >+ >+ odr_data_freed = true; First store to odr_data_freed is here. >+ >+ for (unsigned int i = 0; i < odr_types.length (); i++) > if (odr_types[i]) > { >- if (TREE_CODE (odr_types[i]->type) == ENUMERAL_TYPE) >- TYPE_VALUES (odr_types[i]->type) = NULL; >+ tree t = odr_types[i]->type; >+ >+ if (TREE_CODE (t) == ENUMERAL_TYPE) >+ TYPE_VALUES (t) = NULL; >+ TREE_TYPE (TYPE_NAME (t)) = void_type_node; >+ > if (odr_types[i]->types) > for (unsigned int j = 0; j < odr_types[i]->types->length (); j++) >- if (TREE_CODE ((*odr_types[i]->types)[j]) == ENUMERAL_TYPE) >- TYPE_VALUES ((*odr_types[i]->types)[j]) = NULL; >+ { >+ tree td = (*odr_types[i]->types)[j]; >+ >+ if (TREE_CODE (td) == ENUMERAL_TYPE) >+ TYPE_VALUES (td) = NULL; >+ TYPE_NAME (td) = TYPE_NAME (t); >+ } > } >- enum_values_freed = true; >+ odr_data_freed = true; So please remove this second store. Thanks, > } >
Index: tree.c =================================================================== --- tree.c (revision 266325) +++ tree.c (working copy) @@ -5206,6 +5206,24 @@ fld_process_array_type (tree t, tree t2, return array; } +/* Return CTX after removal of contexts that are not relevant */ + +static tree +fld_decl_context (tree ctx) +{ + /* Variably modified types are needed for tree_is_indexable to decide + whether the type needs to go to local or global section. + This code is semi-broken but for now it is easiest to keep contexts + as expected. */ + if (ctx && TYPE_P (ctx) + && !variably_modified_type_p (ctx, NULL_TREE)) + { + while (ctx && TYPE_P (ctx)) + ctx = TYPE_CONTEXT (ctx); + } + return ctx; +} + /* For T being aggregate type try to turn it into a incomplete variant. Return T if no simplification is possible. */ @@ -5267,6 +5285,27 @@ fld_incomplete_type_of (tree t, struct f } else TYPE_VALUES (copy) = NULL; + + /* Build copy of TYPE_DECL in TYPE_NAME if necessary. + This is needed for ODR violation warnings to come out right (we + want duplicate TYPE_DECLs whenever the type is duplicated because + of ODR violation. Because lang data in the TYPE_DECL may not + have been freed yet, rebuild it from scratch and copy relevant + fields. */ + TYPE_NAME (copy) = fld_simplified_type_name (copy); + tree name = TYPE_NAME (copy); + + if (name && TREE_CODE (name) == TYPE_DECL) + { + gcc_checking_assert (TREE_TYPE (name) == t); + tree name2 = build_decl (DECL_SOURCE_LOCATION (name), TYPE_DECL, + DECL_NAME (name), copy); + SET_DECL_ASSEMBLER_NAME (name2, DECL_ASSEMBLER_NAME (name)); + SET_DECL_ALIGN (name2, 0); + DECL_CONTEXT (name2) = fld_decl_context + (DECL_CONTEXT (name)); + TYPE_NAME (copy) = name2; + } } return copy; } @@ -5649,12 +5688,13 @@ free_lang_data_in_decl (tree decl, struc { DECL_VISIBILITY (decl) = VISIBILITY_DEFAULT; DECL_VISIBILITY_SPECIFIED (decl) = 0; - /* TREE_PUBLIC is used to tell if type is anonymous. */ + TREE_PUBLIC (decl) = 0; + TREE_PRIVATE (decl) = 0; + DECL_ARTIFICIAL (decl) = 0; TYPE_DECL_SUPPRESS_DEBUG (decl) = 0; DECL_INITIAL (decl) = NULL_TREE; DECL_ORIGINAL_TYPE (decl) = NULL_TREE; DECL_MODE (decl) = VOIDmode; - TREE_TYPE (decl) = void_type_node; SET_DECL_ALIGN (decl, 0); } else if (TREE_CODE (decl) == FIELD_DECL) @@ -5688,20 +5728,7 @@ free_lang_data_in_decl (tree decl, struc if (TREE_CODE (decl) != FIELD_DECL && ((TREE_CODE (decl) != VAR_DECL && TREE_CODE (decl) != FUNCTION_DECL) || !DECL_VIRTUAL_P (decl))) - { - tree ctx = DECL_CONTEXT (decl); - /* Variably modified types are needed for tree_is_indexable to decide - whether the type needs to go to local or global section. - This code is semi-broken but for now it is easiest to keep contexts - as expected. */ - if (ctx && TYPE_P (ctx) - && !variably_modified_type_p (ctx, NULL_TREE)) - { - while (ctx && TYPE_P (ctx)) - ctx = TYPE_CONTEXT (ctx); - DECL_CONTEXT (decl) = ctx; - } - } + DECL_CONTEXT (decl) = fld_decl_context (DECL_CONTEXT (decl)); } Index: ipa-devirt.c =================================================================== --- ipa-devirt.c (revision 266325) +++ ipa-devirt.c (working copy) @@ -2289,27 +2289,39 @@ dump_type_inheritance_graph (FILE *f) "%i duplicates overall\n", num_all_types, num_types, num_duplicates); } -/* Save some WPA->ltrans streaming by freeing enum values. */ +/* Save some WPA->ltrans streaming by freeing stuff needed only for good + ODR warnings. */ static void -free_enum_values () +free_odr_warning_data () { - static bool enum_values_freed = false; - if (enum_values_freed || !flag_wpa || !odr_types_ptr) + static bool odr_data_freed = false; + + if (odr_data_freed || !flag_wpa || !odr_types_ptr) return; - enum_values_freed = true; - unsigned int i; - for (i = 0; i < odr_types.length (); i++) + + odr_data_freed = true; + + for (unsigned int i = 0; i < odr_types.length (); i++) if (odr_types[i]) { - if (TREE_CODE (odr_types[i]->type) == ENUMERAL_TYPE) - TYPE_VALUES (odr_types[i]->type) = NULL; + tree t = odr_types[i]->type; + + if (TREE_CODE (t) == ENUMERAL_TYPE) + TYPE_VALUES (t) = NULL; + TREE_TYPE (TYPE_NAME (t)) = void_type_node; + if (odr_types[i]->types) for (unsigned int j = 0; j < odr_types[i]->types->length (); j++) - if (TREE_CODE ((*odr_types[i]->types)[j]) == ENUMERAL_TYPE) - TYPE_VALUES ((*odr_types[i]->types)[j]) = NULL; + { + tree td = (*odr_types[i]->types)[j]; + + if (TREE_CODE (td) == ENUMERAL_TYPE) + TYPE_VALUES (td) = NULL; + TYPE_NAME (td) = TYPE_NAME (t); + } } - enum_values_freed = true; + odr_data_freed = true; } /* Initialize IPA devirt and build inheritance tree graph. */ @@ -2323,7 +2335,7 @@ build_type_inheritance_graph (void) if (odr_hash) { - free_enum_values (); + free_odr_warning_data (); return; } timevar_push (TV_IPA_INHERITANCE); @@ -2370,7 +2382,7 @@ build_type_inheritance_graph (void) dump_type_inheritance_graph (inheritance_dump_file); dump_end (TDI_inheritance, inheritance_dump_file); } - free_enum_values (); + free_odr_warning_data (); timevar_pop (TV_IPA_INHERITANCE); }