public inbox for jit@gcc.gnu.org
 help / color / mirror / Atom feed
* Re: [PATCH][gcc] libgccjit: introduce version entry points
  2020-01-01  0:00 ` David Malcolm
  2020-01-01  0:00   ` Florian Weimer
  2020-01-01  0:00   ` David Malcolm
@ 2020-01-01  0:00   ` Andrea Corallo
  2 siblings, 0 replies; 16+ messages in thread
From: Andrea Corallo @ 2020-01-01  0:00 UTC (permalink / raw)
  To: David Malcolm; +Cc: gcc-patches, jit, nd

David Malcolm <dmalcolm@redhat.com> writes:

> 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?

Hi Dave,

The use case is where client code wants to check specifically at
run-time for the version.  This to warn for a known to be buggy version
or to take any other decision that depends on the libgccjit version.
One could decide to layout the generated code differently depending on
the compiler version.

For these cases the granularity we have with with macros defining for
the ABI may be not sufficient.

As you say this is speculative now given that will become helpful only
in the future.

Thanks for reviewing both patches.  I'll re-spin them this weekend.

  Andrea

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH][gcc] libgccjit: introduce version entry points
@ 2020-01-01  0:00 Andrea Corallo
  2020-01-01  0:00 ` David Malcolm
  2020-03-18 22:51 ` [PATCH V3][gcc] " Andrea Corallo
  0 siblings, 2 replies; 16+ messages in thread
From: Andrea Corallo @ 2020-01-01  0:00 UTC (permalink / raw)
  To: gcc-patches, jit; +Cc: dmalcolm, nd

[-- Attachment #1: Type: text/plain, Size: 820 bytes --]

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.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: jit-version2.patch --]
[-- Type: text/x-diff; name="jit-version2.patch", Size: 5397 bytes --]

diff --git a/gcc/jit/docs/topics/compatibility.rst b/gcc/jit/docs/topics/compatibility.rst
index a6faee0810e..0c0ce070d72 100644
--- a/gcc/jit/docs/topics/compatibility.rst
+++ b/gcc/jit/docs/topics/compatibility.rst
@@ -61,6 +61,28 @@ You can see the symbol tags provided by libgccjit.so using ``objdump``:
            LIBGCCJIT_ABI_0
    [...snip...]
 
+Programmatically checking version
+***************
+
+Client code can programmatically check libgccjit version using:
+
+.. function::  int gcc_jit_version_major (void)
+
+   Return libgccjit major version.  This is analogous to __GNUC__ in C code.
+
+.. function::  int gcc_jit_version_minor (void)
+
+   Return libgccjit minor version.  This is analogous to
+   __GNUC_MINOR__ in C code.
+
+.. function::  int gcc_jit_version_patchlevel (void)
+
+   Return libgccjit patchlevel version.  This is analogous to
+   __GNUC_PATCHLEVEL__ in C code.
+
+.. note:: These entry points has been added with ``LIBGCCJIT_ABI_13``
+          (see below).
+
 ABI symbol tags
 ***************
 
@@ -182,3 +204,14 @@ entrypoints:
 --------------------
 ``LIBGCCJIT_ABI_12`` covers the addition of
 :func:`gcc_jit_context_new_bitfield`
+
+``LIBGCCJIT_ABI_13``
+--------------------
+``LIBGCCJIT_ABI_13`` covers the addition of version functions via API
+entrypoints:
+
+  * :func:`gcc_jit_version_major`
+
+  * :func:`gcc_jit_version_minor`
+
+  * :func:`gcc_jit_version_patchlevel`
diff --git a/gcc/jit/libgccjit++.h b/gcc/jit/libgccjit++.h
index 82a62d614c5..afb92194c28 100644
--- a/gcc/jit/libgccjit++.h
+++ b/gcc/jit/libgccjit++.h
@@ -49,6 +49,8 @@ namespace gccjit
   class timer;
   class auto_time;
 
+  namespace version {};
+
   /* Errors within the API become C++ exceptions of this class.  */
   class error
   {
@@ -1913,6 +1915,26 @@ auto_time::~auto_time ()
   m_timer.pop (m_item_name);
 }
 
+namespace version
+{
+inline int
+major ()
+{
+  return gcc_jit_version_major ();
+}
+
+inline int
+minor ()
+{
+  return gcc_jit_version_minor ();
+}
+
+inline int
+patchlevel ()
+{
+  return gcc_jit_version_patchlevel ();
+}
+} // namespace version
 } // namespace gccjit
 
 #endif /* #ifndef LIBGCCJIT_PLUS_PLUS_H */
diff --git a/gcc/jit/libgccjit.h b/gcc/jit/libgccjit.h
index 21a0dc09b03..1c5a12e9c01 100644
--- a/gcc/jit/libgccjit.h
+++ b/gcc/jit/libgccjit.h
@@ -1487,6 +1487,22 @@ gcc_jit_context_new_rvalue_from_vector (gcc_jit_context *ctxt,
 					size_t num_elements,
 					gcc_jit_rvalue **elements);
 
