[PATCH, lto-plugin] Make the plugin more -save-temps friendly.

[PATCH, lto-plugin] Make the plugin more -save-temps friendly.

[Iain Sandoe]

[-- Attachment #1: Type: text/plain, Size: 1185 bytes --]

Currently the lto plugin has a somewhat obscure incantation to get it to save its temp files, and at least one is not named in any sensible way for development examination.

This patch makes it follow the same approach as collect2. 

-save-temps causes the temp file to be named meaningfully, and for the relevant input files to be saved in CWD.
-v, —version causes the save actions to be output to stderr.

——

one can get this to happen by just putting -save-temps, -v on the regular link line or (for compatibility with the way the -debug flag works, by appending -plugin-opt=-save-temps, etc.

(the latter change might be a bit OTT, I was in two minds about deleting that part, but …)

tested on x86_64-linux-gnu.

OK for trunk?
Iain

lto-plugin/

	* lto-plugin.c (exec_lto_wrapper): Make the wrapper
	arguments filename more user-friendly.
	(file_exists, maybe_unlink): New.
	(cleanup_handler): Use maybe unlink to handle the
	case when temps should be saved.
	(process_option): Look for -v, —version, -save-temps.
	(onload): Record the linker output file name.
	Check for  -v, —version, -save-temps in GCC collect
	options environment.


[-- Attachment #2: lto-plugin-save-temps.diff --]
[-- Type: application/octet-stream, Size: 6288 bytes --]

From 1e329e6cb2cdbfe2d76b5a09b4c521b3c2e191da Mon Sep 17 00:00:00 2001
From: Iain Sandoe <iain@sandoe.co.uk>
Date: Tue, 14 May 2019 12:48:38 +0000
Subject: [PATCH] v1

---
 lto-plugin/lto-plugin.c | 97 ++++++++++++++++++++++++++++++++---------
 1 file changed, 76 insertions(+), 21 deletions(-)

diff --git a/lto-plugin/lto-plugin.c b/lto-plugin/lto-plugin.c
index 3788fdbb64..92bca50b09 100644
--- a/lto-plugin/lto-plugin.c
+++ b/lto-plugin/lto-plugin.c
@@ -41,6 +41,7 @@ along with this program; see the file COPYING3.  If not see
 #if HAVE_STDINT_H
 #include <stdint.h>
 #endif
+#include <stdbool.h>
 #include <assert.h>
 #include <errno.h>
 #include <string.h>
@@ -184,12 +185,15 @@ static int lto_wrapper_num_args;
 static char **pass_through_items = NULL;
 static unsigned int num_pass_through_items;
 
-static char debug;
+static bool debug;
+static bool save_temps;
+static bool verbose;
 static char nop;
 static char *resolution_file = NULL;
 static enum ld_plugin_output_file_type linker_output;
 static int linker_output_set;
 static int linker_output_known;
+static const char *link_output_name = NULL;
 
 /* The version of gold being used, or -1 if not gold.  The number is
    MAJOR * 100 + MINOR.  */
@@ -560,8 +564,17 @@ exec_lto_wrapper (char *argv[])
   struct pex_obj *pex;
   const char *errmsg;
 
-  /* Write argv to a file to avoid a command line that is too long. */
-  arguments_file_name = make_temp_file ("");
+  /* Write argv to a file to avoid a command line that is too long
+     Save the file locally on save-temps.  */
+  if (save_temps && link_output_name)
+    {
+      arguments_file_name = (char *) xmalloc (strlen (link_output_name)
+				  + sizeof (".lto_wrapper_args") + 1);
+      strcpy (arguments_file_name, link_output_name);
+      strcat (arguments_file_name, ".lto_wrapper_args");
+    }
+  else
+     arguments_file_name = make_temp_file (".lto_wrapper_args");
   check (arguments_file_name, LDPL_FATAL,
          "Failed to generate a temorary file name");
 
@@ -579,15 +592,21 @@ exec_lto_wrapper (char *argv[])
   for (i = 1; argv[i]; i++)
     {
       char *a = argv[i];
+      /* Check the input argument list for a verbose marker too.  */
       if (a[0] == '-' && a[1] == 'v' && a[2] == '\0')
 	{
-	  for (i = 0; argv[i]; i++)
-	    fprintf (stderr, "%s ", argv[i]);
-	  fprintf (stderr, "\n");
+	  verbose = true;
 	  break;
 	}
     }
 
+  if (verbose)
+    {
+      for (i = 0; argv[i]; i++)
+	fprintf (stderr, "%s ", argv[i]);
+      fprintf (stderr, "\n");
+    }
+
   new_argv[0] = argv[0];
   new_argv[1] = at_args;
   new_argv[2] = NULL;
@@ -599,7 +618,6 @@ exec_lto_wrapper (char *argv[])
       fprintf (stderr, "\n");
     }
 
-
   pex = pex_init (PEX_USE_PIPES, "lto-wrapper", NULL);
   check (pex != NULL, LDPL_FATAL, "could not pex_init lto-wrapper");
 
@@ -759,6 +777,29 @@ all_symbols_read_handler (void)
   return LDPS_OK;
 }
 
+/* Helper, as used in collect2.  */
+static int
+file_exists (const char *name)
+{
+  return access (name, R_OK) == 0;
+}
+
+/* Unlink FILE unless we have save-temps set.
+   Note that we're saving files if verbose output is set. */
+
+static void
+maybe_unlink (const char *file)
+{
+  if (save_temps && file_exists (file))
+    {
+      if (verbose)
+	fprintf (stderr, "[Leaving %s]\n", file);
+      return;
+    }
+
+  unlink_if_ordinary (file);
+}
+
 /* Remove temporary files at the end of the link. */
 
 static enum ld_plugin_status
@@ -771,16 +812,10 @@ cleanup_handler (void)
     return LDPS_OK;
 
   if (arguments_file_name)
-    {
-      t = unlink (arguments_file_name);
-      check (t == 0, LDPL_FATAL, "could not unlink arguments file");
-    }
+    maybe_unlink (arguments_file_name);
 
   for (i = 0; i < num_output_files; i++)
-    {
-      t = unlink (output_files[i]);
-      check (t == 0, LDPL_FATAL, "could not unlink output file");
-    }
+    maybe_unlink (output_files[i]);
 
   free_2 ();
   return LDPS_OK;
@@ -1143,7 +1178,12 @@ process_option (const char *option)
   if (strcmp (option, "-linker-output-known") == 0)
     linker_output_known = 1;
   if (strcmp (option, "-debug") == 0)
-    debug = 1;
+    debug = true;
+  else if ((strcmp (option, "-v") == 0)
+           || (strcmp (option, "--verbose") == 0))
+    verbose = true;
+  else if (strcmp (option, "-save-temps") == 0)
+    save_temps = true;
   else if (strcmp (option, "-nop") == 0)
     nop = 1;
   else if (!strncmp (option, "-pass-through=", strlen("-pass-through=")))
@@ -1180,6 +1220,8 @@ process_option (const char *option)
       if (strncmp (option, "-fresolution=", sizeof ("-fresolution=") - 1) == 0)
 	resolution_file = opt + sizeof ("-fresolution=") - 1;
     }
