Re: [PATCH][gcc] libgccjit: introduce version entry points

[David Malcolm <dmalcolm@redhat.com>]

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