Re: Better merging of -fPIC/pic/PIE/pie in lto-wrapper

[Richard Biener <richard dot guenther at gmail dot com>]

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.