diff mbox series

Communicate lto-wrapper and ld through a file

Message ID 20191016174600.leadenwgkgb63bxd@smtp.gmail.com
State New
Headers show
Series Communicate lto-wrapper and ld through a file | expand

Commit Message

Giuliano Belinassi Oct. 16, 2019, 5:46 p.m. UTC
Hi,

Previously, the lto-wrapper communicates with ld by creating a pipe from
lto-wrapper's stdout to ld's stdin. This patch uses a temporary file for
this communication, releasing stdout to be used for debugging.

I've run a full testsuite and bootstrapped LTO in a linux x86_64, and found
no issues so far. Do I need to write a testcase for this feature?

Giuliano.

gcc/ChangeLog
2019-10-16  Giuliano Belinassi  <giuliano.belinassi@usp.br>

    * lto-wrapper.c (STATIC_LEN): New macro.
    (to_ld): New.
    (find_crtofftable): Print to file to_ld.
    (find_ld_list_file): New.
    (main): Check if to_ld is valid or is stdout.

gcc/libiberty
2019-10-16  Giuliano Belinassi  <giuliano.belinassi@usp.br>

    * pex-unix.c (pex_unix_exec_child): check for PEX_KEEP_STD_IO flag.
    (to_ld): New.

gcc/include
19-10-16  Giuliano Belinassi  <giuliano.belinassi@usp.br>

    * libiberty.h (PEX_KEEP_STD_IO): New macro.

gcc/lto-plugin
2019-10-16  Giuliano Belinassi  <giuliano.belinassi@usp.br>

    * lto-plugin.c (exec_lto_wrapper): Replace pipe from stdout to temporary
    file, and pass its name in argv.

Comments

Richard Biener Oct. 17, 2019, 7:52 a.m. UTC | #1
On Wed, Oct 16, 2019 at 7:46 PM Giuliano Belinassi
<giuliano.belinassi@usp.br> wrote:
>
> Hi,
>
> Previously, the lto-wrapper communicates with ld by creating a pipe from
> lto-wrapper's stdout to ld's stdin. This patch uses a temporary file for
> this communication, releasing stdout to be used for debugging.

I'm not sure it is a good idea on its own.  Then you have to consider that
the lto-plugin is used to drive different GCC versions (and thus lto-wrappers)
and you are breaking compatibility with older versions which makes it
really not an option.

There's stderr for debugging...

> I've run a full testsuite and bootstrapped LTO in a linux x86_64, and found
> no issues so far. Do I need to write a testcase for this feature?
>
> Giuliano.
>
> gcc/ChangeLog
> 2019-10-16  Giuliano Belinassi  <giuliano.belinassi@usp.br>
>
>     * lto-wrapper.c (STATIC_LEN): New macro.
>     (to_ld): New.
>     (find_crtofftable): Print to file to_ld.
>     (find_ld_list_file): New.
>     (main): Check if to_ld is valid or is stdout.
>
> gcc/libiberty
> 2019-10-16  Giuliano Belinassi  <giuliano.belinassi@usp.br>
>
>     * pex-unix.c (pex_unix_exec_child): check for PEX_KEEP_STD_IO flag.
>     (to_ld): New.
>
> gcc/include
> 19-10-16  Giuliano Belinassi  <giuliano.belinassi@usp.br>
>
>     * libiberty.h (PEX_KEEP_STD_IO): New macro.
>
> gcc/lto-plugin
> 2019-10-16  Giuliano Belinassi  <giuliano.belinassi@usp.br>
>
>     * lto-plugin.c (exec_lto_wrapper): Replace pipe from stdout to temporary
>     file, and pass its name in argv.
>
Giuliano Belinassi Oct. 17, 2019, 5:59 p.m. UTC | #2
Hi

On 10/17, Richard Biener wrote:
> On Wed, Oct 16, 2019 at 7:46 PM Giuliano Belinassi
> <giuliano.belinassi@usp.br> wrote:
> >
> > Hi,
> >
> > Previously, the lto-wrapper communicates with ld by creating a pipe from
> > lto-wrapper's stdout to ld's stdin. This patch uses a temporary file for
> > this communication, releasing stdout to be used for debugging.
> 
> I'm not sure it is a good idea on its own.