+#define LIBGCCJIT_HAVE_gcc_jit_version
+
+/* Functions to retrive libgccjit version.
+   Analogous to __GNUC__, __GNUC_MINOR__, __GNUC_PATCHLEVEL__ in C code.
+
+   These API entrypoints were added in LIBGCCJIT_ABI_13; you can test for their
+   presence using
+     #ifdef LIBGCCJIT_HAVE_gcc_jit_version
+ */
+extern int
+gcc_jit_version_major (void);
+extern int
+gcc_jit_version_minor (void);
+extern int
+gcc_jit_version_patchlevel (void);
+
 #ifdef __cplusplus
 }
 #endif /* __cplusplus */
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;
+}
diff --git a/gcc/jit/libgccjit.map b/gcc/jit/libgccjit.map
index 4514bd3aa33..6137dd4b4b0 100644
--- a/gcc/jit/libgccjit.map
+++ b/gcc/jit/libgccjit.map
@@ -179,4 +179,11 @@ LIBGCCJIT_ABI_11 {
 LIBGCCJIT_ABI_12 {
   global:
     gcc_jit_context_new_bitfield;
-} LIBGCCJIT_ABI_11;
\ No newline at end of file
+} LIBGCCJIT_ABI_11;
+
+LIBGCCJIT_ABI_13 {
+  global:
+    gcc_jit_version_major;
+    gcc_jit_version_minor;
+    gcc_jit_version_patchlevel;
+} LIBGCCJIT_ABI_12;
\ No newline at end of file
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 ();
+}

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH][gcc] libgccjit: introduce version entry points
  2020-01-01  0:00 [PATCH][gcc] libgccjit: introduce version entry points Andrea Corallo
@ 2020-01-01  0:00 ` David Malcolm
  2020-01-01  0:00   ` Florian Weimer
                     ` (2 more replies)
  2020-03-18 22:51 ` [PATCH V3][gcc] " Andrea Corallo
  1 sibling, 3 replies; 16+ messages in thread
From: David Malcolm @ 2020-01-01  0:00 UTC (permalink / raw)
  To: Andrea Corallo, gcc-patches, jit; +Cc: nd

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH][gcc] libgccjit: introduce version entry points
  2020-01-01  0:00 ` David Malcolm
  2020-01-01  0:00   ` Florian Weimer
@ 2020-01-01  0:00   ` David Malcolm
  2020-03-08 14:08     ` Andrea Corallo
  2020-01-01  0:00   ` Andrea Corallo
  2 siblings, 1 reply; 16+ messages in thread
From: David Malcolm @ 2020-01-01  0:00 UTC (permalink / raw)
  To: Andrea Corallo, gcc-patches, jit; +Cc: nd

On Thu, 2020-03-05 at 21:34 -0500, David Malcolm wrote:
> On Thu, 2020-01-16 at 11:11 +0000, Andrea Corallo wrote:

Responding to my own ideas about thread-safety.

[...]

> 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)

In retrospect, I don't think this other approach would work: the state
is within parse_basever, so if two threads both determine they need to
access it at about the same time, then they will race.

> 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)

I'd hoped that we could rely on static init being thread-safe, but in
general it isn't, according to:
https://eli.thegreenplace.net/2011/08/30/construction-of-function-static-variables-in-c-is-not-thread-safe
(apparently GCC 4 onwards makes it so, but other compilers don't)


From what I can tell parse_basever is only called once for the regular
compiler use-case.  So maybe it makes sense to remove the caching from
it, and move the caching to libgccjit.c where we can have a mutex
(AFAIK none of the rest of the host code uses mutexes).

Or split out the actual parsing logic into a parse_basever_uncached
that libgccjit.c can use, and manage its own caching with a pthread
mutex like in my suggested version_info code above.

Thoughts?
Dave

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH][gcc] libgccjit: introduce version entry points
  2020-01-01  0:00 ` David Malcolm
@ 2020-01-01  0:00   ` Florian Weimer
  2020-01-01  0:00   ` David Malcolm
  2020-01-01  0:00   ` Andrea Corallo
  2 siblings, 0 replies; 16+ messages in thread
From: Florian Weimer @ 2020-01-01  0:00 UTC (permalink / raw)
  To: David Malcolm; +Cc: Andrea Corallo, gcc-patches, jit, nd

* David Malcolm:

>
> 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:

Right, it's not thread-safe.

One possiblity would be to store all three numbers in one unsigned
int, and used relaxed MO loads and stores.  A zero value would
indicate that initialization is needed.

It will break if there are ever more than 1000 or so GCC releases, but
it will be good for a while.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH][gcc] libgccjit: introduce version entry points
  2020-01-01  0:00   ` David Malcolm
@ 2020-03-08 14:08     ` Andrea Corallo
  0 siblings, 0 replies; 16+ messages in thread
From: Andrea Corallo @ 2020-03-08 14:08 UTC (permalink / raw)
  To: David Malcolm; +Cc: gcc-patches, jit, nd

Hi,

thanks for reviewing, I've totally missed this multi-thread aspect.

David Malcolm <dmalcolm@redhat.com> writes:

> On Thu, 2020-03-05 at 21:34 -0500, David Malcolm wrote:
>> On Thu, 2020-01-16 at 11:11 +0000, Andrea Corallo wrote:
>
> Responding to my own ideas about thread-safety.
>
> [...]
>
>> 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)
>
> In retrospect, I don't think this other approach would work: the state
> is within parse_basever, so if two threads both determine they need to
> access it at about the same time, then they will race.

Agree

>> 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)
>
> I'd hoped that we could rely on static init being thread-safe, but in
> general it isn't, according to:
> https://eli.thegreenplace.net/2011/08/30/construction-of-function-static-variables-in-c-is-not-thread-safe
> (apparently GCC 4 onwards makes it so, but other compilers don't)
>
>
> From what I can tell parse_basever is only called once for the regular
> compiler use-case.  So maybe it makes sense to remove the caching from
> it, and move the caching to libgccjit.c where we can have a mutex
> (AFAIK none of the rest of the host code uses mutexes).
>
> Or split out the actual parsing logic into a parse_basever_uncached
> that libgccjit.c can use, and manage its own caching with a pthread
> mutex like in my suggested version_info code above.
>
> Thoughts?

What would be the advantage in splitting the cached and uncached
versions at this stage?  If we accept that parse_basever is not thread
safe we can just protect it with the mutex, we'll have to use one anyway
also if we keep the cache in the jit code.

Coming back on your question I assume client code will call very rarely
this function (especially if we compare against the compile times
involved) so I don't think we can have a perf issue here.

My vote is to go for the mutex solution you've first suggested.

  Andrea

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH V3][gcc] libgccjit: introduce version entry points
  2020-01-01  0:00 [PATCH][gcc] libgccjit: introduce version entry points Andrea Corallo
  2020-01-01  0:00 ` David Malcolm
