* [PATCH] libgccjit cleanups
@ 2014-12-06 20:29 Ulrich Drepper
2014-12-08 16:41 ` David Malcolm
0 siblings, 1 reply; 4+ messages in thread
From: Ulrich Drepper @ 2014-12-06 20:29 UTC (permalink / raw)
To: gcc-patches
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.
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);
}
/* 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,
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] libgccjit cleanups
2014-12-06 20:29 [PATCH] libgccjit cleanups Ulrich Drepper
@ 2014-12-08 16:41 ` David Malcolm
2014-12-11 4:32 ` Ulrich Drepper
0 siblings, 1 reply; 4+ messages in thread
From: David Malcolm @ 2014-12-08 16:41 UTC (permalink / raw)
To: Ulrich Drepper; +Cc: gcc-patches
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] libgccjit cleanups
2014-12-08 16:41 ` David Malcolm
@ 2014-12-11 4:32 ` Ulrich Drepper
2014-12-11 17:58 ` David Malcolm
0 siblings, 1 reply; 4+ messages in thread
From: Ulrich Drepper @ 2014-12-11 4:32 UTC (permalink / raw)
To: David Malcolm; +Cc: GCC Patches
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.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] libgccjit cleanups
2014-12-11 4:32 ` Ulrich Drepper
@ 2014-12-11 17:58 ` David Malcolm
0 siblings, 0 replies; 4+ messages in thread
From: David Malcolm @ 2014-12-11 17:58 UTC (permalink / raw)
To: Ulrich Drepper; +Cc: GCC Patches
On Wed, 2014-12-10 at 23:32 -0500, Ulrich Drepper wrote:
> 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.
...as r218617. Thanks.
The jit subdirectory has its own ChangeLog file. I realize now that
your ChangeLog entries went in gcc/ChangeLog; they should have been in
gcc/jit/ChangeLog.
Sorry for not spotting that in review. I've fixed it in r218637.
Does your editor do some kind of auto-reindent? FWIW, I see various
whitespace-only changes in that commit. I assume we try to avoid such
changes
I've added documentation of libgccjit++.h to the .rst files as of
yesterday. So I've added documentation of the new function as r218636:
gcc/jit/ChangeLog:
* docs/cp/topics/contexts.rst (gccjit::context::set_str_option):
Document new function.
* docs/_build/texinfo/libgccjit.texi: Regenerate.
---
gcc/jit/docs/cp/topics/contexts.rst | 15 ++++++++++++---
1 file changed, 12 insertions(+), 3 deletions(-)
diff --git a/gcc/jit/docs/cp/topics/contexts.rst b/gcc/jit/docs/cp/topics/contexts.rst
index 72815fb..4becd51 100644
--- a/gcc/jit/docs/cp/topics/contexts.rst
+++ b/gcc/jit/docs/cp/topics/contexts.rst
@@ -148,9 +148,18 @@ Debugging
Options
-------
-..
- FIXME: gccjit::context::set_str_option doesn't seem to exist yet in the
- C++ API
+String Options
+**************
+
+.. function:: void \
+ gccjit::context::set_str_option (enum gcc_jit_str_option, \
+ const char *value)
+
+ Set a string option of the context.
+
+ This is a thin wrapper around the C API
+ :c:func:`gcc_jit_context_set_str_option`; the options have the same
+ meaning.
Boolean options
***************
--
1.8.5.3
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2014-12-11 17:58 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-12-06 20:29 [PATCH] libgccjit cleanups Ulrich Drepper
2014-12-08 16:41 ` David Malcolm
2014-12-11 4:32 ` Ulrich Drepper
2014-12-11 17:58 ` David Malcolm
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).