The issue is that debugging a gcc python plugin in LTO mode using pdb, the pdb
output will be redirected to ld. As a consequence, no messages from pdb will be
displayed and lto will crash, as we discussed in IRC.

Sure, one can use another python debugger such as wdb, as suggested in IRC, but
it seems really fragile that python stdout gets mixed with lto1 stdout.

The point of this patch is to fix this issue, since using stdout seems fragile
based on this information, as discussed in IRC.

> Then you have to consider that
> the lto-plugin is used to drive different GCC versions (and thus lto-wrappers)
> and you are breaking compatibility with older versions which makes it
> really not an option.

I didn't know about that, but I could address that considering that there are
two cases:

(1) lto-plugin  is outdated
(2) lto-wrapper is outdated

In (1), we have no issue at all because lto-wrapper will be launched without
the --to_ld_list and thus lto-wrapper will fallback to stdout, even if
lto-wrapper is patched.  If it is not, since it won't receive this parameter at
all, everything will work as before.

In (2), we have the problem that if lto-wrapper is present, it won't understand
the --to_ld_list parameter and it will crash. Therefore I could patch
lto-wrapper to have the following behaviour:

lto-wrapper will accept an extra parameter, say `--support-file`. If lto-plugin
launches lto-wrapper with this arguments and it returns success, then it is
known that lto-wrapper will accept the temporary file to output the file names
to ld. If it fails, then it is known that lto-wrapper is unpatched and
lto-plugin should proceed the old way.

However, I would like to know if this is a suitable solution, or if won't be
accepted for other reasons.

> 
> There's stderr for debugging...

One could do `sys.stdout = sys.stderr` in the python code, but it doesn't seems
to be an nice solution...

Giuliano.

> 
> > I've run a full testsuite and bootstrapped LTO in a linux x86_64, and found
> > no issues so far. Do I need to write a testcase for this feature?
> >
> > Giuliano.
> >
> > gcc/ChangeLog
> > 2019-10-16  Giuliano Belinassi  <giuliano.belinassi@usp.br>
> >
> >     * lto-wrapper.c (STATIC_LEN): New macro.
> >     (to_ld): New.
> >     (find_crtofftable): Print to file to_ld.
> >     (find_ld_list_file): New.
> >     (main): Check if to_ld is valid or is stdout.
> >
> > gcc/libiberty
> > 2019-10-16  Giuliano Belinassi  <giuliano.belinassi@usp.br>
> >
> >     * pex-unix.c (pex_unix_exec_child): check for PEX_KEEP_STD_IO flag.
> >     (to_ld): New.
> >
> > gcc/include
> > 19-10-16  Giuliano Belinassi  <giuliano.belinassi@usp.br>
> >
> >     * libiberty.h (PEX_KEEP_STD_IO): New macro.
> >
> > gcc/lto-plugin
> > 2019-10-16  Giuliano Belinassi  <giuliano.belinassi@usp.br>
> >
> >     * lto-plugin.c (exec_lto_wrapper): Replace pipe from stdout to temporary
> >     file, and pass its name in argv.
> >
diff mbox series

Patch

diff --git gcc/lto-wrapper.c gcc/lto-wrapper.c
index 9a7bbd0c022..d794b493d5d 100644
--- gcc/lto-wrapper.c
+++ gcc/lto-wrapper.c
@@ -49,6 +49,8 @@  along with GCC; see the file COPYING3.  If not see
 #include "lto-section-names.h"
 #include "collect-utils.h"
 
+#define STATIC_LEN(x) (sizeof (x) / sizeof (*x))
+
 /* Environment variable, used for passing the names of offload targets from GCC
    driver to lto-wrapper.  */
 #define OFFLOAD_TARGET_NAMES_ENV	"OFFLOAD_TARGET_NAMES"
@@ -74,6 +76,8 @@  static char *makefile;
 static unsigned int num_deb_objs;
 static const char **early_debug_object_names;
 
+static FILE *to_ld;
+
 const char tool_name[] = "lto-wrapper";
 
 /* Delete tempfiles.  Called from utils_cleanup.  */
@@ -955,7 +959,7 @@  find_crtoffloadtable (void)
 	/* The linker will delete the filename we give it, so make a copy.  */
 	char *crtoffloadtable = make_temp_file (".crtoffloadtable.o");
 	copy_file (crtoffloadtable, paths[i]);
