Re: [PATCH, driver specs][2] Put -flto-partition= on the collect2 c/l

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

On Fri, Dec 7, 2018 at 1:24 AM Iain Sandoe <iain@sandoe.co.uk> wrote:
>
> Hi
>
> This got stuck in my stack of patches for LTO debug support, and I forgot to ping it…
>
> > On 22 Aug 2018, at 14:20, Richard Biener <richard.guenther@gmail.com> wrote:
> >
> > On Wed, Aug 22, 2018 at 2:56 PM Iain Sandoe <iain@sandoe.co.uk> wrote:
> >>
> >>
> >>> On 20 Aug 2018, at 11:01, Richard Biener <richard.guenther@gmail.com> wrote:
> >>>
> >>> On Sat, Aug 18, 2018 at 9:00 PM Iain Sandoe <iain@sandoe.co.uk> wrote:
> >>>>
> >>
> >>>> I plan on making Darwin default to fno-lto for link unless there’s an “flto*” on the link line, since otherwise there’s a process launch for every object on the c/l to do “nm”, which is quite heavy weight.  At present, ISTM if the compiler is configured with —enable-lto, the link line defaults to assuming that every object needs to be checked.
> >>>
> >>> I think we wanted to transparently handle LTO objects (similar to how
> >>> it works with a linker plugin).
> >>
> >> At present, we are not only calling nm for every object, but also dsymutil sometimes unnecessarily - since the assumption on the “maybe_lto” path is that an temporary file will be generated.
> >>
> >> as a headline - when I did the simplistic “default is to assume fno-lto” that took 10% off the bootstrap time (with a stage#1 compiler without the optimisation).
> >>
> >> - I have a patch under test as below + better fidelity of avoiding dsymutil runs.
> >>
> >>> Ideally collect2 wouldn't use nm but simple-object to inspect files
> >>> (but that doesn't have symbol table query support
> >>
> >> Hrm.my WIP had grow symtab awareness to support debug-section copy on mach-o
> >> (and the ELF impl. looks like it understands both symtab and relocs)
> >> - so maybe that’s not as far away as we might think.
> >>
> >>> though looking for LTO specific sections would have been better in the
> >>> first place - I'd suggest .gnu.lto_.symtab).
> >>
> >> I’ve done this noting that the symtab seems to be the last emitted section - is that why you chose it (for security, rather than just stopping as soon as we see a .gnu.lto_. section?)
> >
> > Ah, any .gnu.lto_.section would work as well I guess - I just picked
> > one that should be always
> > created.  .gnu.lto_.opts is another one.
>
> >> comments?
> >
> > OK.  Feel free to stop when recognizing any LTO section.  Btw, as you
> > include lto-section-names.h
> > you should probably use LTO_SECTION_NAME_PREFIX instead of hard-coding
> > .gnu.lto_.
>
> So I just look for the LTO_SECTION_NAME_PREFIX
>
> > I'm not sure if/how we should handle offload sections and if those
> > work at all without a
> > linker plugin currently?
>
> In this version, I look for the OFFLOAD_SECTION_NAME_PREFIX as well,
> although AFAICS collect2 itself has no specific handling of offload, the detection
> is done in lto-wrapper.
>
> OK for trunk now?

OK.

Thanks,
Richard.

> thanks
> Iain
>
>
> From: Iain Sandoe <iain@sandoe.co.uk>
> Date: Tue, 21 Aug 2018 12:55:02 +0100
> Subject: [PATCH] [PATCH, collect2] Use simple-object to find out if objects
>  contain LTO.
>
> This replaces the use of nm to search for the LTO common symbol marker
> and uses simple object to see if there's a section starting with
> ".gnu.lto_." or .gnu.offload_lto_
> ---
>  gcc/collect2.c | 120 +++++++++++++++++++++++--------------------------
>  1 file changed, 57 insertions(+), 63 deletions(-)
>
> diff --git a/gcc/collect2.c b/gcc/collect2.c
> index 60269682ec..dcbd3e18a6 100644
> --- a/gcc/collect2.c
> +++ b/gcc/collect2.c
> @@ -30,6 +30,8 @@ along with GCC; see the file COPYING3.  If not see
>  #include "tm.h"
>  #include "filenames.h"
>  #include "file-find.h"
> +#include "simple-object.h"
> +#include "lto-section-names.h"
>
>  /* TARGET_64BIT may be defined to use driver specific functionality. */
>  #undef TARGET_64BIT
> @@ -804,7 +806,9 @@ maybe_run_lto_and_relink (char **lto_ld_argv, char **object_lst,
>        /* Run the linker again, this time replacing the object files
>           optimized by the LTO with the temporary file generated by the LTO.  */
>        fork_execute ("ld", out_lto_ld_argv, HAVE_GNU_LD && at_file_supplied);
> -      post_ld_pass (true);
> +      /* We assume that temp files were created, and therefore we need to take
> +         that into account (maybe run dsymutil).  */
> +      post_ld_pass (/*temp_file*/true);
>        free (lto_ld_argv);
>
>        maybe_unlink_list (lto_o_files);
> @@ -814,10 +818,11 @@ maybe_run_lto_and_relink (char **lto_ld_argv, char **object_lst,
>        /* Our caller is relying on us to do the link
>           even though there is no LTO back end work to be done.  */
>        fork_execute ("ld", lto_ld_argv, HAVE_GNU_LD && at_file_supplied);
> -      post_ld_pass (false);
> +      /* No LTO objects were found, so no new temp file.  */
> +      post_ld_pass (/*temp_file*/false);
>      }
>    else
> -    post_ld_pass (true);
> +    post_ld_pass (false); /* No LTO objects were found, no temp file.  */
>  }
>
>  /* Main program.  */
> @@ -1710,7 +1715,7 @@ main (int argc, char **argv)
>         if (lto_mode != LTO_MODE_NONE)
>           maybe_run_lto_and_relink (ld1_argv, object_lst, object, false);
>         else
> -         post_ld_pass (false);
> +         post_ld_pass (/*temp_file*/false);
>
>         return 0;
>        }
> @@ -1780,7 +1785,7 @@ main (int argc, char **argv)
>  #ifdef COLLECT_EXPORT_LIST
>        maybe_unlink (export_file);
>  #endif
> -      post_ld_pass (false);
> +      post_ld_pass (/*temp_file*/false);
>
>        maybe_unlink (c_file);
>        maybe_unlink (o_file);
> @@ -1874,7 +1879,7 @@ main (int argc, char **argv)
>    else
>      {
>        fork_execute ("ld", ld2_argv, HAVE_GNU_LD && at_file_supplied);
> -      post_ld_pass (false);
> +      post_ld_pass (/*temp_file*/false);
>      }
>
>    /* Let scan_prog_file do any final mods (OSF/rose needs this for
> @@ -2332,38 +2337,52 @@ write_aix_file (FILE *stream, struct id *list)
>
>  /* Check to make sure the file is an LTO object file.  */
>
> +static int
> +has_lto_section (void *data, const char *name ATTRIBUTE_UNUSED,
> +                off_t offset ATTRIBUTE_UNUSED,
> +                off_t length ATTRIBUTE_UNUSED)
> +{
> +  int *found = (int *) data;
> +
> +  if (strncmp (name, LTO_SECTION_NAME_PREFIX,
> +              sizeof (LTO_SECTION_NAME_PREFIX) - 1) != 0)
> +    {
> +      if (strncmp (name, OFFLOAD_SECTION_NAME_PREFIX,
> +                  sizeof (OFFLOAD_SECTION_NAME_PREFIX) - 1) != 0)
> +        return 1;
> +    }
> +
> +  *found = 1;
> +
> +  /* Stop iteration.  */
> +  return 0;
> +}
> +
>  static bool
> -maybe_lto_object_file (const char *prog_name)
> +is_lto_object_file (const char *prog_name)
>  {
> -  FILE *f;
> -  unsigned char buf[4];
> -  int i;
> +  const char *errmsg;
> +  int err;
> +  int found = 0;
> +  off_t inoff = 0;
> +  int infd = open (prog_name, O_RDONLY | O_BINARY);
>
> -  static unsigned char elfmagic[4] = { 0x7f, 'E', 'L', 'F' };
> -  static unsigned char coffmagic[2] = { 0x4c, 0x01 };
> -  static unsigned char coffmagic_x64[2] = { 0x64, 0x86 };
> -  static unsigned char machomagic[4][4] = {
> -    { 0xcf, 0xfa, 0xed, 0xfe },
> -    { 0xce, 0xfa, 0xed, 0xfe },
> -    { 0xfe, 0xed, 0xfa, 0xcf },
> -    { 0xfe, 0xed, 0xfa, 0xce }
> -  };
> +  if (infd == -1)
> +    return false;
>
> -  f = fopen (prog_name, "rb");
> -  if (f == NULL)
> +  simple_object_read *inobj = simple_object_start_read (infd, inoff,
> +                                                       LTO_SEGMENT_NAME,
> +                                                       &errmsg, &err);
> +  if (!inobj)
>      return false;
> -  if (fread (buf, sizeof (buf), 1, f) != 1)
> -    buf[0] = 0;
> -  fclose (f);
>
> -  if (memcmp (buf, elfmagic, sizeof (elfmagic)) == 0
> -      || memcmp (buf, coffmagic, sizeof (coffmagic)) == 0
> -      || memcmp (buf, coffmagic_x64, sizeof (coffmagic_x64)) == 0)
> +  errmsg = simple_object_find_sections (inobj, has_lto_section,
> +                                       (void *) &found, &err);
> +  if (! errmsg && found)
>      return true;
> -  for (i = 0; i < 4; i++)
> -    if (memcmp (buf, machomagic[i], sizeof (machomagic[i])) == 0)
> -      return true;
>
> +  if (errmsg)
> +    fatal_error (0, "%s: %s\n", errmsg, xstrerror (err));
>    return false;
>  }
>
> @@ -2386,7 +2405,6 @@ scan_prog_file (const char *prog_name, scanpass which_pass,
>    int err;
>    char *p, buf[1024];
>    FILE *inf;
> -  int found_lto = 0;
>
>    if (which_pass == PASS_SECOND)
>      return;
> @@ -2394,8 +2412,13 @@ scan_prog_file (const char *prog_name, scanpass which_pass,
>    /* LTO objects must be in a known format.  This check prevents
>       us from accepting an archive containing LTO objects, which
>       gcc cannot currently handle.  */
> -  if (which_pass == PASS_LTOINFO && !maybe_lto_object_file (prog_name))
> -    return;
> +  if (which_pass == PASS_LTOINFO)
> +    {
> +      if(is_lto_object_file (prog_name)) {
> +       add_lto_object (&lto_objects, prog_name);
> +      }
> +      return;
> +    }
>
>    /* If we do not have an `nm', complain.  */
>    if (nm_file_name == 0)
> @@ -2450,12 +2473,7 @@ scan_prog_file (const char *prog_name, scanpass which_pass,
>      fatal_error (input_location, "can't open nm output: %m");
>
>    if (debug)
> -    {
> -      if (which_pass == PASS_LTOINFO)
> -        fprintf (stderr, "\nnm output with LTO info marker symbol.\n");
> -      else
> -        fprintf (stderr, "\nnm output with constructors/destructors.\n");
> -    }
> +    fprintf (stderr, "\nnm output with constructors/destructors.\n");
>
>    /* Read each line of nm output.  */
>    while (fgets (buf, sizeof buf, inf) != (char *) 0)
> @@ -2466,30 +2484,6 @@ scan_prog_file (const char *prog_name, scanpass which_pass,
>        if (debug)
>          fprintf (stderr, "\t%s\n", buf);
>
> -      if (which_pass == PASS_LTOINFO)
> -        {
> -          if (found_lto)
> -            continue;
> -
> -          /* Look for the LTO info marker symbol, and add filename to
> -             the LTO objects list if found.  */
> -          for (p = buf; (ch = *p) != '\0' && ch != '\n'; p++)
> -            if (ch == ' '  && p[1] == '_' && p[2] == '_'
> -               && (strncmp (p + (p[3] == '_' ? 2 : 1), "__gnu_lto_v1", 12) == 0)
> -               && ISSPACE (p[p[3] == '_' ? 14 : 13]))
> -              {
> -                add_lto_object (&lto_objects, prog_name);
> -
> -                /* We need to read all the input, so we can't just
> -                   return here.  But we can avoid useless work.  */
> -                found_lto = 1;
> -
> -                break;
> -              }
> -
> -         continue;
> -        }
> -
>        /* If it contains a constructor or destructor name, add the name
>          to the appropriate list unless this is a kind of symbol we're
>          not supposed to even consider.  */
> --
> 2.17.1
>
>
>