libgccjit cleanups

This patch broken out of one I sent earlier with some extensions.  It
contains only little cleanups to the libgccjit code.

When creating the linker command line the code now uses an auto_vec
instead of the fixed size array.

The second change adds the missing context::set_str_option member
function to the C++ interface.

The third change it to the string option handling.  Instead of just
using the pointer passed to the function the code now makes a copy
of the string.


OK?


gcc/ChangeLog:

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

	* jit/jit-playback.c (convert_to_dso): Use auto_vec instead
        of automatic array to build up command line.
        * jit/jit-recording.c (recording::context::set_str_option):
        Make copy of the string.
        (recording::context::~context): Free string options.
        * jit/jit-recording.h (recording::context): Adjust type
        of m_str_options member.
        * jit/libgccjit.h: Adjust comment about
        gcc_jit_context_set_str_option parameter begin used after
        the call.
        * jit/libgccjit++.h (gccjit::context): Add set_str_option
        member function.

On Sat, 2014-12-06 at 15:29 -0500, Ulrich Drepper wrote:
> This patch broken out of one I sent earlier with some extensions.  It
> contains only little cleanups to the libgccjit code.
> 
> When creating the linker command line the code now uses an auto_vec
> instead of the fixed size array.
> 
> The second change adds the missing context::set_str_option member
> function to the C++ interface.
> 
> The third change it to the string option handling.  Instead of just
> using the pointer passed to the function the code now makes a copy
> of the string.
> 
> 
> OK?

Thanks.  Overall this is good, a few nitpicks inline below:

> gcc/ChangeLog:
> 
> 2014-12-06  Ulrich Drepper  <drepper@gmail.com>
> 
> 	* jit/jit-playback.c (convert_to_dso): Use auto_vec instead
>         of automatic array to build up command line.
>         * jit/jit-recording.c (recording::context::set_str_option):
>         Make copy of the string.
>         (recording::context::~context): Free string options.
>         * jit/jit-recording.h (recording::context): Adjust type
>         of m_str_options member.
>         * jit/libgccjit.h: Adjust comment about
>         gcc_jit_context_set_str_option parameter begin used after
>         the call.
>         * jit/libgccjit++.h (gccjit::context): Add set_str_option
>         member function.
> 
> 
> diff --git a/gcc/jit/jit-playback.c b/gcc/jit/jit-playback.c
> index ecdae80..6d1eb8a 100644
> --- a/gcc/jit/jit-playback.c
> +++ b/gcc/jit/jit-playback.c
> @@ -1726,18 +1726,19 @@ convert_to_dso (const char *ctxt_progname)
>       TV_ASSEMBLE.  */
>    auto_timevar assemble_timevar (TV_ASSEMBLE);
>    const char *errmsg;
> -  const char *argv[7];
> +  auto_vec <const char *> argvec;
> +#define ADD_ARG(arg) argvec.safe_push (arg)
>    int exit_status = 0;
>    int err = 0;
>    const char *gcc_driver_name = GCC_DRIVER_NAME;
>  
> -  argv[0] = gcc_driver_name;
> -  argv[1] = "-shared";
> +  ADD_ARG (gcc_driver_name);
> +  ADD_ARG ("-shared");
>    /* The input: assembler.  */
> -  argv[2] = m_path_s_file;
> +  ADD_ARG (m_path_s_file);
>    /* The output: shared library.  */
> -  argv[3] = "-o";
> -  argv[4] = m_path_so_file;
> +  ADD_ARG ("-o");
> +  ADD_ARG (m_path_so_file);
>  
>    /* Don't use the linker plugin.
>       If running with just a "make" and not a "make install", then we'd
> @@ -1746,17 +1747,17 @@ convert_to_dso (const char *ctxt_progname)
>       libto_plugin is a .la at build time, with it becoming installed with
>       ".so" suffix: i.e. it doesn't exist with a .so suffix until install
>       time.  */
> -  argv[5] = "-fno-use-linker-plugin";
> +  ADD_ARG ("-fno-use-linker-plugin");
>  
>    /* pex argv arrays are NULL-terminated.  */
> -  argv[6] = NULL;
> +  ADD_ARG (NULL);
>  
>    /* pex_one's error-handling requires pname to be non-NULL.  */
>    gcc_assert (ctxt_progname);
>  
>    errmsg = pex_one (PEX_SEARCH, /* int flags, */
>  		    gcc_driver_name,
> -		    const_cast<char * const *> (argv),
> +		    const_cast <char *const *> (argvec.address ()),
>  		    ctxt_progname, /* const char *pname */
>  		    NULL, /* const char *outname */
>  		    NULL, /* const char *errname */
> @@ -1783,6 +1784,7 @@ convert_to_dso (const char *ctxt_progname)
>  		 getenv ("PATH"));
>        return;
>      }
> +#undef ADD_ARG
>  }
>  
>  /* Top-level hook for playing back a recording context.
> diff --git a/gcc/jit/jit-recording.c b/gcc/jit/jit-recording.c
> --- a/gcc/jit/jit-recording.c
> +++ b/gcc/jit/jit-recording.c
> @@ -215,6 +215,9 @@ recording::context::~context ()
>        delete m;
>      }
>  
> +  for (i = 0; i < GCC_JIT_NUM_STR_OPTIONS; ++i)
> +    free (m_str_options[i]);
> +
>    if (m_builtins_manager)
>      delete m_builtins_manager;
>  
> @@ -827,7 +830,7 @@ recording::context::set_str_option (enum gcc_jit_str_option opt,
>  		 "unrecognized (enum gcc_jit_str_option) value: %i", opt);
>        return;
>      }
> -  m_str_options[opt] = value;
> +  m_str_options[opt] = xstrdup (value);
>  }

If the user repeatedly calls set_str_option on the same opt, this will
be a leak.

So something like:

  /* Free any existing value.  */
  free (m_str_options[opt]);
  m_str_options[opt] = xstrdup (value);


