On Thu, 2020-01-16 at 11:11 +0000, Andrea Corallo wrote:
> Hi, second version of the patch here cleaning up an unnecessary
> change.
>
> Does not introduce regressions with make check-jit.
>
> Andrea
>
> gcc/jit/ChangeLog
> 2020-??-?? Andrea Corallo <andrea.cora...@arm.com>
>
> * docs/topics/compatibility.rst (LIBGCCJIT_ABI_13): New ABI tag
> plus add version paragraph.
> * libgccjit++.h (namespace gccjit::version): Add new namespace.
> * libgccjit.c (gcc_jit_version_major, gcc_jit_version_minor)
> (gcc_jit_version_patchlevel): New functions.
> * libgccjit.h (LIBGCCJIT_HAVE_gcc_jit_version): New macro.
> (gcc_jit_version_major, gcc_jit_version_minor)
> (gcc_jit_version_patchlevel): New functions.
> * libgccjit.map (LIBGCCJIT_ABI_13) New ABI tag.
>
> gcc/testsuite/ChangeLog
> 2020-??-?? Andrea Corallo <andrea.cora...@arm.com>
>
> * jit.dg/test-version.c: New testcase.
[...]
Thanks for the patch; sorry for the delay in reviewing this.
Out of interest, do you have a specific use for this, or is it more
speculative?
> diff --git a/gcc/jit/libgccjit.c b/gcc/jit/libgccjit.c
> index 83055fc297b..572c82f053c 100644
> --- a/gcc/jit/libgccjit.c
> +++ b/gcc/jit/libgccjit.c
> @@ -23,6 +23,7 @@ along with GCC; see the file COPYING3. If not see
> #include "coretypes.h"
> #include "timevar.h"
> #include "typed-splay-tree.h"
> +#include "cppbuiltin.h"
>
> #include "libgccjit.h"
> #include "jit-recording.h"
> @@ -3175,3 +3176,27 @@ gcc_jit_context_new_rvalue_from_vector
> (gcc_jit_context *ctxt,
> as_vec_type,
> (gcc::jit::recording::rvalue **)elements);
> }
> +
> +extern int
> +gcc_jit_version_major (void)
> +{
> + int major, minor, patchlevel;
> + parse_basever (&major, &minor, &patchlevel);
> + return major;
> +}
> +
> +extern int
> +gcc_jit_version_minor (void)
> +{
> + int major, minor, patchlevel;
> + parse_basever (&major, &minor, &patchlevel);
> + return minor;
> +}
> +
> +extern int
> +gcc_jit_version_patchlevel (void)
> +{
> + int major, minor, patchlevel;
> + parse_basever (&major, &minor, &patchlevel);
> + return patchlevel;
> +}
My first thought here was that we should have a way to get all three at
once, but it turns out that parse_basever does its own caching
internally.
I don't think the current implementation is thread-safe; parse_basever
has:
static int s_major = -1, s_minor, s_patchlevel;
if (s_major == -1)
if (sscanf (BASEVER, "%d.%d.%d", &s_major, &s_minor, &s_patchlevel) != 3)
{
sscanf (BASEVER, "%d.%d", &s_major, &s_minor);
s_patchlevel = 0;
}
I think there's a race here: if two threads call parse_basever at the
same time, it looks like:
(1) thread A could set s_major
(2) thread B could read s_major, find it's set
(3) thread B could read the uninitialized s_minor
(4) thread A sets s_minor
and various similar issues.
One fix might be to add a version mutex to libgccjit.c; maybe something
like the following (caveat: I haven't tried compiling this):
/* A mutex around the cached state in parse_basever.
Ideally this would be within parse_basever, but the mutex is only needed
by libgccjit. */
static pthread_mutex_t version_mutex = PTHREAD_MUTEX_INITIALIZER;
struct version_info
{
/* Default constructor. Populate via parse_basever,
guarded by version_mutex. */
version_info ()
{
pthread_mutex_lock (&version_mutex);
parse_basever (&major, &minor, &patchlevel);
pthread_mutex_unlock (&version_mutex);
}
int major;
int minor;
int patchlevel;
};
int
gcc_jit_version_major (void)
{
version_info vi;
return vi.major;
}
int
gcc_jit_version_minor (void)
{
version_info vi;
return vi.minor;
}
int
gcc_jit_version_patchlevel (void)
{
version_info vi;
return vi.patchlevel;
}
Is adding a mutex a performance issue? How frequently are these going
to be called?
Alternatively, maybe make these functions take a gcc_jit_context and
cache the version information within the context? (since the API
requires multithreaded programs to use their own locking if threads
share a context)
Or some kind of caching in libgccjit.c? (perhaps simply by making the
version_info instances above static? my memory of C++ function-static
init rules and what we can rely on on our minimal compiler is a little
hazy)
> diff --git a/gcc/testsuite/jit.dg/test-version.c
> b/gcc/testsuite/jit.dg/test-version.c
> new file mode 100644
> index 00000000000..4338a00018b
> --- /dev/null
> +++ b/gcc/testsuite/jit.dg/test-version.c
> @@ -0,0 +1,26 @@
> +#include <stdlib.h>
> +#include <stdio.h>
> +
> +#include "libgccjit.h"
> +
> +#include "harness.h"
> +
> +#ifndef LIBGCCJIT_HAVE_gcc_jit_version
> +#error LIBGCCJIT_HAVE_gcc_jit_version was not defined
> +#endif
> +
> +void
> +create_code (gcc_jit_context *ctxt, void *user_data)
> +{
> + /* Do nothing. */
> +}
> +
> +void
> +verify_code (gcc_jit_context *ctxt, gcc_jit_result *result)
> +{
> + if (!gcc_jit_version_major ())
> + fail ("Major version is zero");
> + /* Minor and patchlevel can be zero. */
> + gcc_jit_version_minor ();
> + gcc_jit_version_patchlevel ();
> +}
Please add this testcase to the "testcases" array in
gcc/testsuite/jit.dg/all-non-failing-tests.h (see the comment towards
the end of jit/docs/internals/index.rst). In particular this will
exercise it from multiple threads.
Dave