+  save_temps = save_temps || debug;
+  verbose = verbose || debug;
 }
 
 /* Called by gold after loading the plugin. TV is the transfer vector. */
@@ -1232,6 +1274,10 @@ onload (struct ld_plugin_tv *tv)
 	  linker_output = (enum ld_plugin_output_file_type) p->tv_u.tv_val;
 	  linker_output_set = 1;
 	  break;
+	case LDPT_OUTPUT_NAME:
+	  /* We only use this to make user-friendly temp file names.  */
+	  link_output_name = p->tv_u.tv_string;
+	  break;
 	default:
 	  break;
 	}
@@ -1259,12 +1305,21 @@ onload (struct ld_plugin_tv *tv)
 	     "could not register the all_symbols_read callback");
     }
 
-  /* Support -fno-use-linker-plugin by failing to load the plugin
-     for the case where it is auto-loaded by BFD.  */
   char *collect_gcc_options = getenv ("COLLECT_GCC_OPTIONS");
-  if (collect_gcc_options
-      && strstr (collect_gcc_options, "'-fno-use-linker-plugin'"))
-    return LDPS_ERR;
+  if (collect_gcc_options)
+    {
+      /* Support -fno-use-linker-plugin by failing to load the plugin
+	 for the case where it is auto-loaded by BFD.  */
+      if (strstr (collect_gcc_options, "'-fno-use-linker-plugin'"))
+	return LDPS_ERR;
+
+      if ( strstr (collect_gcc_options, "'-save-temps'"))
+	save_temps = true;
+
+      if (strstr (collect_gcc_options, "'-v'")
+          || strstr (collect_gcc_options, "'--verbose'"))
+	verbose = true;
+    }
 
   return LDPS_OK;
 }
-- 
2.17.1

Re: [PATCH, lto-plugin] Make the plugin more -save-temps friendly.

[Richard Biener]