@ 2020-03-18 22:51 ` Andrea Corallo
  2020-03-21  1:32   ` David Malcolm
  1 sibling, 1 reply; 16+ messages in thread
From: Andrea Corallo @ 2020-03-18 22:51 UTC (permalink / raw)
  To: gcc-patches; +Cc: jit, dmalcolm, nd

[-- Attachment #1: Type: text/plain, Size: 1159 bytes --]

Hi all,

Updated version of the patch mainly addressing comments on the
concurrency issues.

I came to the conclusions that the caching should be done in the
function that we decide to be thread safe.  However I haven't touched
parse_basever in any direction in the hope of having this still in
stage4.  As result I've mostly applied the mutex solution.

'make check-jit' runs clean

Bests

  Andrea

gcc/jit/ChangeLog
2020-??-??  Andrea Corallo  <andrea.corallo@arm.com>
	    David Malcolm  <dmalcolm@redhat.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.
	* jit.dg/all-non-failing-tests.h: Add test-version.c.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Add-new-version-entry-point.patch --]
[-- Type: text/x-diff, Size: 7002 bytes --]

From ad86baf8472c6684aed9b62652922a83e147952a Mon Sep 17 00:00:00 2001
From: AndreaCorallo <andrea.corallo@arm.com>
Date: Sun, 8 Mar 2020 13:46:33 +0000
Subject: [PATCH] Add new version entry point

---
 gcc/jit/docs/topics/compatibility.rst        | 33 ++++++++++++++
 gcc/jit/libgccjit++.h                        | 22 ++++++++++
 gcc/jit/libgccjit.c                          | 46 ++++++++++++++++++++
 gcc/jit/libgccjit.h                          | 16 +++++++
 gcc/jit/libgccjit.map                        |  9 +++-
 gcc/testsuite/jit.dg/all-non-failing-tests.h |  7 +++
 gcc/testsuite/jit.dg/test-version.c          | 26 +++++++++++
 7 files changed, 158 insertions(+), 1 deletion(-)
 create mode 100644 gcc/testsuite/jit.dg/test-version.c

diff --git a/gcc/jit/docs/topics/compatibility.rst b/gcc/jit/docs/topics/compatibility.rst
index a6faee0810e..0c0ce070d72 100644
--- a/gcc/jit/docs/topics/compatibility.rst
+++ b/gcc/jit/docs/topics/compatibility.rst
@@ -61,6 +61,28 @@ You can see the symbol tags provided by libgccjit.so using ``objdump``:
            LIBGCCJIT_ABI_0
    [...snip...]
 
+Programmatically checking version
+***************
+
+Client code can programmatically check libgccjit version using:
+
+.. function::  int gcc_jit_version_major (void)
+
+   Return libgccjit major version.  This is analogous to __GNUC__ in C code.
+
+.. function::  int gcc_jit_version_minor (void)
+
+   Return libgccjit minor version.  This is analogous to
+   __GNUC_MINOR__ in C code.
+
+.. function::  int gcc_jit_version_patchlevel (void)
+
+   Return libgccjit patchlevel version.  This is analogous to
+   __GNUC_PATCHLEVEL__ in C code.
+
+.. note:: These entry points has been added with ``LIBGCCJIT_ABI_13``
+          (see below).
+
 ABI symbol tags
 ***************
 
@@ -182,3 +204,14 @@ entrypoints:
 --------------------
 ``LIBGCCJIT_ABI_12`` covers the addition of
 :func:`gcc_jit_context_new_bitfield`
+
+``LIBGCCJIT_ABI_13``
+--------------------
+``LIBGCCJIT_ABI_13`` covers the addition of version functions via API
+entrypoints:
+
+  * :func:`gcc_jit_version_major`
+
+  * :func:`gcc_jit_version_minor`
+
+  * :func:`gcc_jit_version_patchlevel`
diff --git a/gcc/jit/libgccjit++.h b/gcc/jit/libgccjit++.h
index 82a62d614c5..69e67766640 100644
--- a/gcc/jit/libgccjit++.h
+++ b/gcc/jit/libgccjit++.h
@@ -49,6 +49,8 @@ namespace gccjit
   class timer;
   class auto_time;
 
+  namespace version {};
+
   /* Errors within the API become C++ exceptions of this class.  */
   class error
   {
@@ -1913,6 +1915,26 @@ auto_time::~auto_time ()
   m_timer.pop (m_item_name);
 }
 
