influence JIT linker command line

This patch supercedes the patch I sent earlier this week to add
dependencies to the linker command line.  The implementation is
different.

First, based on Dave's comment that he wants to keep the interface
simple, to enable the linker optimizations no new interface is added.
Instead optimizations are enabled in the linker whenever the compiler
optimizes, too.  I don't think this will create problems at all since
the time it takes nowadays is really low; it's only really measurable
for extremely large files.

The way to add dependencies is changed.  Instead of allowing an
unstructured string parameter to be added to the command line the new
proposed interface allows to introduce a dependency with possibly
information about the path the dependency is found.  This should be
useful and implementable if at some point the explicit linker invocation
is replaced by a library implementation.  The path argument of the new
interface is used differently depending on the name.  If the name is of
the form -l* then the -L option is used and a runpath is added.
Otherwise the path is used to locate the file.


Comments?

gcc/ChangeLog:

2014-12-06  Ulrich Drepper  <drepper@gmail.com>

	* jit/jit-recording.c (recording::context::add_dependency):
        New function.
        (recording::context::~context): Free newly added lists.
        * jit/jit-recording.h (recording::context): Add new
        member functions.
        * jit/libgccjit++.h (context): Add add_dependency member
        function.
        * jit/libgccjit.h: Declare gcc_jit_context_add_dependency.
        * jit/libgccjit.c: Define gcc_jit_context_add_dependency.
        * jit/libgccjit.map: Add gcc_jit_context_add_dependency.
	* jit/jit-playback.c (convert_to_dso): Add dependencies
        and library path arguments to the command line.
        * docs/topics/contexts.rst: Document new interface.

On Sat, 2014-12-06 at 15:56 -0500, Ulrich Drepper wrote:
> This patch supercedes the patch I sent earlier this week to add
> dependencies to the linker command line.  The implementation is
> different.
> 
> First, based on Dave's comment that he wants to keep the interface
> simple, to enable the linker optimizations no new interface is added.
> Instead optimizations are enabled in the linker whenever the compiler
> optimizes, too.  I don't think this will create problems at all since
> the time it takes nowadays is really low; it's only really measurable
> for extremely large files.
> 
> The way to add dependencies is changed.  Instead of allowing an
> unstructured string parameter to be added to the command line the new
> proposed interface allows to introduce a dependency with possibly
> information about the path the dependency is found.  This should be
> useful and implementable if at some point the explicit linker invocation
> is replaced by a library implementation.  The path argument of the new
> interface is used differently depending on the name.  If the name is of
> the form -l* then the -L option is used and a runpath is added.
> Otherwise the path is used to locate the file.
> 
> 
> Comments?

Thanks.

The special meaning of -l as a prefix to make the entrypoint have two
behaviors seems wrong to me.  Similarly, on reading the docs, the
repeated uses of the word "path" as both a param name and a concept seem
confusing, to me at least.

I think I'd prefer splitting the proposed
  gcc_jit_context_add_dependency
into two API entrypoints, one for the -l case, and another for the
filesystem name case.  I think the resulting API would be easier for the
end-user to understand.

Or is there a reason to have both of the -l vs non-dash-l uses go
through the same entrypoint?

"man ld" uses "namespec" as the term for the arg to -l, so maybe the API
could be two entrypoints looking something like this:

(A)

extern void
gcc_jit_context_requires_namespec (gcc_jit_context *ctxt,
                                   const char *namespec,
                                   const char *searchdir, /* can be NULL */
                                   int flags);

which becomes a "-lnamespec", and perhaps a "-Lsearchdir"/"-Rsearchdir"

(putting flags to the end so that we can default them to 0 in the C++
bindings).

and:

(B)

extern void
gcc_jit_context_requires_library (gcc_jit_context *ctxt,
                                  const char *path,
                                  int flags);

Do we need separate name and path params here?  If the user has name and
path, can't they simply supply us with the path to the object?  (i.e.
"path" can be absolute, relative, or just a filename)?

Or do we need to distinguish the possiblity that the linker might be
searching in its standard locations, hence something like:

extern void
gcc_jit_context_requires_library (gcc_jit_context *ctxt,
                                  const char *filename,
                                  const char *searchdir, /* can be NULL
*/
                                  int flags);


Again, I'm sorry if I'm not fully grokking your use-cases here.

Some possible concrete examples:

  gcc_jit_context_requires_namespec (ctxt, "foo", NULL, 0);

    adds a -lfoo to the link line, looking in the default places.

  gcc_jit_context_requires_namespec (ctxt, "bar",
                                    "/opt/bar-2.0/lib", 0);

    adds a -lbar to the link line, looking within the given dir first.

The same, but via the filesystem-based entrypoint:

  gcc_jit_context_requires_library (ctxt, "libfoo.so", 0);

  gcc_jit_context_requires_library (ctxt,
                                    "/opt/bar-2.0/libbar.so",
					 0);
and if I understand you, we can also do other kinds of object:
  gcc_jit_context_requires_library (ctxt,
                                    "/opt/bar-2.0/libbar.a",
					 0);
  gcc_jit_context_requires_library (ctxt,
                                    "/opt/bar-2.0/libbar.o",
					 0);

In the C++ binding API these would become just:

  ctxt.requires_namespec ("foo");
  ctxt.requires_namespec ("bar", "/opt/bar-2.0/lib");


  ctxt.requires_library ("libfoo.so");
  ctxt.requires_library ("/opt/bar-2.0/libbar.so");
  ctxt.requires_library ("/opt/bar-2.0/libbar.a");
  ctxt.requires_library ("/opt/bar-2.0/libbar.o");

Thoughts?

I'm not in love with the names I suggested (though sadly I've already
claimed the word "object" to mean something in the API, when it has its
own meaning in the linker world).

Maybe:
  import_by_{namespec|path}?
  import_{namespec|path}?
  link_by_{namespec|path}?
  link_to_{namespec|path}?
  add_library_by_{namespec|path|filename}?
etc

Various other stuff inline below.

> gcc/ChangeLog:
> 
> 2014-12-06  Ulrich Drepper  <drepper@gmail.com>
> 
> 	* jit/jit-recording.c (recording::context::add_dependency):
>         New function.
>         (recording::context::~context): Free newly added lists.
>         * jit/jit-recording.h (recording::context): Add new
>         member functions.
>         * jit/libgccjit++.h (context): Add add_dependency member
>         function.
>         * jit/libgccjit.h: Declare gcc_jit_context_add_dependency.
>         * jit/libgccjit.c: Define gcc_jit_context_add_dependency.
>         * jit/libgccjit.map: Add gcc_jit_context_add_dependency.
> 	* jit/jit-playback.c (convert_to_dso): Add dependencies
>         and library path arguments to the command line.
>         * docs/topics/contexts.rst: Document new interface.
> 
> 
> diff -u b/gcc/jit/jit-playback.c b/gcc/jit/jit-playback.c
> --- b/gcc/jit/jit-playback.c
> +++ b/gcc/jit/jit-playback.c
> @@ -1749,6 +1749,18 @@
>       time.  */
>    ADD_ARG ("-fno-use-linker-plugin");
>  
> +  /* Linker optimization.  We always tell the linker to optimize if the
> +     compiler is optimizing, too.  */
> +  if (get_int_option (GCC_JIT_INT_OPTION_OPTIMIZATION_LEVEL) > 0)
> +    ADD_ARG ("-Wl,-O");

I wonder if this could lead to issues on non-GNU linkers?  What does a
normal "gcc" driver invocation assume about the linker?  Does every
linker support a -O option?

(I know I said I wanted to have a simple API, so I'm sorry if I seem to
be changing my mind here)

