This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [LTO] PATCH: Fix WPA errors seen with make -j2


Diego Novillo wrote:
On Mon, Oct 27, 2008 at 15:42, Ollie Wild <aaw@google.com> wrote:

Here's another possibility. Since the -fwpa (and implicitly -fltrans)
invocations of lto1 are driven by the linker driver and since it
already has code for passing environment variables, it probably
wouldn't be too much work to have collect2 (a) create a temporary
directory in the current build directory and (b) set TMPDIR to point
to the new directory. That would allow us to use the libiberty code
as is.

Sounds good to me. Long term, I like the idea of


1- Create a unique directory in $CWD
2- Emit all the temporary object files there
3- Clean up only if -save-temps is not given

But I would do this as a separate patch.  For now the important thing
is to fix the overwriting of object files.

One could argue that setting TMPDIR explicitly in collect2 can be a
negative surprise from a usability POV, but for now I don't mind
either way.  I'm more concerned with fixing the bug first.


It turns out that the alterations to libiberty to create a make_cwd_temp_file() are trivial, and have the advantage of using already battle-tested code; this seems preferable to playing games with TMPDIR just to avoid changing libiberty.

Attached below, then, is an alternative slant on this patch that creates unique temporary files, named with the traditional gcc pattern, in place of bogus*.mumble in the build directory. The patch also adds code to delete these files when no longer required, unless -debug is passed to collect2 (-Wl,-debug). Using -debug this rather than -save-temps seems slightly more appropriate since at this point in a WPA build we're (conceptually, at least from the user perspective) linking rather than compiling.

This patch seems to be performing well in current testing. Any thoughts?

--
Google UK Limited | Registered Office: Belgrave House, 76 Buckingham
Palace Road, London SW1W 9TQ | Registered in England Number: 3977902

LTO whole program analysis writes temporary intermediate files named
bogus0.lto.o, bogus1.lto.o, and so on, into the output directory.  If multiple
LTO links occur in that directory at the same time (parallel make, say),
intermediate files from one LTO link can overwrite those of the other while
it's still executing, causing it to fail in exciting and unpredictable ways.

This patch addresses the problem by replacing bogus*.lto.o files with
uniquely named temporary files, located in the build directory.  It also adds
deletion of these intermediate files, suppressed with -debug to collect2.


gcc/lto/ChangeLog
2008-10-28  Simon Baldwin  <simonb@google.com>

	* lto.c (lto_wpa_write_files): Create intermediate files with
	make_cwd_temp_file().
	(lto_maybe_unlink): New.  Delete intermediate WPA files unless
	WPA_SAVE_LTRANS is set.
	(lto_main): Call lto_maybe_unlink() for intermediate WPA files.
	* ltrans-driver: Do not strip directory from output files.

include/ChangeLog
2008-10-28  Simon Baldwin  <simonb@google.com>

	* libiberty.h (make_cwd_temp_file): New declaration.

libiberty/ChangeLog
2008-10-28  Simon Baldwin  <simonb@google.com>

	* make-temp-file.c (make_temp_file_common): Split out from original
	make_temp_file() function.
	* (make_temp_file): Call make_temp_file_common() with tmp dir.
	* (make_cwd_temp_file): New.  Call make_temp_file_common() with "./".


Index: gcc/lto/lto.c
===================================================================
--- gcc/lto/lto.c	(revision 141381)
+++ gcc/lto/lto.c	(working copy)
@@ -26,6 +26,7 @@ Boston, MA 02110-1301, USA.  */
 #include "toplev.h"
 #include "tree.h"
 #include "tm.h"
+#include "libiberty.h"
 #include "cgraph.h"
 #include "ggc.h"
 #include "tree-ssa-operands.h"