+namespace version
+{
+inline int
+major_v ()
+{
+  return gcc_jit_version_major ();
+}
+
+inline int
+minor_v ()
+{
+  return gcc_jit_version_minor ();
+}
+
+inline int
+patchlevel_v ()
+{
+  return gcc_jit_version_patchlevel ();
+}
+} // namespace version
 } // namespace gccjit
 
 #endif /* #ifndef LIBGCCJIT_PLUS_PLUS_H */
diff --git a/gcc/jit/libgccjit.h b/gcc/jit/libgccjit.h
index 21a0dc09b03..1c5a12e9c01 100644
--- a/gcc/jit/libgccjit.h
+++ b/gcc/jit/libgccjit.h
@@ -1487,6 +1487,22 @@ gcc_jit_context_new_rvalue_from_vector (gcc_jit_context *ctxt,
 					size_t num_elements,
 					gcc_jit_rvalue **elements);
 
+#define LIBGCCJIT_HAVE_gcc_jit_version
+
+/* Functions to retrive libgccjit version.
+   Analogous to __GNUC__, __GNUC_MINOR__, __GNUC_PATCHLEVEL__ in C code.
+
+   These API entrypoints were added in LIBGCCJIT_ABI_13; you can test for their
+   presence using
+     #ifdef LIBGCCJIT_HAVE_gcc_jit_version
+ */
+extern int
+gcc_jit_version_major (void);
+extern int
+gcc_jit_version_minor (void);
+extern int
+gcc_jit_version_patchlevel (void);
+
 #ifdef __cplusplus
 }
 #endif /* __cplusplus */
diff --git a/gcc/jit/libgccjit.c b/gcc/jit/libgccjit.c
index 83055fc297b..a29e9885e59 100644
--- a/gcc/jit/libgccjit.c
+++ b/gcc/jit/libgccjit.c
@@ -23,6 +23,8 @@ 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 <pthread.h>
 
 #include "libgccjit.h"
 #include "jit-recording.h"
@@ -3175,3 +3177,47 @@ gcc_jit_context_new_rvalue_from_vector (gcc_jit_context *ctxt,
      as_vec_type,
      (gcc::jit::recording::rvalue **)elements);
 }
+
+/* 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;
+};
+
+
+extern int
+gcc_jit_version_major (void)
+{
+  version_info vi;
+  return vi.major;
+}
+
+extern int
+gcc_jit_version_minor (void)
+{
+  version_info vi;
+  return vi.minor;
+}
+
+extern int
+gcc_jit_version_patchlevel (void)
+{
+  version_info vi;
+  return vi.patchlevel;
+}
diff --git a/gcc/jit/libgccjit.map b/gcc/jit/libgccjit.map
index 4514bd3aa33..6137dd4b4b0 100644
--- a/gcc/jit/libgccjit.map
+++ b/gcc/jit/libgccjit.map
@@ -179,4 +179,11 @@ LIBGCCJIT_ABI_11 {
 LIBGCCJIT_ABI_12 {
   global:
     gcc_jit_context_new_bitfield;
-} LIBGCCJIT_ABI_11;
\ No newline at end of file
+} LIBGCCJIT_ABI_11;
+
+LIBGCCJIT_ABI_13 {
+  global:
+    gcc_jit_version_major;
+    gcc_jit_version_minor;
+    gcc_jit_version_patchlevel;
+} LIBGCCJIT_ABI_12;
\ No newline at end of file
diff --git a/gcc/testsuite/jit.dg/all-non-failing-tests.h b/gcc/testsuite/jit.dg/all-non-failing-tests.h
index 0272e6f846f..cba4ac51cc9 100644
--- a/gcc/testsuite/jit.dg/all-non-failing-tests.h
+++ b/gcc/testsuite/jit.dg/all-non-failing-tests.h
@@ -8,6 +8,13 @@
    hooks provided by each test case.  */
 #define COMBINED_TEST
 
+/* test-version.c */
+#define create_code create_code_version
+#define verify_code verify_code_version
+#include "test-version.c"
+#undef create_code
+#undef verify_code
+
 /* test-accessing-bitfield.c */
 #define create_code create_code_accessing_bitfield
 #define verify_code verify_code_accessing_bitfield
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 ();
+}
-- 
2.17.1


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH V3][gcc] libgccjit: introduce version entry points
  2020-03-18 22:51 ` [PATCH V3][gcc] " Andrea Corallo
@ 2020-03-21  1:32   ` David Malcolm
  2020-03-23 13:03     ` Richard Biener
  2020-03-29 20:31     ` Andrea Corallo
  0 siblings, 2 replies; 16+ messages in thread
From: David Malcolm @ 2020-03-21  1:32 UTC (permalink / raw)
  To: Andrea Corallo, gcc-patches; +Cc: jit, nd, jakub, Richard Biener

On Wed, 2020-03-18 at 23:51 +0100, Andrea Corallo wrote:
> Hi all,
> 
> Updated version of the patch mainly addressing comments on the
> concurrency issues.
> 
> I came to the conclusions that the caching should be done in the
> function that we decide to be thread safe.  However I haven't touched
> parse_basever in any direction in the hope of having this still in
> stage4.  As result I've mostly applied the mutex solution.
> 
> 'make check-jit' runs clean
> 
> Bests
> 
>   Andrea
> 
> gcc/jit/ChangeLog
> 2020-??-??  Andrea Corallo  <andrea.corallo@arm.com>
> 	    David Malcolm  <dmalcolm@redhat.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.
> 	* jit.dg/all-non-failing-tests.h: Add test-version.c.