>  /* Set the given integer option for this context, or add an error if
> diff --git a/gcc/jit/jit-recording.h b/gcc/jit/jit-recording.h
> --- a/gcc/jit/jit-recording.h
> +++ b/gcc/jit/jit-recording.h
> @@ -246,7 +246,7 @@ private:
>    char *m_first_error_str;
>    bool m_owns_first_error_str;
>  
> -  const char *m_str_options[GCC_JIT_NUM_STR_OPTIONS];
> +  char *m_str_options[GCC_JIT_NUM_STR_OPTIONS];
>    int m_int_options[GCC_JIT_NUM_INT_OPTIONS];
>    bool m_bool_options[GCC_JIT_NUM_BOOL_OPTIONS];
>  
> diff --git a/gcc/jit/libgccjit++.h b/gcc/jit/libgccjit++.h
> --- a/gcc/jit/libgccjit++.h
> +++ b/gcc/jit/libgccjit++.h
> @@ -99,6 +99,9 @@ namespace gccjit
>      void dump_to_file (const std::string &path,
>  		       bool update_locations);
>  
> +    void set_str_option (enum gcc_jit_str_option opt,
> +			 const char *value);
> +
>      void set_int_option (enum gcc_jit_int_option opt,
>  			 int value);
>  
> @@ -535,6 +538,14 @@ context::dump_to_file (const std::string &path,
>  }
>  
>  inline void
> +context::set_str_option (enum gcc_jit_str_option opt,
> +			 const char *value)
> +{
> +  gcc_jit_context_set_str_option (m_inner_ctxt, opt, value);
> +
> +}
> +
> +inline void
>  context::set_int_option (enum gcc_jit_int_option opt,
>  			 int value)
>  {
> diff --git a/gcc/jit/libgccjit.h b/gcc/jit/libgccjit.h
> --- a/gcc/jit/libgccjit.h
> +++ b/gcc/jit/libgccjit.h
> @@ -213,8 +213,8 @@ enum gcc_jit_bool_option
>  
>  /* Set a string option on the given context.
>  
> -   The context directly stores the (const char *), so the passed string
> -   must outlive the context.  */
> +   The (const char *) value is not needed anymore after the call
> +   returns.  */
>  extern void
>  gcc_jit_context_set_str_option (gcc_jit_context *ctxt,
>  				enum gcc_jit_str_option opt,

Perhaps reword the comment to:

  The context takes a copy of the string, so the
  (const char *) buffer is not needed anymore after the call
  returns.  */

Also, elsewhere in the header, there's a comment:

 All (const char *) string arguments passed to these functions are
 copied, so you don't need to keep them around.  Note that this *isn't*
 the case for other parts of the API.

I believe that your fix removes the last place where the API relies on a
const char * outliving the context, so the last sentence of that comment
can be dropped.

OK with these fixes.

Thanks
Dave

On Mon, Dec 8, 2014 at 11:36 AM, David Malcolm <dmalcolm@redhat.com> wrote:
> Thanks.  Overall this is good, a few nitpicks inline below:

I've made the changes and checked in the patch.