-	printf ("%s\n", crtoffloadtable);
+	fprintf (to_ld, "%s\n", crtoffloadtable);
 	XDELETEVEC (crtoffloadtable);
 	break;
       }
@@ -1556,7 +1560,7 @@  cont1:
 	{
 	  find_crtoffloadtable ();
 	  for (i = 0; offload_names[i]; i++)
-	    printf ("%s\n", offload_names[i]);
+	    fprintf (to_ld, "%s\n", offload_names[i]);
 	  free_array_of_ptrs ((void **) offload_names, i);
 	}
     }
@@ -1686,12 +1690,12 @@  cont1:
 
   if (lto_mode == LTO_MODE_LTO)
     {
-      printf ("%s\n", flto_out);
+      fprintf (to_ld, "%s\n", flto_out);
       if (!skip_debug)
 	{
 	  for (i = 0; i < ltoobj_argc; ++i)
 	    if (early_debug_object_names[i] != NULL)
-	      printf ("%s\n", early_debug_object_names[i]);	      
+	      fprintf (to_ld, "%s\n", early_debug_object_names[i]);
 	}
       /* These now belong to collect2.  */
       free (flto_out);
@@ -1866,15 +1870,15 @@  cont:
 	}
       for (i = 0; i < nr; ++i)
 	{
-	  fputs (output_names[i], stdout);
-	  putc ('\n', stdout);
+	  fputs (output_names[i], to_ld);
+	  putc ('\n', to_ld);
 	  free (input_names[i]);
 	}
       if (!skip_debug)
 	{
 	  for (i = 0; i < ltoobj_argc; ++i)
 	    if (early_debug_object_names[i] != NULL)
-	      printf ("%s\n", early_debug_object_names[i]);	      
+	      fprintf (to_ld, "%s\n", early_debug_object_names[i]);
 	}
       nr = 0;
       free (ltrans_priorities);
@@ -1892,13 +1896,43 @@  cont:
   obstack_free (&argv_obstack, NULL);
 }
 
+/* Find out if lto-wrapper was called and output to lto-plugin will be in a file
+   instead of stdout.  */
+
+static const char *find_ld_list_file (int* argc, char *argv[])
+{
+  int i;
+  static const char param[] = "--to_ld_list=";
+
+  for (i = 1; i < *argc; i++)
+    {
+      if (argv[i] != NULL && !strncmp (argv[i], param, STATIC_LEN (param) - 1))
+	{
+	  /* Found.  Retrieve the path to the file.  */
+
+	  /* This simple 'automata' just finds the first ':' of the string,
+	     and then return a pointer from now foward.  */
+	  const char *path = argv[i];
+	  argv[i] = NULL;
+	  (*argc)--;
+	  while (*path != '\0')
+	    if (*path++ == '=')
+		return path;
+	}
+    }
+
+
+  /* Not found.  */
+  return NULL;
+}
+
 
 /* Entry point.  */
 
 int
 main (int argc, char *argv[])
 {
-  const char *p;
+  const char *p, *files_list;
 
   init_opts_obstack ();
 
@@ -1934,11 +1968,22 @@  main (int argc, char *argv[])
   signal (SIGCHLD, SIG_DFL);
 #endif
 
+  files_list = find_ld_list_file (&argc, argv);
+  to_ld = files_list ? fopen (files_list, "a") : stdout;
+
+  if (!to_ld)
+    {
+      fprintf (stderr, "%s: failed to open parameter file.\n", argv[0]);
+      abort ();
+    }
+
   /* We may be called with all the arguments stored in some file and
      passed with @file.  Expand them into argv before processing.  */
   expandargv (&argc, &argv);
 
   run_gcc (argc, argv);
 
+  fclose (to_ld);
+
   return 0;
 }
diff --git include/libiberty.h include/libiberty.h
index 71192a29377..cc3940f91f3 100644
--- include/libiberty.h
+++ include/libiberty.h
@@ -406,6 +406,9 @@  extern void hex_init (void);
 /* Save files used for communication between processes.  */
 #define PEX_SAVE_TEMPS		0x4
 
