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

[Ilya Verbin <iverbin at gmail dot com>]

On 08 Oct 12:26, Jakub Jelinek wrote:
> 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 "/ ?
> 
> 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.

It is "" for the host compiler and starts with "/" for the accel compiler.
Looking at gcc/configure.ac, there are a lot of paths with slashes.
So, I think it's ok to define ACCEL_DIR_SUFFIX in such a way.

> > +#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...

Bernd,
Do you need this ifndef for PTX?  Or I could remove it?

> > @@ -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.

Right, I removed obstack_init from this patch.

> > +#define OFFLOAD_TARGET_NAMES_ENV	"OFFLOAD_TARGET_NAMES"
> 
> Missing comment about the env var and what it is for.

Fixed.

> > +  char *suffix
> > +    = XALLOCAVEC (char, strlen ("/accel//mkoffload") + strlen (target) + 1);
> 
> Use sizeof ("/accel//mkoffload") + strlen (target) instead?

Done.

> > + 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.

Done.

> > +      /* 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?

Oops, gold does not support scripts, outputted from plugins :(
"error: SECTIONS seen after other input files; try -T/--script"

Probably, we should update default linker scripts in binutils?
But without latest ld/gold all binaries compiled without -flto and with
offloading will contain intermediate bytecode...

Thanks,
  -- Ilya