> +  const char *s;
> +  for (unsigned i = 0; (s = get_dependency (i)); ++i)
> +    ADD_ARG (s);
> +
> +  for (unsigned i = 0; (s = get_library_path (i)); ++i)
> +    ADD_ARG (s);
> +
>    /* pex argv arrays are NULL-terminated.  */
>    ADD_ARG (NULL);
>  
> diff -u b/gcc/jit/jit-recording.c b/gcc/jit/jit-recording.c
> --- b/gcc/jit/jit-recording.c
> +++ b/gcc/jit/jit-recording.c
> @@ -218,6 +218,17 @@
>    for (i = 0; i < GCC_JIT_NUM_STR_OPTIONS; ++i)
>      free (m_str_options[i]);
>  
> +  char *s;
> +  FOR_EACH_VEC_ELT (m_dependencies, i, s)
> +    {
> +      free (s);
> +    }
> +
> +  FOR_EACH_VEC_ELT (m_library_path, i, s)
> +    {
> +      free (s);
> +    }

FWIW I have a class in another patch that subclasses
  auto_vec <char *> 
to free() all of the elements in the dtor (as well as the vec's own
buffer via auto_vec).  Presumably something like that could be added to
vec.h and used here, though it's fine to do it manually for now.

>    if (m_builtins_manager)
>      delete m_builtins_manager;
>  
> @@ -871,7 +882,66 @@
>    m_bool_options[opt] = value ? true : false;
>  }
>  
> -/* This mutex guards gcc::jit::recording::context::compile, so that only
> +/* Add the given library to the set of dependencies, or add an error
> +   if it's not recognized.
> +
> +   Implements the post-error-checking part of
> +   gcc_jit_context_add_dependency.  */
> +void
> +recording::context::add_dependency (const char *name, int flags,
> +				    const char *path)
> +{
> +  if (name == NULL)
> +    {
> +      add_error (NULL, "NULL library name");
> +      return;
> +    }
> +  /* So far no flags are defined.  */
> +  if (flags != 0)
> +    {
> +      add_error (NULL,
> +		 "unrecognized flags value: %i", flags);
> +      return;
> +    }
> +
> +  bool named_library = strncmp (name, "-l", 2);
> +
> +  if (strchr (name, '/') != NULL && (named_library || path != NULL))
> +    {
> +      add_error (NULL,
> +		 "path must be NULL unless simple file name is used");
> +      return;
> +    }


Most error-handling in the JIT API is in libgccjit.c, rather than
jit-recording.c.  One benefit is that the error-handling macros in
libgccjit.c use __func__, which means that the error message embeds the
name of the API entrypoint in use.

Hence this validation could happen in gcc_jit_context_add_dependency.

Though as I said above, I think I prefer splitting this into two public
API entrypoints, each with their own validation.


> +  if (named_library == 0 || path == NULL)

named_library is a bool, not an int, so the comparison against 0 seems
wrong.  Should it be an int?
Though if we split into two entrypoints, then the bool variable can go
away as it becomes implicit false vs true for each of the two
entrypoints.

> +    {
> +      m_dependencies.safe_push (xstrdup (name));
> +
> +      if (named_library)
> +	{
> +	  char *v;
> +	  asprintf (&v, "-Wl,-R,%s -L %s", path, path);
Could we have had a named_library with a NULL path? (or am I misreading
the bool vs int and the sense of the strncmp?)
If so, this asprintf looks wrong to me (and presumably we can just skip
the appending to the m_library_path?)

> +	  if (v == NULL)
> +	    {
> +	      add_error (NULL, "cannot allocate memory");
> +	      return;
> +	    }
> +	  m_library_path.safe_push (v);
> +	}
> +    }
> +  else
> +    {
> +      char *v;
> +      asprintf (&v, "%s/%s", path, name);
> +      if (v == NULL)
> +	{
> +	  add_error (NULL, "cannot allocate memory");
> +	  return;
> +	}
> +      m_dependencies.safe_push (v);
> +    }
> +}
> +
> +  /* This mutex guards gcc::jit::recording::context::compile, so that only
>     one thread can be accessing the bulk of GCC's state at once.  */
>  
>  static pthread_mutex_t jit_mutex = PTHREAD_MUTEX_INITIALIZER;
> diff -u b/gcc/jit/jit-recording.h b/gcc/jit/jit-recording.h
> --- b/gcc/jit/jit-recording.h
> +++ b/gcc/jit/jit-recording.h
> @@ -191,6 +191,10 @@
>    set_bool_option (enum gcc_jit_bool_option opt,
>  		   int value);
>  
> +  void
> +  add_dependency (const char *name, int flags,
> +		  const char *path);
> +
>    const char *
>    get_str_option (enum gcc_jit_str_option opt) const
>    {
> @@ -209,6 +213,22 @@
>      return m_bool_options[opt];
>    }
>  
> +  const char *
> +  get_dependency (unsigned idx) const
> +  {
> +    if (idx >= m_dependencies.length ())
> +      return NULL;
> +    return m_dependencies[idx];
> +  }
> +
> +  const char *
> +  get_library_path (unsigned idx) const
> +  {
> +    if (idx >= m_library_path.length ())
> +      return NULL;
> +    return m_library_path[idx];
> +  }
> +
>    result *
>    compile ();
>  
> @@ -250,6 +270,9 @@
>    int m_int_options[GCC_JIT_NUM_INT_OPTIONS];
>    bool m_bool_options[GCC_JIT_NUM_BOOL_OPTIONS];
>  
> +  auto_vec<char *> m_dependencies;
> +  auto_vec<char *> m_library_path;
> +
>    /* Recorded API usage.  */
>    auto_vec<memento *> m_mementos;
>  
> diff -u b/gcc/jit/libgccjit++.h b/gcc/jit/libgccjit++.h
> --- b/gcc/jit/libgccjit++.h
> +++ b/gcc/jit/libgccjit++.h
> @@ -108,6 +108,9 @@
>      void set_bool_option (enum gcc_jit_bool_option opt,
>  			  int value);
>  
> +    void add_dependency (const char *name, int flags = 0,
> +			 const char *path = NULL);
> +
>      location
>      new_location (const std::string &filename,
>  		  int line,
> @@ -561,6 +564,12 @@
>  
>  }
>  
> +inline void
> +context::add_dependency (const char *name, int flags, const char *path)
> +{
> +  gcc_jit_context_add_dependency (m_inner_ctxt, name, flags, path);
> +}
> +
>  inline location
>  context::new_location (const std::string &filename,
>  		       int line,
> diff -u b/gcc/jit/libgccjit.h b/gcc/jit/libgccjit.h
> --- b/gcc/jit/libgccjit.h
> +++ b/gcc/jit/libgccjit.h
> @@ -234,6 +234,16 @@
>  				 enum gcc_jit_bool_option opt,
>  				 int value);
>  
> +/* Add object as dependency.
> +
> +   The (const char *) values are not needed anymore after the call
> +   returns.  */
> +extern void
> +gcc_jit_context_add_dependency (gcc_jit_context *ctxt,
> +				const char *name,
> +				int flags,
> +				const char *path);
> +
>  /* This actually calls into GCC and runs the build, all
>     in a mutex for now.  The result is a wrapper around a .so file.
>     It can only be called once on a given context.  */
> only in patch2:
> unchanged:
> --- a/gcc/jit/docs/topics/contexts.rst
> +++ b/gcc/jit/docs/topics/contexts.rst
> @@ -325,3 +325,47 @@ Integer options
>       -O0 through -O3.
>  
>       The default value is 0 (unoptimized).
> +
> +  .. macro:: GCC_JIT_INT_OPTION_LINK_OPTIMIZATION_LEVEL
> +
> +     How much to optimize the code at the linking phase.
> +
> +     Valid values are 0 and 1.
> +
> +     The default value is 0 (unoptimized).

