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


[ Adding DJ and Ian to the CC list for the libiberty changes in this patch. ]

On Tue, Oct 28, 2008 at 10:37, Simon Baldwin <simonb@google.com> wrote:
> 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?


> 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");

No space after '*'.

> +    }
> +  else
> +    fprintf (stderr, "[Leaving LTRANS %s]\n", file);

No extraneous output, please.  This can generate too much noise.



> 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);
> +}

The libiberty bits look fine to me, but changes to libiberty
affect more than GCC.  Ian, DJ, could you review these changes?
Thanks.


Diego.


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