Message ID | 20190209181017.wj4fyul5oqwtoplx@kam.mff.cuni.cz |
---|---|
State | New |
Headers | show |
Series | Fix odr ICE on Ada LTO | expand |
On Sat, Feb 9, 2019 at 10:10 AM Jan Hubicka <hubicka@ucw.cz> wrote: > > Hi, > this patch fixes ICE in free_lang_data compiling lto8.adb. > The fix is bit symptomatic becuase type_with_linkage_p should return > false for Ada types. Perhaps adding explicit flag to DECL_NAME would > make sense but it can wait for next stage1. > > The fix works because at this stage of free_lang_data all mangled names > must be computed and thus it is cheper to test presence of > DECL_ASSEMBLER_NAME anyway. > > Bootstrapped/regtested x86_64-linux, comitted. > PR lto/87957 > * tree.c (fld_simplified_type_name): Use DECL_ASSEMBLER_NAME_SET_P > instead of type_with_linkage. > Index: tree.c > =================================================================== > --- tree.c (revision 268722) > +++ tree.c (working copy) > @@ -5152,7 +5152,8 @@ fld_simplified_type_name (tree type) > /* 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)) > + if (type != TYPE_MAIN_VARIANT (type) > + || !DECL_ASSEMBLER_NAME_SET_P (TYPE_NAME (type))) > return DECL_NAME (TYPE_NAME (type)); > return TYPE_NAME (type); > } This caused: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=89272
> > This caused: > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=89272 My apologizes for that. Fixed by the attached patch. This is about ICE with -fno-lto-odr-type-merging which is option I think we should drop (probably next stage1 but if it shows to cause troubles, I would not be against dropping it from gcc 9) It is not useful and only leads to code duplication. lto-bootstrapped/regtested x86_64-linux, comitted. Honza Index: ChangeLog =================================================================== --- ChangeLog (revision 267609) +++ ChangeLog (working copy) @@ -1,4 +1,14 @@ +2019-01-03 Jan Hubicka <hubicka@ucw.cz> + Backport from mainline + 2018-08-29 Jan Hubicka <jh@suse.cz> + + PR lto/86517 + PR lto/88185 + * lto-opts.c (lto_write_options): Always stream PIC/PIE mode. + * lto-wrapper.c (merge_and_complain): Fix merging of PIC/PIE. + 2019-01-04 Aaron Sawdey <acsawdey@linux.ibm.com> + Backport from mainline 2018-11-28 Aaron Sawdey <acsawdey@linux.ibm.com> Index: lto-opts.c =================================================================== --- lto-opts.c (revision 267609) +++ lto-opts.c (working copy) @@ -78,6 +78,21 @@ lto_write_options (void) && !global_options.x_flag_openacc) append_to_collect_gcc_options (&temporary_obstack, &first_p, "-fno-openacc"); + /* Append PIC/PIE mode because its default depends on target and it is + subject of merging in lto-wrapper. */ + if (!global_options_set.x_flag_pic && !global_options_set.x_flag_pie) + { + append_to_collect_gcc_options (&temporary_obstack, &first_p, + global_options.x_flag_pic == 2 + ? "-fPIC" + : global_options.x_flag_pic == 1 + ? "-fpic" + : global_options.x_flag_pie == 2 + ? "-fPIE" + : global_options.x_flag_pie == 1 + ? "-fpie" + : "-fno-pie"); + } /* Append options from target hook and store them to offload_lto section. */ if (lto_stream_offload_p) Index: lto-wrapper.c =================================================================== --- lto-wrapper.c (revision 267609) +++ lto-wrapper.c (working copy) @@ -408,6 +408,11 @@ merge_and_complain (struct cl_decoded_op It is a common mistake to mix few -fPIC compiled objects into otherwise non-PIC code. We do not want to build everything with PIC then. + Similarly we merge PIE options, however in addition we keep + -fPIC + -fPIE = -fPIE + -fpic + -fPIE = -fpie + -fPIC/-fpic + -fpie = -fpie + It would be good to warn on mismatches, but it is bit hard to do as we do not know what nothing translates to. */ @@ -415,11 +420,38 @@ merge_and_complain (struct cl_decoded_op if ((*decoded_options)[j].opt_index == OPT_fPIC || (*decoded_options)[j].opt_index == OPT_fpic) { - if (!pic_option - || (pic_option->value > 0) != ((*decoded_options)[j].value > 0)) - remove_option (decoded_options, j, decoded_options_count); - else if (pic_option->opt_index == OPT_fPIC - && (*decoded_options)[j].opt_index == OPT_fpic) + /* -fno-pic in one unit implies -fno-pic everywhere. */ + if ((*decoded_options)[j].value == 0) + j++; + /* If we have no pic option or merge in -fno-pic, we still may turn + existing pic/PIC mode into pie/PIE if -fpie/-fPIE is present. */ + else if ((pic_option && pic_option->value == 0) + || !pic_option) + { + if (pie_option) + { + bool big = (*decoded_options)[j].opt_index == OPT_fPIC + && pie_option->opt_index == OPT_fPIE; + (*decoded_options)[j].opt_index = big ? OPT_fPIE : OPT_fpie; + if (pie_option->value) + (*decoded_options)[j].canonical_option[0] = big ? "-fPIE" : "-fpie"; + else + (*decoded_options)[j].canonical_option[0] = big ? "-fno-pie" : "-fno-pie"; + (*decoded_options)[j].value = pie_option->value; + j++; + } + else if (pic_option) + { + (*decoded_options)[j] = *pic_option; + j++; + } + /* We do not know if target defaults to pic or not, so just remove + option if it is missing in one unit but enabled in other. */ + else + remove_option (decoded_options, j, decoded_options_count); + } + else if (pic_option->opt_index == OPT_fpic + && (*decoded_options)[j].opt_index == OPT_fPIC) { (*decoded_options)[j] = *pic_option; j++; @@ -430,11 +462,42 @@ merge_and_complain (struct cl_decoded_op else if ((*decoded_options)[j].opt_index == OPT_fPIE || (*decoded_options)[j].opt_index == OPT_fpie) { - if (!pie_option - || pie_option->value != (*decoded_options)[j].value) - remove_option (decoded_options, j, decoded_options_count); - else if (pie_option->opt_index == OPT_fPIE - && (*decoded_options)[j].opt_index == OPT_fpie) + /* -fno-pie in one unit implies -fno-pie everywhere. */ + if ((*decoded_options)[j].value == 0) + j++; + /* If we have no pie option or merge in -fno-pie, we still preserve + PIE/pie if pic/PIC is present. */ + else if ((pie_option && pie_option->value == 0) + || !pie_option) + { + /* If -fPIC/-fpic is given, merge it with -fPIE/-fpie. */ + if (pic_option) + { + if (pic_option->opt_index == OPT_fpic + && (*decoded_options)[j].opt_index == OPT_fPIE) + { + (*decoded_options)[j].opt_index = OPT_fpie; + (*decoded_options)[j].canonical_option[0] + = pic_option->value ? "-fpie" : "-fno-pie"; + } + else if (!pic_option->value) + (*decoded_options)[j].canonical_option[0] = "-fno-pie"; + (*decoded_options)[j].value = pic_option->value; + j++; + } + else if (pie_option) + { + (*decoded_options)[j] = *pie_option; + j++; + } + /* Because we always append pic/PIE options this code path should + not happen unless the LTO object was built by old lto1 which + did not contain that logic yet. */ + else + remove_option (decoded_options, j, decoded_options_count); + } + else if (pie_option->opt_index == OPT_fpie + && (*decoded_options)[j].opt_index == OPT_fPIE) { (*decoded_options)[j] = *pie_option; j++;
On February 10, 2019 11:48:01 AM GMT+01:00, Jan Hubicka <hubicka@ucw.cz> wrote: >> >> This caused: >> >> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=89272 > >My apologizes for that. Fixed by the attached patch. This is about ICE >with -fno-lto-odr-type-merging which is option I think we should drop >(probably next stage1 but if it shows to cause troubles, I would not be >against dropping it from gcc 9) > >It is not useful and only leads to code duplication. > >lto-bootstrapped/regtested x86_64-linux, comitted. Looks like you attached the wrong patch. BTW, the option is new, right? If it is and it serves no purpose please remove it now. Richard. >Honza > >Index: ChangeLog >=================================================================== >--- ChangeLog (revision 267609) >+++ ChangeLog (working copy) >@@ -1,4 +1,14 @@ >+2019-01-03 Jan Hubicka <hubicka@ucw.cz> >+ Backport from mainline >+ 2018-08-29 Jan Hubicka <jh@suse.cz> >+ >+ PR lto/86517 >+ PR lto/88185 >+ * lto-opts.c (lto_write_options): Always stream PIC/PIE mode. >+ * lto-wrapper.c (merge_and_complain): Fix merging of PIC/PIE. >+ > 2019-01-04 Aaron Sawdey <acsawdey@linux.ibm.com> >+ > Backport from mainline > 2018-11-28 Aaron Sawdey <acsawdey@linux.ibm.com> > >Index: lto-opts.c >=================================================================== >--- lto-opts.c (revision 267609) >+++ lto-opts.c (working copy) >@@ -78,6 +78,21 @@ lto_write_options (void) > && !global_options.x_flag_openacc) > append_to_collect_gcc_options (&temporary_obstack, &first_p, > "-fno-openacc"); >+ /* Append PIC/PIE mode because its default depends on target and it >is >+ subject of merging in lto-wrapper. */ >+ if (!global_options_set.x_flag_pic && >!global_options_set.x_flag_pie) >+ { >+ append_to_collect_gcc_options (&temporary_obstack, &first_p, >+ global_options.x_flag_pic == 2 >+ ? "-fPIC" >+ : global_options.x_flag_pic == 1 >+ ? "-fpic" >+ : global_options.x_flag_pie == 2 >+ ? "-fPIE" >+ : global_options.x_flag_pie == 1 >+ ? "-fpie" >+ : "-fno-pie"); >+ } > >/* Append options from target hook and store them to offload_lto >section. */ > if (lto_stream_offload_p) >Index: lto-wrapper.c >=================================================================== >--- lto-wrapper.c (revision 267609) >+++ lto-wrapper.c (working copy) >@@ -408,6 +408,11 @@ merge_and_complain (struct cl_decoded_op >It is a common mistake to mix few -fPIC compiled objects into otherwise > non-PIC code. We do not want to build everything with PIC then. > >+ Similarly we merge PIE options, however in addition we keep >+ -fPIC + -fPIE = -fPIE >+ -fpic + -fPIE = -fpie >+ -fPIC/-fpic + -fpie = -fpie >+ > It would be good to warn on mismatches, but it is bit hard to do as > we do not know what nothing translates to. */ > >@@ -415,11 +420,38 @@ merge_and_complain (struct cl_decoded_op > if ((*decoded_options)[j].opt_index == OPT_fPIC > || (*decoded_options)[j].opt_index == OPT_fpic) > { >- if (!pic_option >- || (pic_option->value > 0) != ((*decoded_options)[j].value > 0)) >- remove_option (decoded_options, j, decoded_options_count); >- else if (pic_option->opt_index == OPT_fPIC >- && (*decoded_options)[j].opt_index == OPT_fpic) >+ /* -fno-pic in one unit implies -fno-pic everywhere. */ >+ if ((*decoded_options)[j].value == 0) >+ j++; >+ /* If we have no pic option or merge in -fno-pic, we still may turn >+ existing pic/PIC mode into pie/PIE if -fpie/-fPIE is present. */ >+ else if ((pic_option && pic_option->value == 0) >+ || !pic_option) >+ { >+ if (pie_option) >+ { >+ bool big = (*decoded_options)[j].opt_index == OPT_fPIC >+ && pie_option->opt_index == OPT_fPIE; >+ (*decoded_options)[j].opt_index = big ? OPT_fPIE : OPT_fpie; >+ if (pie_option->value) >+ (*decoded_options)[j].canonical_option[0] = big ? "-fPIE" : >"-fpie"; >+ else >+ (*decoded_options)[j].canonical_option[0] = big ? >"-fno-pie" : "-fno-pie"; >+ (*decoded_options)[j].value = pie_option->value; >+ j++; >+ } >+ else if (pic_option) >+ { >+ (*decoded_options)[j] = *pic_option; >+ j++; >+ } >+ /* We do not know if target defaults to pic or not, so just >remove >+ option if it is missing in one unit but enabled in other. */ >+ else >+ remove_option (decoded_options, j, decoded_options_count); >+ } >+ else if (pic_option->opt_index == OPT_fpic >+ && (*decoded_options)[j].opt_index == OPT_fPIC) > { > (*decoded_options)[j] = *pic_option; > j++; >@@ -430,11 +462,42 @@ merge_and_complain (struct cl_decoded_op > else if ((*decoded_options)[j].opt_index == OPT_fPIE > || (*decoded_options)[j].opt_index == OPT_fpie) > { >- if (!pie_option >- || pie_option->value != (*decoded_options)[j].value) >- remove_option (decoded_options, j, decoded_options_count); >- else if (pie_option->opt_index == OPT_fPIE >- && (*decoded_options)[j].opt_index == OPT_fpie) >+ /* -fno-pie in one unit implies -fno-pie everywhere. */ >+ if ((*decoded_options)[j].value == 0) >+ j++; >+ /* If we have no pie option or merge in -fno-pie, we still preserve >+ PIE/pie if pic/PIC is present. */ >+ else if ((pie_option && pie_option->value == 0) >+ || !pie_option) >+ { >+ /* If -fPIC/-fpic is given, merge it with -fPIE/-fpie. */ >+ if (pic_option) >+ { >+ if (pic_option->opt_index == OPT_fpic >+ && (*decoded_options)[j].opt_index == OPT_fPIE) >+ { >+ (*decoded_options)[j].opt_index = OPT_fpie; >+ (*decoded_options)[j].canonical_option[0] >+ = pic_option->value ? "-fpie" : "-fno-pie"; >+ } >+ else if (!pic_option->value) >+ (*decoded_options)[j].canonical_option[0] = "-fno-pie"; >+ (*decoded_options)[j].value = pic_option->value; >+ j++; >+ } >+ else if (pie_option) >+ { >+ (*decoded_options)[j] = *pie_option; >+ j++; >+ } >+ /* Because we always append pic/PIE options this code path should >+ not happen unless the LTO object was built by old lto1 which >+ did not contain that logic yet. */ >+ else >+ remove_option (decoded_options, j, decoded_options_count); >+ } >+ else if (pie_option->opt_index == OPT_fpie >+ && (*decoded_options)[j].opt_index == OPT_fPIE) > { > (*decoded_options)[j] = *pie_option; > j++;
Hi, I am attaching correct patch. The option is new only in a relative sense - it was added 5 years ago with the orinal ODR warning infrastructure. We have -Wodr-type-merging that controls streming data needed for -Wodr to work and -fno-devirtualize that controls streaming of BINFOs. I was concerned at that time about extra overhead this streaming causes, but with all the optimizations this overhead is quite small now (i.e. the mangled type names and there are "only" about 4k types in Firefox) What is anoying about -Wno-odr-type-merging is that we lose mangled names that are also used by devirtualization. ipa-devirt still has two implementations of the main hash - one based on mangled names and the original one based on virtual table names, but combining both hashes results in incomplete type inheritance graphs. Honza PR lto/89272 * tree.c (fld_simplified_type_name): Also keep TYPE_DECL for polymorphic types. --- trunk/gcc/tree.c 2019/02/10 09:45:55 268741 +++ trunk/gcc/tree.c 2019/02/10 10:46:43 268742 @@ -5153,7 +5153,10 @@ TYPE_DECL if the type doesn't have linkage. this must match fld_ */ if (type != TYPE_MAIN_VARIANT (type) - || !DECL_ASSEMBLER_NAME_SET_P (TYPE_NAME (type))) + || (!DECL_ASSEMBLER_NAME_SET_P (TYPE_NAME (type)) + && (TREE_CODE (type) != RECORD_TYPE + || !TYPE_BINFO (type) + || !BINFO_VTABLE (TYPE_BINFO (type))))) return DECL_NAME (TYPE_NAME (type)); return TYPE_NAME (type); }
On Sun, Feb 10, 2019 at 11:07 PM Jan Hubicka <hubicka@ucw.cz> wrote: > > Hi, > I am attaching correct patch. > The option is new only in a relative sense - it was added 5 years ago > with the orinal ODR warning infrastructure. > We have -Wodr-type-merging that controls streming data needed for -Wodr > to work and -fno-devirtualize that controls streaming of BINFOs. > > I was concerned at that time about extra overhead this streaming causes, > but with all the optimizations this overhead is quite small now (i.e. > the mangled type names and there are "only" about 4k types in Firefox) > > What is anoying about -Wno-odr-type-merging is that we lose mangled > names that are also used by devirtualization. ipa-devirt still has two > implementations of the main hash - one based on mangled names and the > original one based on virtual table names, but combining both hashes > results in incomplete type inheritance graphs. Ah, I see. I guess we can clean this up for GCC 10 then. Richard. > Honza > > > PR lto/89272 > * tree.c (fld_simplified_type_name): Also keep TYPE_DECL for > polymorphic types. > > > --- trunk/gcc/tree.c 2019/02/10 09:45:55 268741 > +++ trunk/gcc/tree.c 2019/02/10 10:46:43 268742 > @@ -5153,7 +5153,10 @@ > TYPE_DECL if the type doesn't have linkage. > this must match fld_ */ > if (type != TYPE_MAIN_VARIANT (type) > - || !DECL_ASSEMBLER_NAME_SET_P (TYPE_NAME (type))) > + || (!DECL_ASSEMBLER_NAME_SET_P (TYPE_NAME (type)) > + && (TREE_CODE (type) != RECORD_TYPE > + || !TYPE_BINFO (type) > + || !BINFO_VTABLE (TYPE_BINFO (type))))) > return DECL_NAME (TYPE_NAME (type)); > return TYPE_NAME (type); > } >
Index: tree.c =================================================================== --- tree.c (revision 268722) +++ tree.c (working copy) @@ -5152,7 +5152,8 @@ fld_simplified_type_name (tree type) /* 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)) + if (type != TYPE_MAIN_VARIANT (type) + || !DECL_ASSEMBLER_NAME_SET_P (TYPE_NAME (type))) return DECL_NAME (TYPE_NAME (type)); return TYPE_NAME (type); }