> diff --git a/gcc/testsuite/jit.dg/all-non-failing-tests.h b/gcc/testsuite/jit.dg/all-non-failing-tests.h
> index 0272e6f846f..cba4ac51cc9 100644
> --- a/gcc/testsuite/jit.dg/all-non-failing-tests.h
> +++ b/gcc/testsuite/jit.dg/all-non-failing-tests.h
> @@ -8,6 +8,13 @@
>     hooks provided by each test case.  */
>  #define COMBINED_TEST
>  
> +/* test-version.c */
> +#define create_code create_code_version
> +#define verify_code verify_code_version
> +#include "test-version.c"
> +#undef create_code
> +#undef verify_code
> +
>  /* test-accessing-bitfield.c */
>  #define create_code create_code_accessing_bitfield
>  #define verify_code verify_code_accessing_bitfield

Please add the new test to the header in its alphabetical location,
i.e. between:

  /* test-vector-types.cc: We don't use this, since it's C++.  */

and

  /* test-volatile.c */

An entry also needs to be added to the "testcases" array at the end of
the header (again, in the alphabetical-sorted location).

OK by me with that change (assuming you re-test the patch), but given
we're in stage 4, it's probably worth double-checking with a release
manager.  I've CCed Jakub and Richi.

Dave


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH V3][gcc] libgccjit: introduce version entry points
  2020-03-21  1:32   ` David Malcolm
@ 2020-03-23 13:03     ` Richard Biener
  2020-03-29 20:31     ` Andrea Corallo
  1 sibling, 0 replies; 16+ messages in thread
From: Richard Biener @ 2020-03-23 13:03 UTC (permalink / raw)
  To: David Malcolm; +Cc: Andrea Corallo, gcc-patches, jit, nd, jakub

On Fri, 20 Mar 2020, David Malcolm wrote:

> On Wed, 2020-03-18 at 23:51 +0100, Andrea Corallo wrote:
> > Hi all,
> > 
> > Updated version of the patch mainly addressing comments on the
> > concurrency issues.
> > 
> > I came to the conclusions that the caching should be done in the
> > function that we decide to be thread safe.  However I haven't touched
> > parse_basever in any direction in the hope of having this still in
> > stage4.  As result I've mostly applied the mutex solution.
> > 
> > 'make check-jit' runs clean
> > 
> > Bests
> > 
> >   Andrea
> > 
> > gcc/jit/ChangeLog
> > 2020-??-??  Andrea Corallo  <andrea.corallo@arm.com>
> > 	    David Malcolm  <dmalcolm@redhat.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.
> > 	* jit.dg/all-non-failing-tests.h: Add test-version.c.
> 
> > diff --git a/gcc/testsuite/jit.dg/all-non-failing-tests.h b/gcc/testsuite/jit.dg/all-non-failing-tests.h
> > index 0272e6f846f..cba4ac51cc9 100644
> > --- a/gcc/testsuite/jit.dg/all-non-failing-tests.h
> > +++ b/gcc/testsuite/jit.dg/all-non-failing-tests.h
> > @@ -8,6 +8,13 @@
> >     hooks provided by each test case.  */
> >  #define COMBINED_TEST
> >  
> > +/* test-version.c */
> > +#define create_code create_code_version
> > +#define verify_code verify_code_version
> > +#include "test-version.c"
> > +#undef create_code
> > +#undef verify_code
> > +
> >  /* test-accessing-bitfield.c */
> >  #define create_code create_code_accessing_bitfield
> >  #define verify_code verify_code_accessing_bitfield
> 
> Please add the new test to the header in its alphabetical location,
> i.e. between:
> 
>   /* test-vector-types.cc: We don't use this, since it's C++.  */
> 
> and
> 
>   /* test-volatile.c */
> 
> An entry also needs to be added to the "testcases" array at the end of
> the header (again, in the alphabetical-sorted location).
> 
> OK by me with that change (assuming you re-test the patch), but given
> we're in stage 4, it's probably worth double-checking with a release
> manager.  I've CCed Jakub and Richi.

I don't really care.

Richard.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH V3][gcc] libgccjit: introduce version entry points
  2020-03-21  1:32   ` David Malcolm
  2020-03-23 13:03     ` Richard Biener
@ 2020-03-29 20:31     ` Andrea Corallo
  2020-03-30 16:09       ` David Malcolm
  2020-03-31 12:05       ` [PATCH V4][gcc] " Andrea Corallo
  1 sibling, 2 replies; 16+ messages in thread
From: Andrea Corallo @ 2020-03-29 20:31 UTC (permalink / raw)
  To: David Malcolm; +Cc: gcc-patches, jit, nd

David Malcolm <dmalcolm@redhat.com> writes:

> On Wed, 2020-03-18 at 23:51 +0100, Andrea Corallo wrote:
> Please add the new test to the header in its alphabetical location,
> i.e. between:
>
>   /* test-vector-types.cc: We don't use this, since it's C++.  */
>
> and
>
>   /* test-volatile.c */
>
> An entry also needs to be added to the "testcases" array at the end of
> the header (again, in the alphabetical-sorted location).

I tried adding the test into the "testcases" array but this makes the
threads test failing.

I think this has nothing to do with this patch and happen just because
this test does not define any code.  Infact I see the same happening
just adding "test-empty.c" to the "testcases" array on the current
master.

The error is not very reproducible, I tried a run under valgrind but
have found nothing so far :/