[-- Attachment #1: Type: text/plain, Size: 1480 bytes --]

On Tue, 14 May 2019, Iain Sandoe wrote:

> Currently the lto plugin has a somewhat obscure incantation to get it to save its temp files, and at least one is not named in any sensible way for development examination.
> 
> This patch makes it follow the same approach as collect2. 
> 
> -save-temps causes the temp file to be named meaningfully, and for the relevant input files to be saved in CWD.
> -v, —version causes the save actions to be output to stderr.
> 
> ——
> 
> one can get this to happen by just putting -save-temps, -v on the regular link line or (for compatibility with the way the -debug flag works, by appending -plugin-opt=-save-temps, etc.
> 
> (the latter change might be a bit OTT, I was in two minds about deleting that part, but …)
> 
> tested on x86_64-linux-gnu.
> 
> OK for trunk?

OK.

Thanks,
Richard.

> Iain
> 
> lto-plugin/
> 
> 	* lto-plugin.c (exec_lto_wrapper): Make the wrapper
> 	arguments filename more user-friendly.
> 	(file_exists, maybe_unlink): New.
> 	(cleanup_handler): Use maybe unlink to handle the
> 	case when temps should be saved.
> 	(process_option): Look for -v, —version, -save-temps.
> 	(onload): Record the linker output file name.
> 	Check for  -v, —version, -save-temps in GCC collect
> 	options environment.
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE Linux GmbH, Maxfeldstrasse 5, 90409 Nuernberg, Germany;
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah; HRB 21284 (AG Nürnberg)

Re: [PATCH, lto-plugin] Make the plugin more -save-temps friendly.

[Martin Liška]

On 5/14/19 6:45 PM, Iain Sandoe wrote:
> Currently the lto plugin has a somewhat obscure incantation to get it to save its temp files, and at least one is not named in any sensible way for development examination.
> 
> This patch makes it follow the same approach as collect2. 
> 
> -save-temps causes the temp file to be named meaningfully, and for the relevant input files to be saved in CWD.
> -v, —version causes the save actions to be output to stderr.
> 
> ——
> 
> one can get this to happen by just putting -save-temps, -v on the regular link line or (for compatibility with the way the -debug flag works, by appending -plugin-opt=-save-temps, etc.
> 
> (the latter change might be a bit OTT, I was in two minds about deleting that part, but …)
> 
> tested on x86_64-linux-gnu.
> 
> OK for trunk?
> Iain
> 
> lto-plugin/
> 
> 	* lto-plugin.c (exec_lto_wrapper): Make the wrapper
> 	arguments filename more user-friendly.
> 	(file_exists, maybe_unlink): New.
> 	(cleanup_handler): Use maybe unlink to handle the
> 	case when temps should be saved.
> 	(process_option): Look for -v, —version, -save-temps.
> 	(onload): Record the linker output file name.
> 	Check for  -v, —version, -save-temps in GCC collect
> 	options environment.
> 

Hi.

I see:

libtool: compile:  gcc -DHAVE_CONFIG_H -I. -I/home/marxin/Programming/gcc/lto-plugin -I/home/marxin/Programming/gcc/lto-plugin/../include -DHAVE_CONFIG_H -Wall -g -O2 -c /home/marxin/Programming/gcc/lto-plugin/lto-plugin.c  -fPIC -DPIC -o .libs/lto-plugin.o
/home/marxin/Programming/gcc/lto-plugin/lto-plugin.c: In function ‘cleanup_handler’:
/home/marxin/Programming/gcc/lto-plugin/lto-plugin.c:809:7: warning: unused variable ‘t’ [-Wunused-variable]
   int t;
       ^

Please take a look.
Martin

Re: [PATCH, lto-plugin] Make the plugin more -save-temps friendly.

[Iain Sandoe]

> On 15 May 2019, at 14:52, Martin Liška <mliska@suse.cz> wrote:
> 
> On 5/14/19 6:45 PM, Iain Sandoe wrote:
>> Currently the lto plugin has a somewhat obscure incantation to get it to save its temp files, and at least one is not named in any 

> 
> libtool: compile:  gcc -DHAVE_CONFIG_H -I. -I/home/marxin/Programming/gcc/lto-plugin -I/home/marxin/Programming/gcc/lto-plugin/../include -DHAVE_CONFIG_H -Wall -g -O2 -c /home/marxin/Programming/gcc/lto-plugin/lto-plugin.c  -fPIC -DPIC -o .libs/lto-plugin.o
> /home/marxin/Programming/gcc/lto-plugin/lto-plugin.c: In function ‘cleanup_handler’:
> /home/marxin/Programming/gcc/lto-plugin/lto-plugin.c:809:7: warning: unused variable ‘t’ [-Wunused-variable]
>   int t;

sorry about that, fixed
Iain

lto-plugin/

 2019-05-15  Iain Sandoe  <iain@sandoe.co.uk>
 
	* lto-plugin.c (cleanup_handler): Remove unused var.

Index: lto-plugin/lto-plugin.c
===================================================================
--- lto-plugin/lto-plugin.c	(revision 271211)
+++ lto-plugin/lto-plugin.c	(working copy)
@@ -806,7 +806,6 @@
 cleanup_handler (void)
 {
   unsigned int i;
-  int t;
 
   if (debug)
     return LDPS_OK;