@@ -625,20 +626,7 @@ lto_wpa_write_files (void)
 
   for (i = 0; VEC_iterate (cgraph_node_set, lto_cgraph_node_sets, i, set); i++)
     {
-      size_t needed = 16;
-      size_t len = needed;
-      char * temp_filename = XNEWVEC (char, len);
-
-      do 
-	{
-	  if (needed > len)
-	    {
-	      len = needed;
-	      temp_filename = XRESIZEVEC (char, temp_filename, len);
-	    }
-	  needed = snprintf (temp_filename, len, "bogus%d.lto.o", i);
-	}
-      while (needed >= len);
+      char * temp_filename = make_cwd_temp_file (".lto.o");
 
       output_files[i] = temp_filename;
 
@@ -969,6 +957,20 @@ lto_fixup_decls (struct lto_file_decl_da
   pointer_set_destroy (seen);
 }
 
+/* Unlink a temporary LTRANS file unless requested otherwise.  */
+
+static void
+lto_maybe_unlink (const char *file)
+{
+  if (!getenv ("WPA_SAVE_LTRANS"))
+    {
+      if (unlink_if_ordinary (file))
+        error ("deleting LTRANS file %s: %m", file);
+    }
+  else
+    fprintf (stderr, "[Leaving LTRANS %s]\n", file);
+}
+
 void
 lto_main (int debug_p ATTRIBUTE_UNUSED)
 {
@@ -1114,7 +1116,10 @@ lto_main (int debug_p ATTRIBUTE_UNUSED)
       lto_execute_ltrans (output_files);
 
       for (i = 0; output_files[i]; ++i)
-	XDELETEVEC (output_files[i]);
+        {
+	  lto_maybe_unlink (output_files[i]);
+	  free (output_files[i]);
+        }
       XDELETEVEC (output_files);
     }
 
Index: gcc/lto/ltrans-driver
===================================================================
--- gcc/lto/ltrans-driver	(revision 141381)
+++ gcc/lto/ltrans-driver	(working copy)
@@ -52,7 +52,7 @@ inputlist="$@"
 outputlist=
 for input in $inputlist
 do
-  output=`basename $input .o`.ltrans.o
+  output=`dirname $input`/`basename $input .o`.ltrans.o
   outputlist="$outputlist $output"
 
   echo "$output: $input" >> $makefile
Index: gcc/collect2.c
===================================================================
--- gcc/collect2.c	(revision 141381)
+++ gcc/collect2.c	(working copy)
@@ -933,6 +933,10 @@ maybe_run_lto_and_relink (char **lto_ld_
 	  strcpy (tmp, ltrans_output_file);
 
 	  *lto_c_ptr++ = "-fwpa";
+
+	  /* Save intermediate WPA files in lto1 if debug.  */
+	  if (debug)
+	    putenv ("WPA_SAVE_LTRANS=1");
 	}
       else
 	fatal ("invalid LTO mode");
Index: include/libiberty.h
===================================================================
--- include/libiberty.h	(revision 141381)
+++ include/libiberty.h	(working copy)
@@ -215,6 +215,7 @@ extern char *choose_temp_base (void) ATT
 /* Return a temporary file name or NULL if unable to create one.  */
 
 extern char *make_temp_file (const char *) ATTRIBUTE_MALLOC;
+extern char *make_cwd_temp_file (const char *) ATTRIBUTE_MALLOC;
 
 /* Remove a link to a file unless it is special. */
 
Index: libiberty/make-temp-file.c
===================================================================
--- libiberty/make-temp-file.c	(revision 141381)
+++ libiberty/make-temp-file.c	(working copy)
@@ -80,6 +80,7 @@ static const char usrtmp[] =
 { DIR_SEPARATOR, 'u', 's', 'r', DIR_SEPARATOR, 't', 'm', 'p', 0 };
 static const char vartmp[] =
 { DIR_SEPARATOR, 'v', 'a', 'r', DIR_SEPARATOR, 't', 'm', 'p', 0 };
+static const char cwd[] = { '.', DIR_SEPARATOR, 0 };
 
 static char *memoized_tmpdir;
 
@@ -133,22 +134,15 @@ choose_tmpdir (void)
   return tmpdir;
 }
 
-/*
-
-@deftypefn Replacement char* make_temp_file (const char *@var{suffix})
-
-Return a temporary file name (as a string) or @code{NULL} if unable to
-create one.  @var{suffix} is a suffix to append to the file name.  The
-string is @code{malloc}ed, and the temporary file has been created.
-
-@end deftypefn
-
-*/
+/* Common function to create temporary files.
+   Returns a temporary file name (as a string) or NULL if unable to
+   create one.  SUFFIX is a suffix to append to the file name, and BASE
+   is a prefix.  The return string is allocated, and the temporary file
+   created before returning.  */
 
 char *
-make_temp_file (const char *suffix)
+make_temp_file_common (const char *suffix, const char *base)
 {
-  const char *base = choose_tmpdir ();
   char *temp_filename;
   int base_len, suffix_len;
   int fd;
@@ -179,3 +173,41 @@ make_temp_file (const char *suffix)
     abort ();
   return temp_filename;
 }
+
+/*
+
+@deftypefn Replacement char* make_temp_file (const char *@var{suffix})
+
+Return a temporary file name (as a string) or @code{NULL} if unable to
+create one.  @var{suffix} is a suffix to append to the file name.  The
+string is @code{malloc}ed, and the temporary file has been created.  The
+temporary file is created in an appropriate temporary directory.
+
+@end deftypefn
+
+*/
+
+char *
+make_temp_file (const char *suffix)
+{
+  return make_temp_file_common (suffix, choose_tmpdir ());
+}
+
+/*
+
+@deftypefn Replacement char* make_cwd_temp_file (const char *@var{suffix})
+
+Return a temporary file name (as a string) or @code{NULL} if unable to
+create one.  @var{suffix} is a suffix to append to the file name.  The
+string is @code{malloc}ed, and the temporary file has been created.  The
+temporary file is created in the current working directory.
+
+@end deftypefn
+
+*/
+
+char *
+make_cwd_temp_file (const char *suffix)
+{
+  return make_temp_file_common (suffix, cwd);
+}

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]