Dave do you recall if there was a specific reason not to have
"test-empty.c" into the "testcases" array?

  Andrea

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH V3][gcc] libgccjit: introduce version entry points
  2020-03-29 20:31     ` Andrea Corallo
@ 2020-03-30 16:09       ` David Malcolm
  2020-03-31  1:13         ` David Malcolm
  2020-03-31 12:05       ` [PATCH V4][gcc] " Andrea Corallo
  1 sibling, 1 reply; 16+ messages in thread
From: David Malcolm @ 2020-03-30 16:09 UTC (permalink / raw)
  To: Andrea Corallo; +Cc: gcc-patches, jit, nd

On Sun, 2020-03-29 at 21:31 +0100, Andrea Corallo wrote:
> David Malcolm <dmalcolm@redhat.com> writes:
> 
> > On Wed, 2020-03-18 at 23:51 +0100, Andrea Corallo wrote:
> > Please add the new test to the header in its alphabetical location,
> > i.e. between:
> > 
> >   /* test-vector-types.cc: We don't use this, since it's C++.  */
> > 
> > and
> > 
> >   /* test-volatile.c */
> > 
> > An entry also needs to be added to the "testcases" array at the end
> > of
> > the header (again, in the alphabetical-sorted location).
> 
> I tried adding the test into the "testcases" array but this makes the
> threads test failing.
> 
> I think this has nothing to do with this patch and happen just
> because
> this test does not define any code.  Infact I see the same happening
> just adding "test-empty.c" to the "testcases" array on the current
> master.
> 
> The error is not very reproducible, I tried a run under valgrind but
> have found nothing so far :/
> 
> Dave do you recall if there was a specific reason not to have
> "test-empty.c" into the "testcases" array?
> 
>   Andrea

I replied to this as:

  [PATCH] lra: set insn_code_data to NULL when freeing
  https://gcc.gnu.org/pipermail/gcc-patches/2020-March/542929.html

but forgot to set the reply-to in git send-email; sorry.

Dave


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH V3][gcc] libgccjit: introduce version entry points
  2020-03-30 16:09       ` David Malcolm
@ 2020-03-31  1:13         ` David Malcolm
  2020-03-31  8:03           ` Andrea Corallo
  0 siblings, 1 reply; 16+ messages in thread
From: David Malcolm @ 2020-03-31  1:13 UTC (permalink / raw)
  To: Andrea Corallo; +Cc: jit, nd, gcc-patches

On Mon, 2020-03-30 at 12:09 -0400, David Malcolm via Gcc-patches wrote:
> On Sun, 2020-03-29 at 21:31 +0100, Andrea Corallo wrote:
> > David Malcolm <dmalcolm@redhat.com> writes:
> > 
> > > On Wed, 2020-03-18 at 23:51 +0100, Andrea Corallo wrote:
> > > Please add the new test to the header in its alphabetical
> > > location,
> > > i.e. between:
> > > 
> > >   /* test-vector-types.cc: We don't use this, since it's C++.  */
> > > 
> > > and
> > > 
> > >   /* test-volatile.c */
> > > 
> > > An entry also needs to be added to the "testcases" array at the
> > > end
> > > of
> > > the header (again, in the alphabetical-sorted location).
> > 
> > I tried adding the test into the "testcases" array but this makes
> > the
> > threads test failing.
> > 
> > I think this has nothing to do with this patch and happen just
> > because
> > this test does not define any code.  Infact I see the same
> > happening
> > just adding "test-empty.c" to the "testcases" array on the current
> > master.
> > 
> > The error is not very reproducible, I tried a run under valgrind
> > but
> > have found nothing so far :/
> > 
> > Dave do you recall if there was a specific reason not to have
> > "test-empty.c" into the "testcases" array?
> > 
> >   Andrea
> 
> I replied to this as:
> 
>   [PATCH] lra: set insn_code_data to NULL when freeing
>   https://gcc.gnu.org/pipermail/gcc-patches/2020-March/542929.html
> 
> but forgot to set the reply-to in git send-email; sorry.
> 

Andrea: I've pushed my proposed fix for the above to master as
3809bcd6c0ee324cbd855c68cee104c8bf134dbe.  Does this fix the issue you
were seeing?

Thanks
Dave


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH V3][gcc] libgccjit: introduce version entry points
  2020-03-31  1:13         ` David Malcolm