+/* Don't close the standard I/O streams (stdout & stderr).  */
+#define PEX_KEEP_STD_IO		0x8
+
 /* Max number of alloca bytes per call before we must switch to malloc.
 
    ?? Swiped from gnulib's regex_internal.h header.  Is this actually
diff --git libiberty/pex-unix.c libiberty/pex-unix.c
index f9e5277897b..afa5d9cc288 100644
--- libiberty/pex-unix.c
+++ libiberty/pex-unix.c
@@ -651,14 +651,14 @@  pex_unix_exec_child (struct pex_obj *obj, int flags, const char *executable,
 	    else if (close (in) < 0)
 	      failed.fn = "close", failed.err = errno;
 	  }
-	if (!failed.fn && out != STDOUT_FILE_NO)
+	if (!failed.fn && out != STDOUT_FILE_NO && !(flags & PEX_KEEP_STD_IO))
 	  {
 	    if (dup2 (out, STDOUT_FILE_NO) < 0)
 	      failed.fn = "dup2", failed.err = errno;
 	    else if (close (out) < 0)
 	      failed.fn = "close", failed.err = errno;
 	  }
-	if (!failed.fn && errdes != STDERR_FILE_NO)
+	if (!failed.fn && errdes != STDERR_FILE_NO && !(flags & PEX_KEEP_STD_IO))
 	  {
 	    if (dup2 (errdes, STDERR_FILE_NO) < 0)
 	      failed.fn = "dup2", failed.err = errno;
diff --git lto-plugin/lto-plugin.c lto-plugin/lto-plugin.c
index 556e3ebcf76..40d25a30cfd 100644
--- lto-plugin/lto-plugin.c
+++ lto-plugin/lto-plugin.c
@@ -560,9 +560,10 @@  exec_lto_wrapper (char *argv[])
   char *at_args;
   FILE *args;
   FILE *wrapper_output;
-  char *new_argv[3];
+  char *new_argv[4];
   struct pex_obj *pex;
   const char *errmsg;
+  char *temp_filename;
 
   /* Write argv to a file to avoid a command line that is too long
      Save the file locally on save-temps.  */
@@ -607,10 +608,6 @@  exec_lto_wrapper (char *argv[])
       fprintf (stderr, "\n");
     }
 
-  new_argv[0] = argv[0];
-  new_argv[1] = at_args;
-  new_argv[2] = NULL;
-
   if (debug)
     {
       for (i = 0; new_argv[i]; i++)
@@ -618,26 +615,36 @@  exec_lto_wrapper (char *argv[])
       fprintf (stderr, "\n");
     }
 
-  pex = pex_init (PEX_USE_PIPES, "lto-wrapper", NULL);
+  pex = pex_init (0, "lto-wrapper", NULL);
   check (pex != NULL, LDPL_FATAL, "could not pex_init lto-wrapper");
 
-  errmsg = pex_run (pex, 0, new_argv[0], new_argv, NULL, NULL, &t);
-  check (errmsg == NULL, LDPL_FATAL, "could not run lto-wrapper");
-  check (t == 0, LDPL_FATAL, "could not run lto-wrapper");
+  temp_filename = make_temp_file ("-ld-files.lto");
 
-  wrapper_output = pex_read_output (pex, 0);
-  check (wrapper_output, LDPL_FATAL, "could not read lto-wrapper output");
+  new_argv[0] = argv[0];
+  new_argv[1] = at_args;
+  new_argv[2] = concat ("--to_ld_list=", temp_filename, NULL);
+  new_argv[3] = NULL;
 
-  add_output_files (wrapper_output);
+  errmsg = pex_run (pex, PEX_KEEP_STD_IO, new_argv[0], new_argv,
+		    temp_filename, NULL, &t);
+  check (errmsg == NULL, LDPL_FATAL, "could not run lto-wrapper");
+  check (t == 0, LDPL_FATAL, "could not run lto-wrapper");
 
   t = pex_get_status (pex, 1, &status);
   check (t == 1, LDPL_FATAL, "could not get lto-wrapper exit status");
   check (WIFEXITED (status) && WEXITSTATUS (status) == 0, LDPL_FATAL,
          "lto-wrapper failed");
 
-  pex_free (pex);
+  wrapper_output = fopen (temp_filename, "r");
+  check (wrapper_output, LDPL_FATAL, "could not read lto-wrapper output");
+
+  add_output_files (wrapper_output);
 
+  pex_free (pex);
+  free (temp_filename);
   free (at_args);
+  free (argv[2]);
+  fclose (wrapper_output);
 }
 
 /* Pass the original files back to the linker. */