You're still documenting this option - though as I mentioned above,
maybe we *do* want it split out from the other one.  Not sure.

> +Dependencies
> +------------
> +
> +The JIT-created code might reference functions which are provided by
> +external libraries or is available in object files.

s/is/are/, surely, assuming you're referring to the functions again.

Presumably it's always legal to reference a function in "yourself" i.e.
where the client DSO using libgccjit references one of its own symbols?


> +.. function:: void gcc_jit_context_add_dependency (gcc_jit_context *ctxt, \
> +                                                  const char *name, \
> +                                                  int flags, \
> +                                                  const char *path)

Thanks; documentation patches make it much easier for me to see what the
intent is.

Concrete usage examples are even better :)
(see e.g. the examples I suggested above).

> +  Add the named object to the list of dependencies for the context.
> +
> +  .. name:: name of the file with the object to be added.  The string can
> +           have multiple forms:
> +
> +           - if the string starts with "-l" it names a library which
> +             is looked up using the usual rules a linker would follow,
> +             possibly extended by the :name `path` parameter.  The
> +             :name `name` must consist only of valid characters for a
> +             library name.
> +
> +           - if the string is a general file name it names a file
> +             available in the file system which can be accessible.  The
> +             file can be a library or an object file.
> +
> +           - if the string is a path name it must name the file.  In this
> +             case the :name `path` must be :macro:`NULL`.