@ 2020-03-31  8:03           ` Andrea Corallo
  0 siblings, 0 replies; 16+ messages in thread
From: Andrea Corallo @ 2020-03-31  8:03 UTC (permalink / raw)
  To: David Malcolm; +Cc: jit, nd, gcc-patches

David Malcolm <dmalcolm@redhat.com> writes:

> Andrea: I've pushed my proposed fix for the above to master as
> 3809bcd6c0ee324cbd855c68cee104c8bf134dbe.  Does this fix the issue you
> were seeing?
>
> Thanks
> Dave

Hi,

yes super! I'll update the patch thanks.

  Andrea

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH V4][gcc] libgccjit: introduce version entry points
  2020-03-29 20:31     ` Andrea Corallo
  2020-03-30 16:09       ` David Malcolm
@ 2020-03-31 12:05       ` Andrea Corallo
  2020-03-31 17:33         ` David Malcolm
  1 sibling, 1 reply; 16+ messages in thread
From: Andrea Corallo @ 2020-03-31 12:05 UTC (permalink / raw)
  To: David Malcolm; +Cc: jit, nd, gcc-patches

[-- Attachment #1: Type: text/plain, Size: 874 bytes --]

Hi all,

Updated version of the patch addressing last comments.

Regression clean, okay to apply?

Bests

  Andrea

gcc/jit/ChangeLog
2020-??-??  Andrea Corallo  <andrea.corallo@arm.com>
	    David Malcolm  <dmalcolm@redhat.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.
	* jit.dg/all-non-failing-tests.h: Add test-version.c.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Add-new-version-entry-point.patch --]
[-- Type: text/x-diff, Size: 7253 bytes --]

From 181082c1dfc02521f3f1b44c058af26b0884614a Mon Sep 17 00:00:00 2001
From: AndreaCorallo <andrea.corallo@arm.com>
Date: Sun, 8 Mar 2020 13:46:33 +0000
Subject: [PATCH] Add new version entry point

---
 gcc/jit/docs/topics/compatibility.rst        | 33 ++++++++++++++
 gcc/jit/libgccjit++.h                        | 22 ++++++++++
 gcc/jit/libgccjit.c                          | 46 ++++++++++++++++++++
 gcc/jit/libgccjit.h                          | 16 +++++++
 gcc/jit/libgccjit.map                        |  9 +++-
 gcc/testsuite/jit.dg/all-non-failing-tests.h | 10 +++++
 gcc/testsuite/jit.dg/test-version.c          | 26 +++++++++++
 7 files changed, 161 insertions(+), 1 deletion(-)
 create mode 100644 gcc/testsuite/jit.dg/test-version.c

diff --git a/gcc/jit/docs/topics/compatibility.rst b/gcc/jit/docs/topics/compatibility.rst
index a6faee0810e..0c0ce070d72 100644
--- a/gcc/jit/docs/topics/compatibility.rst
+++ b/gcc/jit/docs/topics/compatibility.rst
@@ -61,6 +61,28 @@ You can see the symbol tags provided by libgccjit.so using ``objdump``:
            LIBGCCJIT_ABI_0
    [...snip...]
 
+Programmatically checking version
+***************
+
+Client code can programmatically check libgccjit version using:
+
+.. function::  int gcc_jit_version_major (void)
+
+   Return libgccjit major version.  This is analogous to __GNUC__ in C code.
+
+.. function::  int gcc_jit_version_minor (void)
+
+   Return libgccjit minor version.  This is analogous to
+   __GNUC_MINOR__ in C code.
+
+.. function::  int gcc_jit_version_patchlevel (void)
+
+   Return libgccjit patchlevel version.  This is analogous to
+   __GNUC_PATCHLEVEL__ in C code.
+
+.. note:: These entry points has been added with ``LIBGCCJIT_ABI_13``
+          (see below).
+
 ABI symbol tags
 ***************
 
@@ -182,3 +204,14 @@ entrypoints:
 --------------------
 ``LIBGCCJIT_ABI_12`` covers the addition of
 :func:`gcc_jit_context_new_bitfield`
+
+``LIBGCCJIT_ABI_13``
+--------------------
+``LIBGCCJIT_ABI_13`` covers the addition of version functions via API
+entrypoints:
+
+  * :func:`gcc_jit_version_major`
+
+  * :func:`gcc_jit_version_minor`
+
+  * :func:`gcc_jit_version_patchlevel`
diff --git a/gcc/jit/libgccjit++.h b/gcc/jit/libgccjit++.h
index 82a62d614c5..69e67766640 100644
--- a/gcc/jit/libgccjit++.h
+++ b/gcc/jit/libgccjit++.h
@@ -49,6 +49,8 @@ namespace gccjit
   class timer;
   class auto_time;
 
+  namespace version {};
+
   /* Errors within the API become C++ exceptions of this class.  */
   class error
   {
@@ -1913,6 +1915,26 @@ auto_time::~auto_time ()
   m_timer.pop (m_item_name);
 }
 
+namespace version
+{
+inline int
+major_v ()
+{
+  return gcc_jit_version_major ();
+}
+
+inline int
+minor_v ()
+{
+  return gcc_jit_version_minor ();
+}
+
+inline int
+patchlevel_v ()
+{
+  return gcc_jit_version_patchlevel ();
+}
+} // namespace version
 } // namespace gccjit
 
 #endif /* #ifndef LIBGCCJIT_PLUS_PLUS_H */
diff --git a/gcc/jit/libgccjit.h b/gcc/jit/libgccjit.h
index 21a0dc09b03..1c5a12e9c01 100644
--- a/gcc/jit/libgccjit.h
+++ b/gcc/jit/libgccjit.h
@@ -1487,6 +1487,22 @@ gcc_jit_context_new_rvalue_from_vector (gcc_jit_context *ctxt,
 					size_t num_elements,
 					gcc_jit_rvalue **elements);
 
+#define LIBGCCJIT_HAVE_gcc_jit_version
+
+/* Functions to retrive libgccjit version.
+   Analogous to __GNUC__, __GNUC_MINOR__, __GNUC_PATCHLEVEL__ in C code.
+
+   These API entrypoints were added in LIBGCCJIT_ABI_13; you can test for their
+   presence using
+     #ifdef LIBGCCJIT_HAVE_gcc_jit_version
+ */
+extern int
+gcc_jit_version_major (void);
+extern int
+gcc_jit_version_minor (void);
+extern int
+gcc_jit_version_patchlevel (void);
+
 #ifdef __cplusplus
 }
 #endif /* __cplusplus */
diff --git a/gcc/jit/libgccjit.c b/gcc/jit/libgccjit.c
index 83055fc297b..a29e9885e59 100644
--- a/gcc/jit/libgccjit.c
+++ b/gcc/jit/libgccjit.c
@@ -23,6 +23,8 @@ 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 <pthread.h>
 
 #include "libgccjit.h"
 #include "jit-recording.h"
@@ -3175,3 +3177,47 @@ gcc_jit_context_new_rvalue_from_vector (gcc_jit_context *ctxt,
      as_vec_type,
      (gcc::jit::recording::rvalue **)elements);
 }
+
+/* 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;
+};
+
+
+extern int
+gcc_jit_version_major (void)
+{
+  version_info vi;
+  return vi.major;
+}
+
+extern int
+gcc_jit_version_minor (void)
+{
+  version_info vi;
+  return vi.minor;
+}
+
+extern int
+gcc_jit_version_patchlevel (void)
+{
+  version_info vi;
+  return vi.patchlevel;
+}
diff --git a/gcc/jit/libgccjit.map b/gcc/jit/libgccjit.map
index 4514bd3aa33..6137dd4b4b0 100644
--- a/gcc/jit/libgccjit.map
+++ b/gcc/jit/libgccjit.map
@@ -179,4 +179,11 @@ LIBGCCJIT_ABI_11 {
 LIBGCCJIT_ABI_12 {
   global:
     gcc_jit_context_new_bitfield;
-} LIBGCCJIT_ABI_11;
\ No newline at end of file
+} LIBGCCJIT_ABI_11;
+
+LIBGCCJIT_ABI_13 {
+  global:
+    gcc_jit_version_major;
+    gcc_jit_version_minor;
+    gcc_jit_version_patchlevel;
+} LIBGCCJIT_ABI_12;
\ No newline at end of file
diff --git a/gcc/testsuite/jit.dg/all-non-failing-tests.h b/gcc/testsuite/jit.dg/all-non-failing-tests.h
index 0272e6f846f..9fb056fef7f 100644
--- a/gcc/testsuite/jit.dg/all-non-failing-tests.h
+++ b/gcc/testsuite/jit.dg/all-non-failing-tests.h
@@ -254,6 +254,13 @@
 
 /* test-vector-types.cc: We don't use this, since it's C++.  */
 
+/* test-version.c */
+#define create_code create_code_version
+#define verify_code verify_code_version
+#include "test-version.c"
+#undef create_code
+#undef verify_code
+
 /* test-volatile.c */
 #define create_code create_code_volatile
 #define verify_code verify_code_volatile
@@ -372,6 +379,9 @@ const struct testcase testcases[] = {
   {"using_global",
    create_code_using_global,
    verify_code_using_global},
+  {"version",
+   create_code_version,
+   verify_code_version},
   {"volatile",
    create_code_volatile,
    verify_code_volatile}
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 ();
+}
-- 
2.17.1


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH V4][gcc] libgccjit: introduce version entry points
  2020-03-31 12:05       ` [PATCH V4][gcc] " Andrea Corallo
