* Fix odr ICE on Ada LTO
@ 2019-02-09 18:10 Jan Hubicka
2019-02-09 23:27 ` H.J. Lu
0 siblings, 1 reply; 6+ messages in thread
From: Jan Hubicka @ 2019-02-09 18:10 UTC (permalink / raw)
To: gcc-patches
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);
}
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Fix odr ICE on Ada LTO
2019-02-09 18:10 Fix odr ICE on Ada LTO Jan Hubicka
@ 2019-02-09 23:27 ` H.J. Lu
2019-02-10 10:48 ` Jan Hubicka
0 siblings, 1 reply; 6+ messages in thread
From: H.J. Lu @ 2019-02-09 23:27 UTC (permalink / raw)
To: Jan Hubicka; +Cc: GCC Patches
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
--
H.J.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Fix odr ICE on Ada LTO
2019-02-09 23:27 ` H.J. Lu
@ 2019-02-10 10:48 ` Jan Hubicka
2019-02-10 18:19 ` Richard Biener
0 siblings, 1 reply; 6+ messages in thread
From: Jan Hubicka @ 2019-02-10 10:48 UTC (permalink / raw)
To: H.J. Lu; +Cc: GCC Patches
>
> 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++;
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Fix odr ICE on Ada LTO
2019-02-10 10:48 ` Jan Hubicka
@ 2019-02-10 18:19 ` Richard Biener
2019-02-10 22:07 ` Jan Hubicka
0 siblings, 1 reply; 6+ messages in thread
From: Richard Biener @ 2019-02-10 18:19 UTC (permalink / raw)
To: gcc-patches, Jan Hubicka, H.J. Lu; +Cc: GCC Patches
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++;
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Fix odr ICE on Ada LTO
2019-02-10 18:19 ` Richard Biener
@ 2019-02-10 22:07 ` Jan Hubicka
2019-02-12 9:49 ` Richard Biener
0 siblings, 1 reply; 6+ messages in thread
From: Jan Hubicka @ 2019-02-10 22:07 UTC (permalink / raw)
To: Richard Biener; +Cc: gcc-patches, H.J. Lu
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);
}
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Fix odr ICE on Ada LTO
2019-02-10 22:07 ` Jan Hubicka
@ 2019-02-12 9:49 ` Richard Biener
0 siblings, 0 replies; 6+ messages in thread
From: Richard Biener @ 2019-02-12 9:49 UTC (permalink / raw)
To: Jan Hubicka; +Cc: GCC Patches, H.J. Lu
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);
> }
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-02-12 9:49 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-09 18:10 Fix odr ICE on Ada LTO Jan Hubicka
2019-02-09 23:27 ` H.J. Lu
2019-02-10 10:48 ` Jan Hubicka
2019-02-10 18:19 ` Richard Biener
2019-02-10 22:07 ` Jan Hubicka
2019-02-12 9:49 ` Richard Biener
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).