Re: [PATCH 4/n] OpenMP 4.0 offloading infrastructure: lto-wrapper

[Jakub Jelinek <jakub@redhat.com>]

On Thu, Oct 02, 2014 at 07:14:57PM +0400, Ilya Verbin wrote:
> @@ -1296,6 +1297,9 @@ static const char *const standard_startfile_prefix_2
>     relative to the driver.  */
>  static const char *const tooldir_base_prefix = TOOLDIR_BASE_PREFIX;
>  
> +/* A prefix to be used when this is an accelerator compiler.  */
> +static const char *const accel_dir_suffix = ACCEL_DIR_SUFFIX;

Is ACCEL_DIR_SUFFIX here "" or something starting with "/ ?

> @@ -4122,15 +4126,15 @@ process_command (unsigned int decoded_options_count,
>      }
>  
>    gcc_assert (!IS_ABSOLUTE_PATH (tooldir_base_prefix));
> -  tooldir_prefix2 = concat (tooldir_base_prefix, spec_machine,
> +  tooldir_prefix2 = concat (tooldir_base_prefix, spec_host_machine,
>  			    dir_separator_str, NULL);
>  
>    /* Look for tools relative to the location from which the driver is
>       running, or, if that is not available, the configured prefix.  */
>    tooldir_prefix
>      = concat (gcc_exec_prefix ? gcc_exec_prefix : standard_exec_prefix,
> -	      spec_machine, dir_separator_str,
> -	      spec_version, dir_separator_str, tooldir_prefix2, NULL);
> +	      spec_host_machine, dir_separator_str, spec_version,
> +	      accel_dir_suffix, dir_separator_str, tooldir_prefix2, NULL);
>    free (tooldir_prefix2);
>  
>    add_prefix (&exec_prefixes,

The reason I'm asking is that otherwise it seems gcc.c heavily uses
dir_separator_str for the separators.  Don't have any experience with
targets where DIR_SEPARATOR is not / though, perhaps it is a non-issue.

> @@ -6878,8 +6882,8 @@ main (int argc, char **argv)
>  
>    /* Read specs from a file if there is one.  */
>  
> -  machine_suffix = concat (spec_machine, dir_separator_str,
> -			   spec_version, dir_separator_str, NULL);
> +  machine_suffix = concat (spec_host_machine, dir_separator_str, spec_version,
> +			   accel_dir_suffix, dir_separator_str, NULL);
>    just_machine_suffix = concat (spec_machine, dir_separator_str, NULL);
>  
>    specs_file = find_a_file (&startfile_prefixes, "specs", R_OK, true);
> @@ -6889,16 +6893,17 @@ main (int argc, char **argv)
>    else
>      init_spec ();
>  
> +#ifndef ACCEL_COMPILER
>    /* We need to check standard_exec_prefix/just_machine_suffix/specs
>       for any override of as, ld and libraries.  */
>    specs_file = (char *) alloca (strlen (standard_exec_prefix)
>  		       + strlen (just_machine_suffix) + sizeof ("specs"));
> -
>    strcpy (specs_file, standard_exec_prefix);
>    strcat (specs_file, just_machine_suffix);
>    strcat (specs_file, "specs");
>    if (access (specs_file, R_OK) == 0)
>      read_specs (specs_file, true, false);
> +#endif

Why do you want to disable specs reading for the accel compiler?
Then users won't have the possibility to override defaults etc. easily...

> @@ -7097,6 +7103,16 @@ main (int argc, char **argv)
>    obstack_grow (&collect_obstack, argv[0], strlen (argv[0]) + 1);
>    xputenv (XOBFINISH (&collect_obstack, char *));
>  
> +  if (strlen (OFFLOAD_TARGETS) > 0)
> +    {
> +      obstack_init (&collect_obstack);
> +      obstack_grow (&collect_obstack, "OFFLOAD_TARGET_NAMES=",
> +		    sizeof ("OFFLOAD_TARGET_NAMES=") - 1);
> +      obstack_grow (&collect_obstack, OFFLOAD_TARGETS,
> +		    strlen (OFFLOAD_TARGETS) + 1);
> +      xputenv (XOBFINISH (&collect_obstack, char *));
> +    }
> +

I'm surprised to see the obstack_init call here, but looking at
gcc.c, I'm surprised to see it in more places.

I've always thought that obstack_init was something you invoke generally
once on a given obstack object, then work with the obstack and then
obstack_free (..., NULL) it at the end.  Now, if it wants the obstack to be
live until exit, it just would not obstack_free it.  But calling
obstack_init on the already initialized obstack results IMHO in memory
leaks.  It should be initialized just once somewhere.

>    /* Set up to remember the pathname of the lto wrapper. */
>  
>    if (have_c)
> diff --git a/gcc/lto-wrapper.c b/gcc/lto-wrapper.c
> index 08fd090..2c9b503 100644
> --- a/gcc/lto-wrapper.c
> +++ b/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 OFFLOAD_TARGET_NAMES_ENV	"OFFLOAD_TARGET_NAMES"

Missing comment about the env var and what it is for.

> +/* Prepare a target image for offload TARGET, using mkoffload tool from
> +   COMPILER_PATH.  Return the name of the resultant object file.  */
> +
> +static char *
> +compile_offload_image (const char *target, const char *compiler_path,
> +		       unsigned in_argc, char *in_argv[])
> +{
> +  char *filename = NULL;
> +  char **argv;
> +  char *suffix
> +    = XALLOCAVEC (char, strlen ("/accel//mkoffload") + strlen (target) + 1);

Use sizeof ("/accel//mkoffload") + strlen (target) instead?

> +  strcpy (suffix, "/accel/");
> +  strcat (suffix, target);
> +  strcat (suffix, "/mkoffload");
> +
> +  char **paths = NULL;
> +  unsigned n_paths = parse_env_var (compiler_path, &paths, suffix);
> +
> +  const char *compiler = NULL;
> +  for (unsigned i = 0; i < n_paths; i++)
> +    if (access_check (paths[i], X_OK) == 0)
> +      {
> +	compiler = paths[i];
> +	break;
> +      }
> +
> +  if (!compiler)
> +    goto out;
> +
> +  /* Generate temporary output file name.  */
> +  filename = make_temp_file (".target.o");
> +
> +  struct obstack argv_obstack;
> +  obstack_init (&argv_obstack);
> +  obstack_ptr_grow (&argv_obstack, compiler);
> +  obstack_ptr_grow (&argv_obstack, "-o");
> +  obstack_ptr_grow (&argv_obstack, filename);
> +
> +  for (unsigned i = 1; i < in_argc; i++)
> +    obstack_ptr_grow (&argv_obstack, in_argv[i]);
> +  obstack_ptr_grow (&argv_obstack, NULL);
> +
> +  argv = XOBFINISH (&argv_obstack, char **);
> +  fork_execute (argv[0], argv, true);
> +  obstack_free (&argv_obstack, NULL);
> +
> + out:

The goto is probably unnecessary here, indenting all the lines by
4 more spaces and using if (compiler) { ... } instead doesn't need
any further warpping.

> +  free_array_of_ptrs ((void **) paths, n_paths);
> +  return filename;
> +}
> +

> +      /* By default linker does not discard .gnu.offload_lto_* sections.  */
> +      const char *linker_script = make_temp_file ("_linker_script.x");
> +      FILE *stream = fopen (linker_script, "w");
> +      if (!stream)
> +	fatal_error ("fopen %s: %m", linker_script);
> +      fprintf (stream, "SECTIONS { /DISCARD/ : { *("
> +		       OFFLOAD_SECTION_NAME_PREFIX "*) } }\n");
> +      fclose (stream);
> +      printf ("%s\n", linker_script);
> +
> +      goto finish;
> +    }

Does this work with gold?  Are there any other linkers that support plugins,
but don't support linker scripts this way?


	Jakub