@ 2020-03-31 17:33         ` David Malcolm
  2020-03-31 19:00           ` Andrea Corallo
  0 siblings, 1 reply; 16+ messages in thread
From: David Malcolm @ 2020-03-31 17:33 UTC (permalink / raw)
  To: Andrea Corallo; +Cc: jit, nd, gcc-patches

On Tue, 2020-03-31 at 14:05 +0200, Andrea Corallo wrote:
> Hi all,
> 
> Updated version of the patch addressing last comments.

Thanks.

> Regression clean, okay to apply?

OK

> Bests
> 
>   Andrea
> 
> gcc/jit/ChangeLog
> 2020-??-??  Andrea Corallo  <andrea.corallo@arm.com>
> 	    David Malcolm  <dmalcolm@redhat.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.
> 	* jit.dg/all-non-failing-tests.h: Add test-version.c.


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH V4][gcc] libgccjit: introduce version entry points
  2020-03-31 17:33         ` David Malcolm
@ 2020-03-31 19:00           ` Andrea Corallo
  0 siblings, 0 replies; 16+ messages in thread
From: Andrea Corallo @ 2020-03-31 19:00 UTC (permalink / raw)
  To: David Malcolm; +Cc: jit, nd, gcc-patches

David Malcolm <dmalcolm@redhat.com> writes:

> On Tue, 2020-03-31 at 14:05 +0200, Andrea Corallo wrote:
>> Hi all,
>> 
>> Updated version of the patch addressing last comments.
>
> Thanks.
>
>> Regression clean, okay to apply?
>
> OK

Committed as 63b2923dc6f5.

Thanks

  Andrea

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2020-03-31 19:00 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-01  0:00 [PATCH][gcc] libgccjit: introduce version entry points Andrea Corallo
2020-01-01  0:00 ` David Malcolm
2020-01-01  0:00   ` Florian Weimer
2020-01-01  0:00   ` David Malcolm
2020-03-08 14:08     ` Andrea Corallo
2020-01-01  0:00   ` Andrea Corallo
2020-03-18 22:51 ` [PATCH V3][gcc] " Andrea Corallo
2020-03-21  1:32   ` David Malcolm
2020-03-23 13:03     ` Richard Biener
2020-03-29 20:31     ` Andrea Corallo
2020-03-30 16:09       ` David Malcolm
2020-03-31  1:13         ` David Malcolm
2020-03-31  8:03           ` Andrea Corallo
2020-03-31 12:05       ` [PATCH V4][gcc] " Andrea Corallo
2020-03-31 17:33         ` David Malcolm
2020-03-31 19:00           ` Andrea Corallo

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).