* Better merging of -fPIC/pic/PIE/pie in lto-wrapper
@ 2017-07-08 11:03 Jan Hubicka
2017-07-17 8:17 ` Richard Biener
0 siblings, 1 reply; 3+ messages in thread
From: Jan Hubicka @ 2017-07-08 11:03 UTC (permalink / raw)
To: gcc-patches
Hi,
PR lto/80838 is about lto+profiledbootstrapped compiler being slower than
profiledboostrapped compiler.
This is caused by a fact that some of object files linked into cc1plus binary
are built with -fPIC and lto-wrapper then decides to make whole binary PIC that
is very slow. While we probably ought to avoid linking PIC code into static
binary but I saw similar issue with firefox and other programs.
I do not think we want to support mixed PIC/non-PIC symbols internally, because
it would need quite some work and I do not see any reasonable use cases.
This patch makes merging more realistic/agressive. Linking -fPIC and non-PIC
code together results in non-PIC binary and thus the corresponding flags are
dropped when mismatches occurs.
It would be nice to warn about it, but I do not know how to make warning
meaningful on targets that are PIC by default.
Bootstrapped/regtested x86_64-linux, plan to commit it tomorrow after
lto-boottrapping if there are no complains.
Honza
PR lto/80838
* lto-wrapper.c (remove_option): New function.
(merge_and_complain): Merge PIC/PIE options more realistically.
Index: lto-wrapper.c
===================================================================
--- lto-wrapper.c (revision 250054)
+++ lto-wrapper.c (working copy)
@@ -192,6 +192,20 @@ append_option (struct cl_decoded_option
sizeof (struct cl_decoded_option));
}
+/* Remove option number INDEX from DECODED_OPTIONS, update
+ DECODED_OPTIONS_COUNT. */
+
+static void
+remove_option (struct cl_decoded_option **decoded_options,
+ int index, unsigned int *decoded_options_count)
+{
+ --*decoded_options_count;
+ memmove (&(*decoded_options)[index + 1],
+ &(*decoded_options)[index],
+ sizeof (struct cl_decoded_option)
+ * (*decoded_options_count - index));
+}
+
/* Try to merge and complain about options FDECODED_OPTIONS when applied
ontop of DECODED_OPTIONS. */
@@ -202,6 +216,8 @@ merge_and_complain (struct cl_decoded_op
unsigned int fdecoded_options_count)
{
unsigned int i, j;
+ struct cl_decoded_option *pic_option = NULL;
+ struct cl_decoded_option *pie_option = NULL;
/* ??? Merge options from files. Most cases can be
handled by either unioning or intersecting
@@ -238,10 +254,6 @@ merge_and_complain (struct cl_decoded_op
case OPT_fdiagnostics_show_option:
case OPT_fdiagnostics_show_location_:
case OPT_fshow_column:
- case OPT_fPIC:
- case OPT_fpic:
- case OPT_fPIE:
- case OPT_fpie:
case OPT_fcommon:
case OPT_fgnu_tm:
/* Do what the old LTO code did - collect exactly one option
@@ -255,6 +267,16 @@ merge_and_complain (struct cl_decoded_op
append_option (decoded_options, decoded_options_count, foption);
break;
+ /* Figure out what PIC/PIE level wins and merge the results. */
+ case OPT_fPIC:
+ case OPT_fpic:
+ pic_option = foption;
+ break;
+ case OPT_fPIE:
+ case OPT_fpie:
+ pie_option = foption;
+ break;
+
case OPT_fopenmp:
case OPT_fopenacc:
case OPT_fcilkplus:
@@ -286,18 +308,6 @@ merge_and_complain (struct cl_decoded_op
foption->orig_option_with_args_text);
break;
- case OPT_foffload_abi_:
- for (j = 0; j < *decoded_options_count; ++j)
- if ((*decoded_options)[j].opt_index == foption->opt_index)
- break;
- if (j == *decoded_options_count)
- append_option (decoded_options, decoded_options_count, foption);
- else if (foption->value != (*decoded_options)[j].value)
- fatal_error (input_location,
- "Option %s not used consistently in all LTO input"
- " files", foption->orig_option_with_args_text);
- break;
-
case OPT_O:
case OPT_Ofast:
case OPT_Og:
@@ -368,12 +378,70 @@ merge_and_complain (struct cl_decoded_op
(*decoded_options)[j].value = 1;
}
break;
+
+
+ case OPT_foffload_abi_:
+ for (j = 0; j < *decoded_options_count; ++j)
+ if ((*decoded_options)[j].opt_index == foption->opt_index)
+ break;
+ if (j == *decoded_options_count)
+ append_option (decoded_options, decoded_options_count, foption);
+ else if (foption->value != (*decoded_options)[j].value)
+ fatal_error (input_location,
+ "Option %s not used consistently in all LTO input"
+ " files", foption->orig_option_with_args_text);
+ break;
+
case OPT_foffload_:
append_option (decoded_options, decoded_options_count, foption);
break;
}
}
+
+ /* Merge PIC options:
+ -fPIC + -fpic = -fpic
+ -fPIC + -fno-pic = -fno-pic
+ -fpic/-fPIC + nothin = nothing.
+ 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.
+
+ It would be good to warn on mismatches, but it is bit hard to do as
+ we do not know what nothing translates to. */
+
+ for (unsigned int j = 0; j < *decoded_options_count;)
+ 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)
+ {
+ (*decoded_options)[j] = *pic_option;
+ j++;
+ }
+ else
+ j++;
+ }
+ 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)
+ {
+ (*decoded_options)[j] = *pie_option;
+ j++;
+ }
+ else
+ j++;
+ }
+ else
+ j++;
}
/* Auxiliary function that frees elements of PTR and PTR itself.
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: Better merging of -fPIC/pic/PIE/pie in lto-wrapper
2017-07-08 11:03 Better merging of -fPIC/pic/PIE/pie in lto-wrapper Jan Hubicka
@ 2017-07-17 8:17 ` Richard Biener
2017-07-17 9:24 ` Jan Hubicka
0 siblings, 1 reply; 3+ messages in thread
From: Richard Biener @ 2017-07-17 8:17 UTC (permalink / raw)
To: Jan Hubicka; +Cc: GCC Patches
On Sat, Jul 8, 2017 at 1:03 PM, Jan Hubicka <hubicka@ucw.cz> wrote:
> Hi,
> PR lto/80838 is about lto+profiledbootstrapped compiler being slower than
> profiledboostrapped compiler.
>
> This is caused by a fact that some of object files linked into cc1plus binary
> are built with -fPIC and lto-wrapper then decides to make whole binary PIC that
> is very slow. While we probably ought to avoid linking PIC code into static
> binary but I saw similar issue with firefox and other programs.
>
> I do not think we want to support mixed PIC/non-PIC symbols internally, because
> it would need quite some work and I do not see any reasonable use cases.
>
> This patch makes merging more realistic/agressive. Linking -fPIC and non-PIC
> code together results in non-PIC binary and thus the corresponding flags are
> dropped when mismatches occurs.
>
> It would be nice to warn about it, but I do not know how to make warning
> meaningful on targets that are PIC by default.
>
> Bootstrapped/regtested x86_64-linux, plan to commit it tomorrow after
> lto-boottrapping if there are no complains.
Hum. I wonder if/why we can't ask the linker about the output binary kind?
Richard.
> Honza
>
> PR lto/80838
> * lto-wrapper.c (remove_option): New function.
> (merge_and_complain): Merge PIC/PIE options more realistically.
> Index: lto-wrapper.c
> ===================================================================
> --- lto-wrapper.c (revision 250054)
> +++ lto-wrapper.c (working copy)
> @@ -192,6 +192,20 @@ append_option (struct cl_decoded_option
> sizeof (struct cl_decoded_option));
> }
>
> +/* Remove option number INDEX from DECODED_OPTIONS, update
> + DECODED_OPTIONS_COUNT. */
> +
> +static void
> +remove_option (struct cl_decoded_option **decoded_options,
> + int index, unsigned int *decoded_options_count)
> +{
> + --*decoded_options_count;
> + memmove (&(*decoded_options)[index + 1],
> + &(*decoded_options)[index],
> + sizeof (struct cl_decoded_option)
> + * (*decoded_options_count - index));
> +}
> +
> /* Try to merge and complain about options FDECODED_OPTIONS when applied
> ontop of DECODED_OPTIONS. */
>
> @@ -202,6 +216,8 @@ merge_and_complain (struct cl_decoded_op
> unsigned int fdecoded_options_count)
> {
> unsigned int i, j;
> + struct cl_decoded_option *pic_option = NULL;
> + struct cl_decoded_option *pie_option = NULL;
>
> /* ??? Merge options from files. Most cases can be
> handled by either unioning or intersecting
> @@ -238,10 +254,6 @@ merge_and_complain (struct cl_decoded_op
> case OPT_fdiagnostics_show_option:
> case OPT_fdiagnostics_show_location_:
> case OPT_fshow_column:
> - case OPT_fPIC:
> - case OPT_fpic:
> - case OPT_fPIE:
> - case OPT_fpie:
> case OPT_fcommon:
> case OPT_fgnu_tm:
> /* Do what the old LTO code did - collect exactly one option
> @@ -255,6 +267,16 @@ merge_and_complain (struct cl_decoded_op
> append_option (decoded_options, decoded_options_count, foption);
> break;
>
> + /* Figure out what PIC/PIE level wins and merge the results. */
> + case OPT_fPIC:
> + case OPT_fpic:
> + pic_option = foption;
> + break;
> + case OPT_fPIE:
> + case OPT_fpie:
> + pie_option = foption;
> + break;
> +
> case OPT_fopenmp:
> case OPT_fopenacc:
> case OPT_fcilkplus:
> @@ -286,18 +308,6 @@ merge_and_complain (struct cl_decoded_op
> foption->orig_option_with_args_text);
> break;
>
> - case OPT_foffload_abi_:
> - for (j = 0; j < *decoded_options_count; ++j)
> - if ((*decoded_options)[j].opt_index == foption->opt_index)
> - break;
> - if (j == *decoded_options_count)
> - append_option (decoded_options, decoded_options_count, foption);
> - else if (foption->value != (*decoded_options)[j].value)
> - fatal_error (input_location,
> - "Option %s not used consistently in all LTO input"
> - " files", foption->orig_option_with_args_text);
> - break;
> -
> case OPT_O:
> case OPT_Ofast:
> case OPT_Og:
> @@ -368,12 +378,70 @@ merge_and_complain (struct cl_decoded_op
> (*decoded_options)[j].value = 1;
> }
> break;
> +
> +
> + case OPT_foffload_abi_:
> + for (j = 0; j < *decoded_options_count; ++j)
> + if ((*decoded_options)[j].opt_index == foption->opt_index)
> + break;
> + if (j == *decoded_options_count)
> + append_option (decoded_options, decoded_options_count, foption);
> + else if (foption->value != (*decoded_options)[j].value)
> + fatal_error (input_location,
> + "Option %s not used consistently in all LTO input"
> + " files", foption->orig_option_with_args_text);
> + break;
> +
>
> case OPT_foffload_:
> append_option (decoded_options, decoded_options_count, foption);
> break;
> }
> }
> +
> + /* Merge PIC options:
> + -fPIC + -fpic = -fpic
> + -fPIC + -fno-pic = -fno-pic
> + -fpic/-fPIC + nothin = nothing.
> + 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.
> +
> + It would be good to warn on mismatches, but it is bit hard to do as
> + we do not know what nothing translates to. */
> +
> + for (unsigned int j = 0; j < *decoded_options_count;)
> + 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)
> + {
> + (*decoded_options)[j] = *pic_option;
> + j++;
> + }
> + else
> + j++;
> + }
> + 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)
> + {
> + (*decoded_options)[j] = *pie_option;
> + j++;
> + }
> + else
> + j++;
> + }
> + else
> + j++;
> }
>
> /* Auxiliary function that frees elements of PTR and PTR itself.
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: Better merging of -fPIC/pic/PIE/pie in lto-wrapper
2017-07-17 8:17 ` Richard Biener
@ 2017-07-17 9:24 ` Jan Hubicka
0 siblings, 0 replies; 3+ messages in thread
From: Jan Hubicka @ 2017-07-17 9:24 UTC (permalink / raw)
To: Richard Biener; +Cc: GCC Patches
> On Sat, Jul 8, 2017 at 1:03 PM, Jan Hubicka <hubicka@ucw.cz> wrote:
> > Hi,
> > PR lto/80838 is about lto+profiledbootstrapped compiler being slower than
> > profiledboostrapped compiler.
> >
> > This is caused by a fact that some of object files linked into cc1plus binary
> > are built with -fPIC and lto-wrapper then decides to make whole binary PIC that
> > is very slow. While we probably ought to avoid linking PIC code into static
> > binary but I saw similar issue with firefox and other programs.
> >
> > I do not think we want to support mixed PIC/non-PIC symbols internally, because
> > it would need quite some work and I do not see any reasonable use cases.
> >
> > This patch makes merging more realistic/agressive. Linking -fPIC and non-PIC
> > code together results in non-PIC binary and thus the corresponding flags are
> > dropped when mismatches occurs.
> >
> > It would be nice to warn about it, but I do not know how to make warning
> > meaningful on targets that are PIC by default.
> >
> > Bootstrapped/regtested x86_64-linux, plan to commit it tomorrow after
> > lto-boottrapping if there are no complains.
>
> Hum. I wonder if/why we can't ask the linker about the output binary kind?
Hmm, you are right. I forgot I implemented also that :).
I bleieve we need both - disable PIC when linker tell us we can and merge option
so we make sane decisions between pic/PIC, pie/PIE. And of course we have the
wonderful non-plugin path.
I bleieve we got code quality regression here because I forgot about clearing flag_shlib
for non-dynamic builds:
Index: lto-lang.c
===================================================================
--- lto-lang.c (revision 250245)
+++ lto-lang.c (working copy)
@@ -840,11 +840,13 @@
flag_pie is 2. */
flag_pie = MAX (flag_pie, flag_pic);
flag_pic = flag_pie;
+ flag_shlib = 0;
break;
case LTO_LINKER_OUTPUT_EXEC: /* Normal executable */
flag_pic = 0;
flag_pie = 0;
+ flag_shlib = 0;
break;
case LTO_LINKER_OUTPUT_UNKNOWN:
I will test this and verify that there is no code quality difference without the option merging
then.
Honza
>
> Richard.
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2017-07-17 9:24 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-07-08 11:03 Better merging of -fPIC/pic/PIE/pie in lto-wrapper Jan Hubicka
2017-07-17 8:17 ` Richard Biener
2017-07-17 9:24 ` Jan Hubicka
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).