As noted above, the multiple senses of "path" here feel unnecessarily
awkward (as both a param name, and a concept, where you can have a path
but must then have the thing in the API named "path" be NULL).


> +  .. flags:: this parameter is reserved for future use and for now must
> +            always be zero.
> +
> +  .. path:: this parameter provides the path of the file named in the
> +           ``name`` parameter.  See above for the rules for when this
> +           parameter must be set to :macro:`NULL`.


> only in patch2:
> unchanged:
> --- a/gcc/jit/jit-playback.h
> +++ b/gcc/jit/jit-playback.h
> @@ -175,6 +175,18 @@ public:
>      return m_recording_ctxt->get_bool_option (opt);
>    }
>  
> +  const char *
> +  get_dependency (unsigned idx) const
> +  {
> +    return m_recording_ctxt->get_dependency (idx);
> +  }
> +
> +  const char *
> +  get_library_path (unsigned idx) const
> +  {
> +    return m_recording_ctxt->get_library_path (idx);
> +  }
> +
>    builtins_manager *get_builtins_manager () const
>    {
>      return m_recording_ctxt->get_builtins_manager ();
> @@ -586,4 +598,3 @@ extern playback::context *active_playback_ctxt;
>  } // namespace gcc
>  
>  #endif /* JIT_PLAYBACK_H */
> -
> only in patch2:
> unchanged:
> --- a/gcc/jit/libgccjit.c
> +++ b/gcc/jit/libgccjit.c
> @@ -2004,6 +2004,24 @@ gcc_jit_context_set_bool_option (gcc_jit_context *ctxt,
>  /* Public entrypoint.  See description in libgccjit.h.
>  
>     After error-checking, the real work is done by the
> +   gcc::jit::recording::context::add_library_dependency method in
> +   jit-recording.c.  */
> +
> +void gcc_jit_context_add_dependency (gcc_jit_context *ctxt,
> +				     const char *name,
> +				     int flags,
> +				     const char *path)
> +{
> +  RETURN_IF_FAIL (ctxt, NULL, NULL, "NULL context");
> +  /* flags is checked in the inner function.  */

...as are name and path.  As noted above, I think it's better to move
this checking from there into here (libgccjit.c) using the macros, so
that they can use __func__ and thus report the API entrypoint.

> +  ctxt->add_dependency (name, flags, path);
> +}
> +
> +
> +/* Public entrypoint.  See description in libgccjit.h.
> +
> +   After error-checking, the real work is done by the
>     gcc::jit::recording::context::compile method in
>     jit-recording.c.  */
>  
> only in patch2:
> unchanged:
> --- a/gcc/jit/libgccjit.map
> +++ b/gcc/jit/libgccjit.map
> @@ -31,6 +31,7 @@
>      gcc_jit_block_end_with_void_return;
>      gcc_jit_block_get_function;
>      gcc_jit_context_acquire;
> +    gcc_jit_context_add_dependency;
>      gcc_jit_context_compile;
>      gcc_jit_context_dump_to_file;
>      gcc_jit_context_get_builtin_function;
> @@ -97,4 +98,4 @@
>      gcc_jit_type_get_volatile;
>  
>    local: *;
> -};
> \ No newline at end of file
> +};

Thanks again.  Hope the above seems sane.

Dave