[PATCH] libgccjit: Add support for TLS variable [PR95415]

[PATCH] libgccjit: Add support for TLS variable [PR95415]

[Antoni Boucher]

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

Hello.
This patch adds support for TLS variables.
One thing to fix before we merge it is the libgccjit.map file which
contains LIBGCCJIT_ABI_16 instead of LIBGCCJIT_ABI_17.
LIBGCCJIT_ABI_16 was added in one of my other patches.
Thanks for the review.

[-- Attachment #2: 0001-Add-support-for-TLS-variable-PR95415.patch --]
[-- Type: text/x-patch, Size: 11789 bytes --]

From 6092e3d347972d331ed9ac6cae153168e98ecd0d Mon Sep 17 00:00:00 2001
From: Antoni Boucher <bouanto@zoho.com>
Date: Tue, 11 May 2021 19:23:54 -0400
Subject: [PATCH] Add support for TLS variable [PR95415]

2021-05-18  Antoni Boucher  <bouanto@zoho.com>

    gcc/jit/
            PR target/95415
            * docs/topics/compatibility.rst (LIBGCCJIT_ABI_17): New ABI
            tag.
            * docs/topics/expressions.rst: Add document for the function
            gcc_jit_lvalue_set_tls_model.
            * jit-playback.h: New function (set_tls_model) and make
            rvalue::m_inner public.
            * jit-recording.c: New function (set_tls_model), new
            variables (tls_models and tls_model_enum_strings) and support
            for setting the tls model.
            * jit-recording.h: New function (set_tls_model) and new
            field m_tls_model.
            * libgccjit.c: New function (gcc_jit_lvalue_set_tls_model).
            * libgccjit.h: New function (gcc_jit_lvalue_set_tls_model)
            and new enum (gcc_jit_tls_model).
            * libgccjit.map (LIBGCCJIT_ABI_17): New ABI tag.

    gcc/testsuite/
            PR target/95415
            * jit.dg/all-non-failing-tests.h: Add test-tls.c.
            * jit.dg/test-tls.c: New test.
---
 gcc/jit/docs/topics/compatibility.rst        |  9 +++++
 gcc/jit/docs/topics/expressions.rst          | 28 +++++++++++++
 gcc/jit/jit-playback.h                       |  8 ++++
 gcc/jit/jit-recording.c                      | 41 ++++++++++++++++++--
 gcc/jit/jit-recording.h                      |  7 +++-
 gcc/jit/libgccjit.c                          | 18 +++++++++
 gcc/jit/libgccjit.h                          | 21 ++++++++++
 gcc/jit/libgccjit.map                        |  5 +++
 gcc/testsuite/jit.dg/all-non-failing-tests.h |  7 ++++
 gcc/testsuite/jit.dg/test-tls.c              | 29 ++++++++++++++
 10 files changed, 169 insertions(+), 4 deletions(-)
 create mode 100644 gcc/testsuite/jit.dg/test-tls.c

diff --git a/gcc/jit/docs/topics/compatibility.rst b/gcc/jit/docs/topics/compatibility.rst
index 239b6aa1a92..d10bc1df080 100644
--- a/gcc/jit/docs/topics/compatibility.rst
+++ b/gcc/jit/docs/topics/compatibility.rst
@@ -243,3 +243,12 @@ embedding assembler instructions:
   * :func:`gcc_jit_extended_asm_add_input_operand`
   * :func:`gcc_jit_extended_asm_add_clobber`
   * :func:`gcc_jit_context_add_top_level_asm`
+
+.. _LIBGCCJIT_ABI_17:
+
+``LIBGCCJIT_ABI_17``
+-----------------------
+``LIBGCCJIT_ABI_17`` covers the addition of an API entrypoint to set the
+thread-local storage model of a variable:
+
+  * :func:`gcc_jit_lvalue_set_tls_model`
diff --git a/gcc/jit/docs/topics/expressions.rst b/gcc/jit/docs/topics/expressions.rst
index 396259ef07e..68defd6a311 100644
--- a/gcc/jit/docs/topics/expressions.rst
+++ b/gcc/jit/docs/topics/expressions.rst
@@ -539,6 +539,34 @@ where the rvalue is computed by reading from the storage area.
 
    in C.
 
+.. function:: void\
+              gcc_jit_lvalue_set_tls_model (gcc_jit_lvalue *lvalue,\
+                                            enum gcc_jit_tls_model model)
+
+   Make a variable a thread-local variable.
+
+   The "model" parameter determines the thread-local storage model of the "lvalue":
+
+   .. type:: enum gcc_jit_tls_model
+
+   .. c:macro:: GCC_JIT_TLS_MODEL_GLOBAL_DYNAMIC
+
+   .. c:macro:: GCC_JIT_TLS_MODEL_LOCAL_DYNAMIC
+
+   .. c:macro:: GCC_JIT_TLS_MODEL_INITIAL_EXEC
+
+   .. c:macro:: GCC_JIT_TLS_MODEL_LOCAL_EXEC
+
+   .. c:macro:: GCC_JIT_TLS_MODEL_DEFAULT
+
+   This is analogous to:
+
+   .. code-block:: c
+
+     _Thread_local int foo;
+
+   in C.
+
 Global variables
 ****************
 
diff --git a/gcc/jit/jit-playback.h b/gcc/jit/jit-playback.h
index 825a3e172e9..654a9c472d4 100644
--- a/gcc/jit/jit-playback.h
+++ b/gcc/jit/jit-playback.h
@@ -650,6 +650,8 @@ public:
 
 private:
   context *m_ctxt;
+
+protected:
   tree m_inner;
 };
 
@@ -670,6 +672,12 @@ public:
   rvalue *
   get_address (location *loc);
 
+  void
+  set_tls_model (enum tls_model tls_model)
+  {
+    set_decl_tls_model (m_inner, tls_model);
+  }
+
 private:
   bool mark_addressable (location *loc);
 };
diff --git a/gcc/jit/jit-recording.c b/gcc/jit/jit-recording.c
index 117ff70114c..64f3ae2d8f9 100644
--- a/gcc/jit/jit-recording.c
+++ b/gcc/jit/jit-recording.c
@@ -3713,6 +3713,12 @@ recording::lvalue::get_address (recording::location *loc)
   return result;
 }
 
+void
+recording::lvalue::set_tls_model (enum gcc_jit_tls_model model)
+{
+    m_tls_model = model;
+}
+
 /* The implementation of class gcc::jit::recording::param.  */
 
 /* Implementation of pure virtual hook recording::memento::replay_into
@@ -4539,6 +4545,15 @@ recording::block::dump_edges_to_dot (pretty_printer *pp)
 #  pragma GCC diagnostic pop
 #endif
 
+namespace recording {
+static const enum tls_model tls_models[] = {
+  TLS_MODEL_GLOBAL_DYNAMIC, /* GCC_JIT_TLS_MODEL_GLOBAL_DYNAMIC */
+  TLS_MODEL_LOCAL_DYNAMIC, /* GCC_JIT_TLS_MODEL_LOCAL_DYNAMIC */
+  TLS_MODEL_INITIAL_EXEC, /* GCC_JIT_TLS_MODEL_INITIAL_EXEC */
+  TLS_MODEL_LOCAL_EXEC, /* GCC_JIT_TLS_MODEL_LOCAL_EXEC */
+};
+} /* namespace recording */
+
 /* The implementation of class gcc::jit::recording::global.  */
 
 /* Implementation of pure virtual hook recording::memento::replay_into
@@ -4547,8 +4562,7 @@ recording::block::dump_edges_to_dot (pretty_printer *pp)
 void
 recording::global::replay_into (replayer *r)
 {
-  set_playback_obj (
-    m_initializer
+    playback::lvalue *global = m_initializer
     ? r->new_global_initialized (playback_location (r, m_loc),
 				 m_kind,
 				 m_type->playback_type (),
@@ -4560,7 +4574,12 @@ recording::global::replay_into (replayer *r)
     : r->new_global (playback_location (r, m_loc),
 		     m_kind,
 		     m_type->playback_type (),
-		     playback_string (m_name)));
+		     playback_string (m_name));
+  if (m_tls_model != GCC_JIT_TLS_MODEL_DEFAULT)
+  {
+      global->set_tls_model (recording::tls_models[m_tls_model]);
+  }
+  set_playback_obj (global);
 }
 
 /* Override the default implementation of
@@ -4658,6 +4677,14 @@ recording::global::write_initializer_reproducer (const char *id, reproducer &r)
 
 /* Implementation of recording::memento::write_reproducer for globals. */
 
+static const char * const tls_model_enum_strings[] = {
+  "GCC_JIT_TLS_MODEL_GLOBAL_DYNAMIC",
+  "GCC_JIT_TLS_MODEL_LOCAL_DYNAMIC",
+  "GCC_JIT_TLS_MODEL_INITIAL_EXEC",
+  "GCC_JIT_TLS_MODEL_LOCAL_EXEC",
+  "GCC_JIT_TLS_MODEL_DEFAULT",
+};
+
 void
 recording::global::write_reproducer (reproducer &r)
 {
@@ -4675,6 +4702,14 @@ recording::global::write_reproducer (reproducer &r)
     r.get_identifier_as_type (get_type ()),
     m_name->get_debug_string ());
 
+  if (m_tls_model)
+  {
+	r.write ("  gcc_jit_lvalue_set_tls_model (%s, /* gcc_jit_lvalue *lvalue */\n"
+	     "                                %s); /* enum gcc_jit_tls_model model */\n",
+    id,
+    tls_model_enum_strings[m_tls_model]);
+  }
+
   if (m_initializer)
     switch (m_type->dereference ()->get_size ())
       {
diff --git a/gcc/jit/jit-recording.h b/gcc/jit/jit-recording.h
index 03fa1160cf0..893bf72dd31 100644
--- a/gcc/jit/jit-recording.h
+++ b/gcc/jit/jit-recording.h
@@ -1105,7 +1105,8 @@ public:
   lvalue (context *ctxt,
 	  location *loc,
 	  type *type_)
-    : rvalue (ctxt, loc, type_)
+    : rvalue (ctxt, loc, type_),
+    m_tls_model (GCC_JIT_TLS_MODEL_DEFAULT)
     {}
 
   playback::lvalue *
@@ -1127,6 +1128,10 @@ public:
   const char *access_as_rvalue (reproducer &r) OVERRIDE;
   virtual const char *access_as_lvalue (reproducer &r);
   virtual bool is_global () const { return false; }
+  void set_tls_model (enum gcc_jit_tls_model model);
+
+protected:
+  enum gcc_jit_tls_model m_tls_model;
 };
 
 class param : public lvalue
diff --git a/gcc/jit/libgccjit.c b/gcc/jit/libgccjit.c
index 0cc650f9810..768b99499cf 100644
--- a/gcc/jit/libgccjit.c
+++ b/gcc/jit/libgccjit.c
@@ -1953,6 +1953,24 @@ gcc_jit_lvalue_get_address (gcc_jit_lvalue *lvalue,
   return (gcc_jit_rvalue *)lvalue->get_address (loc);
 }
 
+/* Public entrypoint.  See description in libgccjit.h.
+
+   After error-checking, the real work is done by the
+   gcc::jit::recording::lvalue::set_tls_model method in jit-recording.c.  */
+
+void
+gcc_jit_lvalue_set_tls_model (gcc_jit_lvalue *lvalue,
+			    enum gcc_jit_tls_model model)
+{
+  RETURN_IF_FAIL (lvalue, NULL, NULL, "NULL lvalue");
+  JIT_LOG_FUNC (lvalue->get_context ()->get_logger ());
+  RETURN_IF_FAIL_PRINTF1 (lvalue->is_global (), NULL, NULL,
+			       "lvalue \"%s\" not a global",
+			       lvalue->get_debug_string ());
+
+  lvalue->set_tls_model (model);
+}
+
 /* Public entrypoint.  See description in libgccjit.h.
 
    After error-checking, the real work is done by the
diff --git a/gcc/jit/libgccjit.h b/gcc/jit/libgccjit.h
index 5c722c2c57f..2a52b351a49 100644
--- a/gcc/jit/libgccjit.h
+++ b/gcc/jit/libgccjit.h
@@ -722,6 +722,16 @@ enum gcc_jit_function_kind
   GCC_JIT_FUNCTION_ALWAYS_INLINE
 };
 
+/* Thread local storage model.  */
+enum gcc_jit_tls_model
+{
+  GCC_JIT_TLS_MODEL_GLOBAL_DYNAMIC,
+  GCC_JIT_TLS_MODEL_LOCAL_DYNAMIC,
+  GCC_JIT_TLS_MODEL_INITIAL_EXEC,
+  GCC_JIT_TLS_MODEL_LOCAL_EXEC,
+  GCC_JIT_TLS_MODEL_DEFAULT,
+};
+
 /* Create a function.  */
 extern gcc_jit_function *
 gcc_jit_context_new_function (gcc_jit_context *ctxt,
@@ -1072,6 +1082,17 @@ extern gcc_jit_rvalue *
 gcc_jit_lvalue_get_address (gcc_jit_lvalue *lvalue,
 			    gcc_jit_location *loc);
 
+#define LIBGCCJIT_HAVE_gcc_jit_lvalue_set_tls_model
+
+/* Set the thread-local storage model of a global variable
+
+   This API entrypoint was added in LIBGCCJIT_ABI_17; you can test for its
+   presence using
+     #ifdef LIBGCCJIT_HAVE_gcc_jit_lvalue_set_tls_model  */
+extern void
+gcc_jit_lvalue_set_tls_model (gcc_jit_lvalue *lvalue,
+			    enum gcc_jit_tls_model model);
+
 extern gcc_jit_lvalue *
 gcc_jit_function_new_local (gcc_jit_function *func,
 			    gcc_jit_location *loc,
diff --git a/gcc/jit/libgccjit.map b/gcc/jit/libgccjit.map
index 337ea6c7fe4..605c624ec4a 100644
--- a/gcc/jit/libgccjit.map
+++ b/gcc/jit/libgccjit.map
@@ -205,3 +205,8 @@ LIBGCCJIT_ABI_15 {
     gcc_jit_extended_asm_add_clobber;
     gcc_jit_context_add_top_level_asm;
 } LIBGCCJIT_ABI_14;
+
+LIBGCCJIT_ABI_16 {
+  global:
+    gcc_jit_lvalue_set_tls_model;
+} LIBGCCJIT_ABI_15;
diff --git a/gcc/testsuite/jit.dg/all-non-failing-tests.h b/gcc/testsuite/jit.dg/all-non-failing-tests.h
index 4202eb7798b..c2d87a30cca 100644
--- a/gcc/testsuite/jit.dg/all-non-failing-tests.h
+++ b/gcc/testsuite/jit.dg/all-non-failing-tests.h
@@ -181,6 +181,13 @@
 #undef create_code
 #undef verify_code
 
+/* test-tls.c */
+#define create_code create_code_tls
+#define verify_code verify_code_tls
+#include "test-tls.c"
+#undef create_code
+#undef verify_code
+
 /* test-hello-world.c */
 #define create_code create_code_hello_world
 #define verify_code verify_code_hello_world
diff --git a/gcc/testsuite/jit.dg/test-tls.c b/gcc/testsuite/jit.dg/test-tls.c
new file mode 100644
index 00000000000..d4508b16c1e
--- /dev/null
+++ b/gcc/testsuite/jit.dg/test-tls.c
@@ -0,0 +1,29 @@
+#include <stdlib.h>
+#include <stdio.h>
+#include <string.h>
+#include <time.h>
+
+#include "libgccjit.h"
+
+#include "harness.h"
+
+void
+create_code (gcc_jit_context *ctxt, void *user_data)
+{
+  /* Let's try to inject the equivalent of:
+
+     _Thread_local int foo;
+  */
+  gcc_jit_type *int_type =
+    gcc_jit_context_get_type (ctxt, GCC_JIT_TYPE_INT);
+
+  gcc_jit_lvalue *foo =
+    gcc_jit_context_new_global (
+      ctxt, NULL, GCC_JIT_GLOBAL_EXPORTED, int_type, "foo");
+  gcc_jit_lvalue_set_tls_model (foo, GCC_JIT_TLS_MODEL_GLOBAL_DYNAMIC);
+}
+
+void
+verify_code (gcc_jit_context *ctxt, gcc_jit_result *result)
+{
+}
-- 
2.31.1

Re: [PATCH] libgccjit: Add support for TLS variable [PR95415]

[David Malcolm]

On Tue, 2021-05-18 at 20:43 -0400, Antoni Boucher via Gcc-patches
wrote:
> Hello.
> This patch adds support for TLS variables.
> One thing to fix before we merge it is the libgccjit.map file which
> contains LIBGCCJIT_ABI_16 instead of LIBGCCJIT_ABI_17.
> LIBGCCJIT_ABI_16 was added in one of my other patches.
> Thanks for the review.

> diff --git a/gcc/jit/docs/topics/compatibility.rst b/gcc/jit/docs/topics/compatibility.rst
> index 239b6aa1a92..d10bc1df080 100644
> --- a/gcc/jit/docs/topics/compatibility.rst
> +++ b/gcc/jit/docs/topics/compatibility.rst
> @@ -243,3 +243,12 @@ embedding assembler instructions:
>    * :func:`gcc_jit_extended_asm_add_input_operand`
>    * :func:`gcc_jit_extended_asm_add_clobber`
>    * :func:`gcc_jit_context_add_top_level_asm`
> +
> +.. _LIBGCCJIT_ABI_17:
> +
> +``LIBGCCJIT_ABI_17``
> +-----------------------
> +``LIBGCCJIT_ABI_17`` covers the addition of an API entrypoint to set the
> +thread-local storage model of a variable:
> +
> +  * :func:`gcc_jit_lvalue_set_tls_model`

Sorry about the delay in reviewing patches.

Is there a summary somewhere of the various outstanding patches and
their associated ABI versions?  Are there dependencies between the
patches?

> diff --git a/gcc/jit/docs/topics/expressions.rst
b/gcc/jit/docs/topics/expressions.rst
> index 396259ef07e..68defd6a311 100644
> --- a/gcc/jit/docs/topics/expressions.rst
> +++ b/gcc/jit/docs/topics/expressions.rst
> @@ -539,6 +539,34 @@ where the rvalue is computed by reading from the storage area.
>  
>     in C.
>  
> +.. function:: void\
> +              gcc_jit_lvalue_set_tls_model (gcc_jit_lvalue *lvalue,\
> +                                            enum gcc_jit_tls_model model)
> +
> +   Make a variable a thread-local variable.
> +
> +   The "model" parameter determines the thread-local storage model of the "lvalue":
> +
> +   .. type:: enum gcc_jit_tls_model
> +
> +   .. c:macro:: GCC_JIT_TLS_MODEL_GLOBAL_DYNAMIC
> +
> +   .. c:macro:: GCC_JIT_TLS_MODEL_LOCAL_DYNAMIC
> +
> +   .. c:macro:: GCC_JIT_TLS_MODEL_INITIAL_EXEC
> +
> +   .. c:macro:: GCC_JIT_TLS_MODEL_LOCAL_EXEC
> +
> +   .. c:macro:: GCC_JIT_TLS_MODEL_DEFAULT
> +
> +   This is analogous to:
> +
> +   .. code-block:: c
> +
> +     _Thread_local int foo;
> +
> +   in C.

This comment needs the usual "This entrypoint was added in" text to
state which API version it was added in.

I confess to being a bit hazy on the different TLS models, and it's
unclear to me what all the different enum values do.  Is this
equivalent to the various values for __attribute__((tls_model(VALUE)))
?  This attribute is documented in
https://gcc.gnu.org/onlinedocs/gcc/Common-Variable-Attributes.html,
though sadly that document doesn't seem to have a good anchor for that
attribute.

https://gcc.gnu.org/onlinedocs/gcc/Thread-Local.html currently links to
https://www.akkadia.org/drepper/tls.pdf "for a detailed explanation of
the four thread-local storage addressing models, and how the runtime is
expected to function."

One thing that should be clarified: does GCC_JIT_TLS_MODEL_DEFAULT mean
(a) thread-local storage, using a default model, or
(b) non-thread-local storage i.e. normal storage.

?

Reading the docs I thought it meant (a), but when I looked in more
detail at the implementation it looks like it means (b); is it meant
to?  This needs clarifying.

Are you using all of these enum values in your code?  Is this something
you need to expose for the rustc backend?


>  Global variables
>  ****************
>  
> diff --git a/gcc/jit/jit-playback.h b/gcc/jit/jit-playback.h
> index 825a3e172e9..654a9c472d4 100644
> --- a/gcc/jit/jit-playback.h
> +++ b/gcc/jit/jit-playback.h
> @@ -650,6 +650,8 @@ public:
>  
>  private:
>    context *m_ctxt;
> +
> +protected:
>    tree m_inner;
>  };

As noted in another review, I don't think you need to make this
protected...

>  
> @@ -670,6 +672,12 @@ public:
>    rvalue *
>    get_address (location *loc);
>  
> +  void
> +  set_tls_model (enum tls_model tls_model)
> +  {
> +    set_decl_tls_model (m_inner, tls_model);
> +  }

...as I think you can use "as_tree ()" to get at m_inner here.


> diff --git a/gcc/jit/jit-recording.c b/gcc/jit/jit-recording.c
> index 117ff70114c..64f3ae2d8f9 100644
> --- a/gcc/jit/jit-recording.c
> +++ b/gcc/jit/jit-recording.c
> @@ -3713,6 +3713,12 @@ recording::lvalue::get_address (recording::location *loc)
>    return result;
>  }
>  
> +void
> +recording::lvalue::set_tls_model (enum gcc_jit_tls_model model)
> +{
> +    m_tls_model = model;
> +}
> +
>  /* The implementation of class gcc::jit::recording::param.  */
>  
>  /* Implementation of pure virtual hook recording::memento::replay_into
> @@ -4539,6 +4545,15 @@ recording::block::dump_edges_to_dot (pretty_printer *pp)
>  #  pragma GCC diagnostic pop
>  #endif
>  
> +namespace recording {
> +static const enum tls_model tls_models[] = {
> +  TLS_MODEL_GLOBAL_DYNAMIC, /* GCC_JIT_TLS_MODEL_GLOBAL_DYNAMIC */
> +  TLS_MODEL_LOCAL_DYNAMIC, /* GCC_JIT_TLS_MODEL_LOCAL_DYNAMIC */
> +  TLS_MODEL_INITIAL_EXEC, /* GCC_JIT_TLS_MODEL_INITIAL_EXEC */
> +  TLS_MODEL_LOCAL_EXEC, /* GCC_JIT_TLS_MODEL_LOCAL_EXEC */
> +};
> +} /* namespace recording */
> +
>  /* The implementation of class gcc::jit::recording::global.  */
>  
>  /* Implementation of pure virtual hook recording::memento::replay_into
> @@ -4547,8 +4562,7 @@ recording::block::dump_edges_to_dot (pretty_printer *pp)
>  void
>  recording::global::replay_into (replayer *r)
>  {
> -  set_playback_obj (
> -    m_initializer
> +    playback::lvalue *global = m_initializer
>      ? r->new_global_initialized (playback_location (r, m_loc),
>  				 m_kind,
>  				 m_type->playback_type (),
> @@ -4560,7 +4574,12 @@ recording::global::replay_into (replayer *r)
>      : r->new_global (playback_location (r, m_loc),
>  		     m_kind,
>  		     m_type->playback_type (),
> -		     playback_string (m_name)));
> +		     playback_string (m_name));
> +  if (m_tls_model != GCC_JIT_TLS_MODEL_DEFAULT)
> +  {
> +      global->set_tls_model (recording::tls_models[m_tls_model]);
> +  }
> +  set_playback_obj (global);
>  }
>  
>  /* Override the default implementation of

[...snip...]

> @@ -4675,6 +4702,14 @@ recording::global::write_reproducer (reproducer &r)
>      r.get_identifier_as_type (get_type ()),
>      m_name->get_debug_string ());
>  
> +  if (m_tls_model)
> +  {

I think this conditional should be:

     if (m_tls_model != GCC_JIT_TLS_MODEL_DEFAULT)

since the default value isn't 0.  (Maybe it should be?)

> +	r.write ("  gcc_jit_lvalue_set_tls_model (%s, /*
gcc_jit_lvalue *lvalue */\n"
> +	     "                                %s); /* enum gcc_jit_tls_model model */\n",
> +    id,
> +    tls_model_enum_strings[m_tls_model]);
> +  }
> +
>    if (m_initializer)
>      switch (m_type->dereference ()->get_size ())
>        {

[...snip...]

>  class param : public lvalue
> diff --git a/gcc/jit/libgccjit.c b/gcc/jit/libgccjit.c
> index 0cc650f9810..768b99499cf 100644
> --- a/gcc/jit/libgccjit.c
> +++ b/gcc/jit/libgccjit.c
> @@ -1953,6 +1953,24 @@ gcc_jit_lvalue_get_address (gcc_jit_lvalue *lvalue,
>    return (gcc_jit_rvalue *)lvalue->get_address (loc);
>  }
>  
> +/* Public entrypoint.  See description in libgccjit.h.
> +
> +   After error-checking, the real work is done by the
> +   gcc::jit::recording::lvalue::set_tls_model method in jit-recording.c.  */
> +
> +void
> +gcc_jit_lvalue_set_tls_model (gcc_jit_lvalue *lvalue,
> +			    enum gcc_jit_tls_model model)
> +{
> +  RETURN_IF_FAIL (lvalue, NULL, NULL, "NULL lvalue");
> +  JIT_LOG_FUNC (lvalue->get_context ()->get_logger ());
> +  RETURN_IF_FAIL_PRINTF1 (lvalue->is_global (), NULL, NULL,
> +			       "lvalue \"%s\" not a global",
> +			       lvalue->get_debug_string ());

This should pass lvalue's context to the RETURN_IF_FAIL_PRINTF1, so
that if it fails, the error is associated with the context.


> +  lvalue->set_tls_model (model);
> +}
> +
>  /* Public entrypoint.  See description in libgccjit.h.
>  
>     After error-checking, the real work is done by the
> diff --git a/gcc/jit/libgccjit.h b/gcc/jit/libgccjit.h
> index 5c722c2c57f..2a52b351a49 100644
> --- a/gcc/jit/libgccjit.h
> +++ b/gcc/jit/libgccjit.h
> @@ -722,6 +722,16 @@ enum gcc_jit_function_kind
>    GCC_JIT_FUNCTION_ALWAYS_INLINE
>  };
>  
> +/* Thread local storage model.  */
> +enum gcc_jit_tls_model
> +{
> +  GCC_JIT_TLS_MODEL_GLOBAL_DYNAMIC,
> +  GCC_JIT_TLS_MODEL_LOCAL_DYNAMIC,
> +  GCC_JIT_TLS_MODEL_INITIAL_EXEC,
> +  GCC_JIT_TLS_MODEL_LOCAL_EXEC,
> +  GCC_JIT_TLS_MODEL_DEFAULT,

As noted above, should the DEFAULT one be the first?

If DEFAULT means "not thread-local", maybe "NONE" would be clearer than
"DEFAULT"?

> +};
> +
>  /* Create a function.  */
>  extern gcc_jit_function *
>  gcc_jit_context_new_function (gcc_jit_context *ctxt,
> @@ -1072,6 +1082,17 @@ extern gcc_jit_rvalue *
>  gcc_jit_lvalue_get_address (gcc_jit_lvalue *lvalue,
>  			    gcc_jit_location *loc);
>  
> +#define LIBGCCJIT_HAVE_gcc_jit_lvalue_set_tls_model
> +
> +/* Set the thread-local storage model of a global variable
> +
> +   This API entrypoint was added in LIBGCCJIT_ABI_17; you can test for its
> +   presence using
> +     #ifdef LIBGCCJIT_HAVE_gcc_jit_lvalue_set_tls_model  */
> +extern void
> +gcc_jit_lvalue_set_tls_model (gcc_jit_lvalue *lvalue,
> +			    enum gcc_jit_tls_model model);
> +
>  extern gcc_jit_lvalue *
>  gcc_jit_function_new_local (gcc_jit_function *func,
>  			    gcc_jit_location *loc,
> diff --git a/gcc/jit/libgccjit.map b/gcc/jit/libgccjit.map
> index 337ea6c7fe4..605c624ec4a 100644
> --- a/gcc/jit/libgccjit.map
> +++ b/gcc/jit/libgccjit.map
> @@ -205,3 +205,8 @@ LIBGCCJIT_ABI_15 {
>      gcc_jit_extended_asm_add_clobber;
>      gcc_jit_context_add_top_level_asm;
>  } LIBGCCJIT_ABI_14;
> +
> +LIBGCCJIT_ABI_16 {
> +  global:
> +    gcc_jit_lvalue_set_tls_model;
> +} LIBGCCJIT_ABI_15;

Obviously the ABI needs to be fixed up here.

> diff --git a/gcc/testsuite/jit.dg/all-non-failing-tests.h b/gcc/testsuite/jit.dg/all-non-failing-tests.h
> index 4202eb7798b..c2d87a30cca 100644
> --- a/gcc/testsuite/jit.dg/all-non-failing-tests.h
> +++ b/gcc/testsuite/jit.dg/all-non-failing-tests.h
> @@ -181,6 +181,13 @@
>  #undef create_code
>  #undef verify_code
>  
> +/* test-tls.c */
> +#define create_code create_code_tls
> +#define verify_code verify_code_tls
> +#include "test-tls.c"
> +#undef create_code
> +#undef verify_code
> +
>  /* test-hello-world.c */
>  #define create_code create_code_hello_world
>  #define verify_code verify_code_hello_world

This is missing an entry in the "testcases" array at the bottom of the
header to make use of the new {create|verify}_code_tls functions.

> diff --git a/gcc/testsuite/jit.dg/test-tls.c b/gcc/testsuite/jit.dg/test-tls.c
> new file mode 100644
> index 00000000000..d4508b16c1e
> --- /dev/null
> +++ b/gcc/testsuite/jit.dg/test-tls.c
> @@ -0,0 +1,29 @@
> +#include <stdlib.h>
> +#include <stdio.h>
> +#include <string.h>
> +#include <time.h>
> +
> +#include "libgccjit.h"
> +
> +#include "harness.h"
> +
> +void
> +create_code (gcc_jit_context *ctxt, void *user_data)
> +{
> +  /* Let's try to inject the equivalent of:
> +
> +     _Thread_local int foo;
> +  */
> +  gcc_jit_type *int_type =
> +    gcc_jit_context_get_type (ctxt, GCC_JIT_TYPE_INT);
> +
> +  gcc_jit_lvalue *foo =
> +    gcc_jit_context_new_global (
> +      ctxt, NULL, GCC_JIT_GLOBAL_EXPORTED, int_type, "foo");
> +  gcc_jit_lvalue_set_tls_model (foo, GCC_JIT_TLS_MODEL_GLOBAL_DYNAMIC);

How many of the different enum values can be supported?  How target-
dependent is this?

> +}
> +
> +void
> +verify_code (gcc_jit_context *ctxt, gcc_jit_result *result)
> +{

Should probably at least try to read and write the global(s).

Hope this is constructive
Dave

Re: [PATCH] libgccjit: Add support for TLS variable [PR95415]

[Antoni Boucher]

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

Hi.
Here's the updated patch.
See comments below.
Thanks for your reviews!

Le jeudi 20 mai 2021 à 16:11 -0400, David Malcolm a écrit :
> On Tue, 2021-05-18 at 20:43 -0400, Antoni Boucher via Gcc-patches
> wrote:
> > Hello.
> > This patch adds support for TLS variables.
> > One thing to fix before we merge it is the libgccjit.map file which
> > contains LIBGCCJIT_ABI_16 instead of LIBGCCJIT_ABI_17.
> > LIBGCCJIT_ABI_16 was added in one of my other patches.
> > Thanks for the review.
> 
> > diff --git a/gcc/jit/docs/topics/compatibility.rst
> > b/gcc/jit/docs/topics/compatibility.rst
> > index 239b6aa1a92..d10bc1df080 100644
> > --- a/gcc/jit/docs/topics/compatibility.rst
> > +++ b/gcc/jit/docs/topics/compatibility.rst
> > @@ -243,3 +243,12 @@ embedding assembler instructions:
> >    * :func:`gcc_jit_extended_asm_add_input_operand`
> >    * :func:`gcc_jit_extended_asm_add_clobber`
> >    * :func:`gcc_jit_context_add_top_level_asm`
> > +
> > +.. _LIBGCCJIT_ABI_17:
> > +
> > +``LIBGCCJIT_ABI_17``
> > +-----------------------
> > +``LIBGCCJIT_ABI_17`` covers the addition of an API entrypoint to set
> > the
> > +thread-local storage model of a variable:
> > +
> > +  * :func:`gcc_jit_lvalue_set_tls_model`
> 
> Sorry about the delay in reviewing patches.
> 
> Is there a summary somewhere of the various outstanding patches and
> their associated ABI versions?  Are there dependencies between the
> patches?

The list of patches is there:
https://github.com/antoyo/libgccjit-patches but I don't keep them up-
to-date.
If that would help you, I could add a README to tell what is the new
ABI version for each patch.
I believe there might be some patches that depend on a previous one.

> 
> > diff --git a/gcc/jit/docs/topics/expressions.rst
> b/gcc/jit/docs/topics/expressions.rst
> > index 396259ef07e..68defd6a311 100644
> > --- a/gcc/jit/docs/topics/expressions.rst
> > +++ b/gcc/jit/docs/topics/expressions.rst
> > @@ -539,6 +539,34 @@ where the rvalue is computed by reading from
> > the storage area.
> >  
> >     in C.
> >  
> > +.. function:: void\
> > +              gcc_jit_lvalue_set_tls_model (gcc_jit_lvalue
> > *lvalue,\
> > +                                            enum gcc_jit_tls_model
> > model)
> > +
> > +   Make a variable a thread-local variable.
> > +
> > +   The "model" parameter determines the thread-local storage model
> > of the "lvalue":
> > +
> > +   .. type:: enum gcc_jit_tls_model
> > +
> > +   .. c:macro:: GCC_JIT_TLS_MODEL_GLOBAL_DYNAMIC
> > +
> > +   .. c:macro:: GCC_JIT_TLS_MODEL_LOCAL_DYNAMIC
> > +
> > +   .. c:macro:: GCC_JIT_TLS_MODEL_INITIAL_EXEC
> > +
> > +   .. c:macro:: GCC_JIT_TLS_MODEL_LOCAL_EXEC
> > +
> > +   .. c:macro:: GCC_JIT_TLS_MODEL_DEFAULT
> > +
> > +   This is analogous to:
> > +
> > +   .. code-block:: c
> > +
> > +     _Thread_local int foo;
> > +
> > +   in C.
> 
> This comment needs the usual "This entrypoint was added in" text to
> state which API version it was added in.
> 
> I confess to being a bit hazy on the different TLS models, and it's
> unclear to me what all the different enum values do.  Is this
> equivalent to the various values for
> __attribute__((tls_model(VALUE)))
> ?  This attribute is documented in
> https://gcc.gnu.org/onlinedocs/gcc/Common-Variable-Attributes.html,
> though sadly that document doesn't seem to have a good anchor for
> that
> attribute.

Yes, it is the equivalent of this attribute.

> 
> https://gcc.gnu.org/onlinedocs/gcc/Thread-Local.html currently links
> to
> https://www.akkadia.org/drepper/tls.pdf "for a detailed explanation
> of
> the four thread-local storage addressing models, and how the runtime
> is
> expected to function."
> 
> One thing that should be clarified: does GCC_JIT_TLS_MODEL_DEFAULT
> mean
> (a) thread-local storage, using a default model, or
> (b) non-thread-local storage i.e. normal storage.
> 
> ?
> 
> Reading the docs I thought it meant (a), but when I looked in more
> detail at the implementation it looks like it means (b); is it meant
> to?  This needs clarifying.
> 
> Are you using all of these enum values in your code?  Is this
> something
> you need to expose for the rustc backend?

Yes, I use all of these enum values in the rustc gcc codegen.

> 
> 
> >  Global variables
> >  ****************
> >  
> > diff --git a/gcc/jit/jit-playback.h b/gcc/jit/jit-playback.h
> > index 825a3e172e9..654a9c472d4 100644
> > --- a/gcc/jit/jit-playback.h
> > +++ b/gcc/jit/jit-playback.h
> > @@ -650,6 +650,8 @@ public:
> >  
> >  private:
> >    context *m_ctxt;
> > +
> > +protected:
> >    tree m_inner;
> >  };
> 
> As noted in another review, I don't think you need to make this
> protected...
> 
> >  
> > @@ -670,6 +672,12 @@ public:
> >    rvalue *
> >    get_address (location *loc);
> >  
> > +  void
> > +  set_tls_model (enum tls_model tls_model)
> > +  {
> > +    set_decl_tls_model (m_inner, tls_model);
> > +  }
> 
> ...as I think you can use "as_tree ()" to get at m_inner here.
> 
> 
> > diff --git a/gcc/jit/jit-recording.c b/gcc/jit/jit-recording.c
> > index 117ff70114c..64f3ae2d8f9 100644
> > --- a/gcc/jit/jit-recording.c
> > +++ b/gcc/jit/jit-recording.c
> > @@ -3713,6 +3713,12 @@ recording::lvalue::get_address
> > (recording::location *loc)
> >    return result;
> >  }
> >  
> > +void
> > +recording::lvalue::set_tls_model (enum gcc_jit_tls_model model)
> > +{
> > +    m_tls_model = model;
> > +}
> > +
> >  /* The implementation of class gcc::jit::recording::param.  */
> >  
> >  /* Implementation of pure virtual hook
> > recording::memento::replay_into
> > @@ -4539,6 +4545,15 @@ recording::block::dump_edges_to_dot
> > (pretty_printer *pp)
> >  #  pragma GCC diagnostic pop
> >  #endif
> >  
> > +namespace recording {
> > +static const enum tls_model tls_models[] = {
> > +  TLS_MODEL_GLOBAL_DYNAMIC, /* GCC_JIT_TLS_MODEL_GLOBAL_DYNAMIC */
> > +  TLS_MODEL_LOCAL_DYNAMIC, /* GCC_JIT_TLS_MODEL_LOCAL_DYNAMIC */
> > +  TLS_MODEL_INITIAL_EXEC, /* GCC_JIT_TLS_MODEL_INITIAL_EXEC */
> > +  TLS_MODEL_LOCAL_EXEC, /* GCC_JIT_TLS_MODEL_LOCAL_EXEC */
> > +};
> > +} /* namespace recording */
> > +
> >  /* The implementation of class gcc::jit::recording::global.  */
> >  
> >  /* Implementation of pure virtual hook
> > recording::memento::replay_into
> > @@ -4547,8 +4562,7 @@ recording::block::dump_edges_to_dot
> > (pretty_printer *pp)
> >  void
> >  recording::global::replay_into (replayer *r)
> >  {
> > -  set_playback_obj (
> > -    m_initializer
> > +    playback::lvalue *global = m_initializer
> >      ? r->new_global_initialized (playback_location (r, m_loc),
> >                                  m_kind,
> >                                  m_type->playback_type (),
> > @@ -4560,7 +4574,12 @@ recording::global::replay_into (replayer *r)
> >      : r->new_global (playback_location (r, m_loc),
> >                      m_kind,
> >                      m_type->playback_type (),
> > -                    playback_string (m_name)));
> > +                    playback_string (m_name));
> > +  if (m_tls_model != GCC_JIT_TLS_MODEL_DEFAULT)
> > +  {
> > +      global->set_tls_model (recording::tls_models[m_tls_model]);
> > +  }
> > +  set_playback_obj (global);
> >  }
> >  
> >  /* Override the default implementation of
> 
> [...snip...]
> 
> > @@ -4675,6 +4702,14 @@ recording::global::write_reproducer
> > (reproducer &r)
> >      r.get_identifier_as_type (get_type ()),
> >      m_name->get_debug_string ());
> >  
> > +  if (m_tls_model)
> > +  {
> 
> I think this conditional should be:
> 
>      if (m_tls_model != GCC_JIT_TLS_MODEL_DEFAULT)
> 
> since the default value isn't 0.  (Maybe it should be?)
> 
> > +       r.write ("  gcc_jit_lvalue_set_tls_model (%s, /*
> gcc_jit_lvalue *lvalue */\n"
> > +            "                                %s); /* enum
> > gcc_jit_tls_model model */\n",
> > +    id,
> > +    tls_model_enum_strings[m_tls_model]);
> > +  }
> > +
> >    if (m_initializer)
> >      switch (m_type->dereference ()->get_size ())
> >        {
> 
> [...snip...]
> 
> >  class param : public lvalue
> > diff --git a/gcc/jit/libgccjit.c b/gcc/jit/libgccjit.c
> > index 0cc650f9810..768b99499cf 100644
> > --- a/gcc/jit/libgccjit.c
> > +++ b/gcc/jit/libgccjit.c
> > @@ -1953,6 +1953,24 @@ gcc_jit_lvalue_get_address (gcc_jit_lvalue
> > *lvalue,
> >    return (gcc_jit_rvalue *)lvalue->get_address (loc);
> >  }
> >  
> > +/* Public entrypoint.  See description in libgccjit.h.
> > +
> > +   After error-checking, the real work is done by the
> > +   gcc::jit::recording::lvalue::set_tls_model method in jit-
> > recording.c.  */
> > +
> > +void
> > +gcc_jit_lvalue_set_tls_model (gcc_jit_lvalue *lvalue,
> > +                           enum gcc_jit_tls_model model)
> > +{
> > +  RETURN_IF_FAIL (lvalue, NULL, NULL, "NULL lvalue");
> > +  JIT_LOG_FUNC (lvalue->get_context ()->get_logger ());
> > +  RETURN_IF_FAIL_PRINTF1 (lvalue->is_global (), NULL, NULL,
> > +                              "lvalue \"%s\" not a global",
> > +                              lvalue->get_debug_string ());
> 
> This should pass lvalue's context to the RETURN_IF_FAIL_PRINTF1, so
> that if it fails, the error is associated with the context.
> 
> 
> > +  lvalue->set_tls_model (model);
> > +}
> > +
> >  /* Public entrypoint.  See description in libgccjit.h.
> >  
> >     After error-checking, the real work is done by the
> > diff --git a/gcc/jit/libgccjit.h b/gcc/jit/libgccjit.h
> > index 5c722c2c57f..2a52b351a49 100644
> > --- a/gcc/jit/libgccjit.h
> > +++ b/gcc/jit/libgccjit.h
> > @@ -722,6 +722,16 @@ enum gcc_jit_function_kind
> >    GCC_JIT_FUNCTION_ALWAYS_INLINE
> >  };
> >  
> > +/* Thread local storage model.  */
> > +enum gcc_jit_tls_model
> > +{
> > +  GCC_JIT_TLS_MODEL_GLOBAL_DYNAMIC,
> > +  GCC_JIT_TLS_MODEL_LOCAL_DYNAMIC,
> > +  GCC_JIT_TLS_MODEL_INITIAL_EXEC,
> > +  GCC_JIT_TLS_MODEL_LOCAL_EXEC,
> > +  GCC_JIT_TLS_MODEL_DEFAULT,
> 
> As noted above, should the DEFAULT one be the first?

I made it the first one.

> 
> If DEFAULT means "not thread-local", maybe "NONE" would be clearer
> than
> "DEFAULT"?
> 
> > +};
> > +
> >  /* Create a function.  */
> >  extern gcc_jit_function *
> >  gcc_jit_context_new_function (gcc_jit_context *ctxt,
> > @@ -1072,6 +1082,17 @@ extern gcc_jit_rvalue *
> >  gcc_jit_lvalue_get_address (gcc_jit_lvalue *lvalue,
> >                             gcc_jit_location *loc);
> >  
> > +#define LIBGCCJIT_HAVE_gcc_jit_lvalue_set_tls_model
> > +
> > +/* Set the thread-local storage model of a global variable
> > +
> > +   This API entrypoint was added in LIBGCCJIT_ABI_17; you can test
> > for its
> > +   presence using
> > +     #ifdef LIBGCCJIT_HAVE_gcc_jit_lvalue_set_tls_model  */
> > +extern void
> > +gcc_jit_lvalue_set_tls_model (gcc_jit_lvalue *lvalue,
> > +                           enum gcc_jit_tls_model model);
> > +
> >  extern gcc_jit_lvalue *
> >  gcc_jit_function_new_local (gcc_jit_function *func,
> >                             gcc_jit_location *loc,
> > diff --git a/gcc/jit/libgccjit.map b/gcc/jit/libgccjit.map
> > index 337ea6c7fe4..605c624ec4a 100644
> > --- a/gcc/jit/libgccjit.map
> > +++ b/gcc/jit/libgccjit.map
> > @@ -205,3 +205,8 @@ LIBGCCJIT_ABI_15 {
> >      gcc_jit_extended_asm_add_clobber;
> >      gcc_jit_context_add_top_level_asm;
> >  } LIBGCCJIT_ABI_14;
> > +
> > +LIBGCCJIT_ABI_16 {
> > +  global:
> > +    gcc_jit_lvalue_set_tls_model;
> > +} LIBGCCJIT_ABI_15;
> 
> Obviously the ABI needs to be fixed up here.
> 
> > diff --git a/gcc/testsuite/jit.dg/all-non-failing-tests.h
> > b/gcc/testsuite/jit.dg/all-non-failing-tests.h
> > index 4202eb7798b..c2d87a30cca 100644
> > --- a/gcc/testsuite/jit.dg/all-non-failing-tests.h
> > +++ b/gcc/testsuite/jit.dg/all-non-failing-tests.h
> > @@ -181,6 +181,13 @@
> >  #undef create_code
> >  #undef verify_code
> >  
> > +/* test-tls.c */
> > +#define create_code create_code_tls
> > +#define verify_code verify_code_tls
> > +#include "test-tls.c"
> > +#undef create_code
> > +#undef verify_code
> > +
> >  /* test-hello-world.c */
> >  #define create_code create_code_hello_world
> >  #define verify_code verify_code_hello_world
> 
> This is missing an entry in the "testcases" array at the bottom of
> the
> header to make use of the new {create|verify}_code_tls functions.
> 
> > diff --git a/gcc/testsuite/jit.dg/test-tls.c
> > b/gcc/testsuite/jit.dg/test-tls.c
> > new file mode 100644
> > index 00000000000..d4508b16c1e
> > --- /dev/null
> > +++ b/gcc/testsuite/jit.dg/test-tls.c
> > @@ -0,0 +1,29 @@
> > +#include <stdlib.h>
> > +#include <stdio.h>
> > +#include <string.h>
> > +#include <time.h>
> > +
> > +#include "libgccjit.h"
> > +
> > +#include "harness.h"
> > +
> > +void
> > +create_code (gcc_jit_context *ctxt, void *user_data)
> > +{
> > +  /* Let's try to inject the equivalent of:
> > +
> > +     _Thread_local int foo;
> > +  */
> > +  gcc_jit_type *int_type =
> > +    gcc_jit_context_get_type (ctxt, GCC_JIT_TYPE_INT);
> > +
> > +  gcc_jit_lvalue *foo =
> > +    gcc_jit_context_new_global (
> > +      ctxt, NULL, GCC_JIT_GLOBAL_EXPORTED, int_type, "foo");
> > +  gcc_jit_lvalue_set_tls_model (foo,
> > GCC_JIT_TLS_MODEL_GLOBAL_DYNAMIC);
> 
> How many of the different enum values can be supported?  How target-
> dependent is this?

I'm not sure what you mean here. Are you asking that I test all the
different enum values?
The tls_model enum is defined in gcc/coretypes.h and does not seem to
change depending on the target. Maybe there are checks elsewhere for
that, though.

> 
> > +}
> > +
> > +void
> > +verify_code (gcc_jit_context *ctxt, gcc_jit_result *result)
> > +{
> 
> Should probably at least try to read and write the global(s).
> 
> Hope this is constructive
> Dave
> 
> 


[-- Attachment #2: 0001-libgccjit-Add-support-for-TLS-variable-PR95415.patch --]
[-- Type: text/x-patch, Size: 13150 bytes --]

From d64f50ff9551d1bd49c9e5304f2d056b031d046b Mon Sep 17 00:00:00 2001
From: Antoni Boucher <bouanto@zoho.com>
Date: Tue, 11 May 2021 19:23:54 -0400
Subject: [PATCH] libgccjit: Add support for TLS variable [PR95415]

2021-11-20  Antoni Boucher  <bouanto@zoho.com>

    gcc/jit/
            PR target/95415
            * docs/topics/compatibility.rst (LIBGCCJIT_ABI_17): New ABI
            tag.
            * docs/topics/expressions.rst: Add document for the function
            gcc_jit_lvalue_set_tls_model.
            * jit-playback.h: New function (set_tls_model) and make
            rvalue::m_inner public.
            * jit-recording.c: New function (set_tls_model), new
            variables (tls_models and tls_model_enum_strings) and support
            for setting the tls model.
            * jit-recording.h: New function (set_tls_model) and new
            field m_tls_model.
            * libgccjit.c: New function (gcc_jit_lvalue_set_tls_model).
            * libgccjit.h: New function (gcc_jit_lvalue_set_tls_model)
            and new enum (gcc_jit_tls_model).
            * libgccjit.map (LIBGCCJIT_ABI_17): New ABI tag.

    gcc/testsuite/
            PR target/95415
            * jit.dg/all-non-failing-tests.h: Add test-tls.c.
            * jit.dg/test-tls.c: New test.
---
 gcc/jit/docs/topics/compatibility.rst        |  9 +++
 gcc/jit/docs/topics/expressions.rst          | 37 +++++++++++
 gcc/jit/jit-playback.h                       |  6 ++
 gcc/jit/jit-recording.c                      | 42 ++++++++++++-
 gcc/jit/jit-recording.h                      |  7 ++-
 gcc/jit/libgccjit.c                          | 18 ++++++
 gcc/jit/libgccjit.h                          | 21 +++++++
 gcc/jit/libgccjit.map                        |  5 ++
 gcc/testsuite/jit.dg/all-non-failing-tests.h | 10 +++
 gcc/testsuite/jit.dg/test-tls.c              | 64 ++++++++++++++++++++
 10 files changed, 215 insertions(+), 4 deletions(-)
 create mode 100644 gcc/testsuite/jit.dg/test-tls.c

diff --git a/gcc/jit/docs/topics/compatibility.rst b/gcc/jit/docs/topics/compatibility.rst
index 52ee3f860a7..2ad6e4232f7 100644
--- a/gcc/jit/docs/topics/compatibility.rst
+++ b/gcc/jit/docs/topics/compatibility.rst
@@ -284,3 +284,12 @@ entrypoints:
   * :func:`gcc_jit_struct_get_field`
 
   * :func:`gcc_jit_struct_get_field_count`
+
+.. _LIBGCCJIT_ABI_17:
+
+``LIBGCCJIT_ABI_17``
+-----------------------
+``LIBGCCJIT_ABI_17`` covers the addition of an API entrypoint to set the
+thread-local storage model of a variable:
+
+  * :func:`gcc_jit_lvalue_set_tls_model`
diff --git a/gcc/jit/docs/topics/expressions.rst b/gcc/jit/docs/topics/expressions.rst
index 396259ef07e..386b80d8f8b 100644
--- a/gcc/jit/docs/topics/expressions.rst
+++ b/gcc/jit/docs/topics/expressions.rst
@@ -539,6 +539,43 @@ where the rvalue is computed by reading from the storage area.
 
    in C.
 
+.. function:: void\
+              gcc_jit_lvalue_set_tls_model (gcc_jit_lvalue *lvalue,\
+                                            enum gcc_jit_tls_model model)
+
+   Make a variable a thread-local variable.
+
+   The "model" parameter determines the thread-local storage model of the "lvalue":
+
+   .. type:: enum gcc_jit_tls_model
+
+   .. c:macro:: GCC_JIT_TLS_MODEL_NONE
+
+      Don't set the TLS model.
+
+   .. c:macro:: GCC_JIT_TLS_MODEL_GLOBAL_DYNAMIC
+
+   .. c:macro:: GCC_JIT_TLS_MODEL_LOCAL_DYNAMIC
+
+   .. c:macro:: GCC_JIT_TLS_MODEL_INITIAL_EXEC
+
+   .. c:macro:: GCC_JIT_TLS_MODEL_LOCAL_EXEC
+
+   This is analogous to:
+
+   .. code-block:: c
+
+     _Thread_local int foo __attribute__ ((tls_model("MODEL")));
+
+   in C.
+
+   This entrypoint was added in :ref:`LIBGCCJIT_ABI_17`; you can test for
+   its presence using
+
+   .. code-block:: c
+
+      #ifdef LIBGCCJIT_HAVE_gcc_jit_lvalue_set_tls_model
+
 Global variables
 ****************
 
diff --git a/gcc/jit/jit-playback.h b/gcc/jit/jit-playback.h
index f670c9e81df..c9839c21a66 100644
--- a/gcc/jit/jit-playback.h
+++ b/gcc/jit/jit-playback.h
@@ -675,6 +675,12 @@ public:
   rvalue *
   get_address (location *loc);
 
+  void
+  set_tls_model (enum tls_model tls_model)
+  {
+    set_decl_tls_model (as_tree (), tls_model);
+  }
+
 private:
   bool mark_addressable (location *loc);
 };
diff --git a/gcc/jit/jit-recording.c b/gcc/jit/jit-recording.c
index 117ff70114c..1296fece4cf 100644
--- a/gcc/jit/jit-recording.c
+++ b/gcc/jit/jit-recording.c
@@ -3713,6 +3713,12 @@ recording::lvalue::get_address (recording::location *loc)
   return result;
 }
 
+void
+recording::lvalue::set_tls_model (enum gcc_jit_tls_model model)
+{
+    m_tls_model = model;
+}
+
 /* The implementation of class gcc::jit::recording::param.  */
 
 /* Implementation of pure virtual hook recording::memento::replay_into
@@ -4539,6 +4545,16 @@ recording::block::dump_edges_to_dot (pretty_printer *pp)
 #  pragma GCC diagnostic pop
 #endif
 
+namespace recording {
+static const enum tls_model tls_models[] = {
+  TLS_MODEL_NONE, /* GCC_JIT_TLS_MODEL_NONE */
+  TLS_MODEL_GLOBAL_DYNAMIC, /* GCC_JIT_TLS_MODEL_GLOBAL_DYNAMIC */
+  TLS_MODEL_LOCAL_DYNAMIC, /* GCC_JIT_TLS_MODEL_LOCAL_DYNAMIC */
+  TLS_MODEL_INITIAL_EXEC, /* GCC_JIT_TLS_MODEL_INITIAL_EXEC */
+  TLS_MODEL_LOCAL_EXEC, /* GCC_JIT_TLS_MODEL_LOCAL_EXEC */
+};
+} /* namespace recording */
+
 /* The implementation of class gcc::jit::recording::global.  */
 
 /* Implementation of pure virtual hook recording::memento::replay_into
@@ -4547,8 +4563,7 @@ recording::block::dump_edges_to_dot (pretty_printer *pp)
 void
 recording::global::replay_into (replayer *r)
 {
-  set_playback_obj (
-    m_initializer
+    playback::lvalue *global = m_initializer
     ? r->new_global_initialized (playback_location (r, m_loc),
 				 m_kind,
 				 m_type->playback_type (),
@@ -4560,7 +4575,12 @@ recording::global::replay_into (replayer *r)
     : r->new_global (playback_location (r, m_loc),
 		     m_kind,
 		     m_type->playback_type (),
-		     playback_string (m_name)));
+		     playback_string (m_name));
+  if (m_tls_model != GCC_JIT_TLS_MODEL_NONE)
+  {
+      global->set_tls_model (recording::tls_models[m_tls_model]);
+  }
+  set_playback_obj (global);
 }
 
 /* Override the default implementation of
@@ -4658,6 +4678,14 @@ recording::global::write_initializer_reproducer (const char *id, reproducer &r)
 
 /* Implementation of recording::memento::write_reproducer for globals. */
 
+static const char * const tls_model_enum_strings[] = {
+  "GCC_JIT_TLS_MODEL_NONE",
+  "GCC_JIT_TLS_MODEL_GLOBAL_DYNAMIC",
+  "GCC_JIT_TLS_MODEL_LOCAL_DYNAMIC",
+  "GCC_JIT_TLS_MODEL_INITIAL_EXEC",
+  "GCC_JIT_TLS_MODEL_LOCAL_EXEC",
+};
+
 void
 recording::global::write_reproducer (reproducer &r)
 {
@@ -4675,6 +4703,14 @@ recording::global::write_reproducer (reproducer &r)
     r.get_identifier_as_type (get_type ()),
     m_name->get_debug_string ());
 
+  if (m_tls_model != GCC_JIT_TLS_MODEL_NONE)
+  {
+	r.write ("  gcc_jit_lvalue_set_tls_model (%s, /* gcc_jit_lvalue *lvalue */\n"
+	     "                                %s); /* enum gcc_jit_tls_model model */\n",
+    id,
+    tls_model_enum_strings[m_tls_model]);
+  }
+
   if (m_initializer)
     switch (m_type->dereference ()->get_size ())
       {
diff --git a/gcc/jit/jit-recording.h b/gcc/jit/jit-recording.h
index 4a994fe7094..f64e9bee9a6 100644
--- a/gcc/jit/jit-recording.h
+++ b/gcc/jit/jit-recording.h
@@ -1112,7 +1112,8 @@ public:
   lvalue (context *ctxt,
 	  location *loc,
 	  type *type_)
-    : rvalue (ctxt, loc, type_)
+    : rvalue (ctxt, loc, type_),
+    m_tls_model (GCC_JIT_TLS_MODEL_NONE)
     {}
 
   playback::lvalue *
@@ -1134,6 +1135,10 @@ public:
   const char *access_as_rvalue (reproducer &r) OVERRIDE;
   virtual const char *access_as_lvalue (reproducer &r);
   virtual bool is_global () const { return false; }
+  void set_tls_model (enum gcc_jit_tls_model model);
+
+protected:
+  enum gcc_jit_tls_model m_tls_model;
 };
 
 class param : public lvalue
diff --git a/gcc/jit/libgccjit.c b/gcc/jit/libgccjit.c
index c744b634f4b..e16eed4a6c0 100644
--- a/gcc/jit/libgccjit.c
+++ b/gcc/jit/libgccjit.c
@@ -2217,6 +2217,24 @@ gcc_jit_lvalue_get_address (gcc_jit_lvalue *lvalue,
   return (gcc_jit_rvalue *)lvalue->get_address (loc);
 }
 
+/* Public entrypoint.  See description in libgccjit.h.
+
+   After error-checking, the real work is done by the
+   gcc::jit::recording::lvalue::set_tls_model method in jit-recording.c.  */
+
+void
+gcc_jit_lvalue_set_tls_model (gcc_jit_lvalue *lvalue,
+			    enum gcc_jit_tls_model model)
+{
+  RETURN_IF_FAIL (lvalue, NULL, NULL, "NULL lvalue");
+  JIT_LOG_FUNC (lvalue->get_context ()->get_logger ());
+  RETURN_IF_FAIL_PRINTF1 (lvalue->is_global (), lvalue->get_context (), NULL,
+			       "lvalue \"%s\" not a global",
+			       lvalue->get_debug_string ());
+
+  lvalue->set_tls_model (model);
+}
+
 /* Public entrypoint.  See description in libgccjit.h.
 
    After error-checking, the real work is done by the
diff --git a/gcc/jit/libgccjit.h b/gcc/jit/libgccjit.h
index a1c9436c545..5aa2e40c7a4 100644
--- a/gcc/jit/libgccjit.h
+++ b/gcc/jit/libgccjit.h
@@ -739,6 +739,16 @@ enum gcc_jit_function_kind
   GCC_JIT_FUNCTION_ALWAYS_INLINE
 };
 
+/* Thread local storage model.  */
+enum gcc_jit_tls_model
+{
+  GCC_JIT_TLS_MODEL_NONE,
+  GCC_JIT_TLS_MODEL_GLOBAL_DYNAMIC,
+  GCC_JIT_TLS_MODEL_LOCAL_DYNAMIC,
+  GCC_JIT_TLS_MODEL_INITIAL_EXEC,
+  GCC_JIT_TLS_MODEL_LOCAL_EXEC,
+};
+
 /* Create a function.  */
 extern gcc_jit_function *
 gcc_jit_context_new_function (gcc_jit_context *ctxt,
@@ -1089,6 +1099,17 @@ extern gcc_jit_rvalue *
 gcc_jit_lvalue_get_address (gcc_jit_lvalue *lvalue,
 			    gcc_jit_location *loc);
 
+#define LIBGCCJIT_HAVE_gcc_jit_lvalue_set_tls_model
+
+/* Set the thread-local storage model of a global variable
+
+   This API entrypoint was added in LIBGCCJIT_ABI_17; you can test for its
+   presence using
+     #ifdef LIBGCCJIT_HAVE_gcc_jit_lvalue_set_tls_model  */
+extern void
+gcc_jit_lvalue_set_tls_model (gcc_jit_lvalue *lvalue,
+			    enum gcc_jit_tls_model model);
+
 extern gcc_jit_lvalue *
 gcc_jit_function_new_local (gcc_jit_function *func,
 			    gcc_jit_location *loc,
diff --git a/gcc/jit/libgccjit.map b/gcc/jit/libgccjit.map
index 64e790949e8..98d693fd00c 100644
--- a/gcc/jit/libgccjit.map
+++ b/gcc/jit/libgccjit.map
@@ -226,3 +226,8 @@ LIBGCCJIT_ABI_16 {
     gcc_jit_type_is_struct;
     gcc_jit_struct_get_field_count;
 } LIBGCCJIT_ABI_15;
+
+LIBGCCJIT_ABI_17 {
+  global:
+    gcc_jit_lvalue_set_tls_model;
+} LIBGCCJIT_ABI_16;
diff --git a/gcc/testsuite/jit.dg/all-non-failing-tests.h b/gcc/testsuite/jit.dg/all-non-failing-tests.h
index a7fddf96db8..f64ea91b14e 100644
--- a/gcc/testsuite/jit.dg/all-non-failing-tests.h
+++ b/gcc/testsuite/jit.dg/all-non-failing-tests.h
@@ -188,6 +188,13 @@
 #undef create_code
 #undef verify_code
 
+/* test-tls.c */
+#define create_code create_code_tls
+#define verify_code verify_code_tls
+#include "test-tls.c"
+#undef create_code
+#undef verify_code
+
 /* test-hello-world.c */
 #define create_code create_code_hello_world
 #define verify_code verify_code_hello_world
@@ -453,6 +460,9 @@ const struct testcase testcases[] = {
   {"switch",
    create_code_switch,
    verify_code_switch},
+  {"tls",
+   create_code_tls,
+   verify_code_tls},
   {"types",
    create_code_types,
    verify_code_types},
diff --git a/gcc/testsuite/jit.dg/test-tls.c b/gcc/testsuite/jit.dg/test-tls.c
new file mode 100644
index 00000000000..3b20182ac10
--- /dev/null
+++ b/gcc/testsuite/jit.dg/test-tls.c
@@ -0,0 +1,64 @@
+#include <stdlib.h>
+#include <stdio.h>
+#include <string.h>
+#include <time.h>
+
+#include "libgccjit.h"
+
+#include "harness.h"
+
+void
+create_code (gcc_jit_context *ctxt, void *user_data)
+{
+  /* Let's try to inject the equivalent of:
+
+     _Thread_local int foo;
+
+     int test_using_tls()
+     {
+      foo = 42;
+      return foo;
+     }
+  */
+  gcc_jit_type *int_type =
+    gcc_jit_context_get_type (ctxt, GCC_JIT_TYPE_INT);
+
+  gcc_jit_lvalue *foo =
+    gcc_jit_context_new_global (
+      ctxt, NULL, GCC_JIT_GLOBAL_EXPORTED, int_type, "foo");
+  gcc_jit_lvalue_set_tls_model (foo, GCC_JIT_TLS_MODEL_GLOBAL_DYNAMIC);
+
+  /* Build the test_fn.  */
+  gcc_jit_function *test_fn =
+    gcc_jit_context_new_function (ctxt, NULL,
+				  GCC_JIT_FUNCTION_EXPORTED,
+				  int_type,
+				  "test_using_tls",
+				  0, NULL,
+				  0);
+  gcc_jit_block *block = gcc_jit_function_new_block (test_fn, NULL);
+  gcc_jit_block_add_assignment (
+    block, NULL,
+    foo,
+    gcc_jit_context_new_rvalue_from_int (ctxt, int_type, 42));
+  gcc_jit_block_end_with_return (block,
+				 NULL,
+				 gcc_jit_lvalue_as_rvalue (foo));
+}
+
+void
+verify_code (gcc_jit_context *ctxt, gcc_jit_result *result)
+{
+  typedef int (*fn_type) (void);
+  CHECK_NON_NULL (result);
+
+  fn_type test_using_tls =
+    (fn_type)gcc_jit_result_get_code (result, "test_using_tls");
+  CHECK_NON_NULL (test_using_tls);
+  int return_value = test_using_tls();
+  CHECK_VALUE (return_value, 42);
+
+  int *foo = (int *)gcc_jit_result_get_global (result, "foo");
+  CHECK_NON_NULL (foo);
+  CHECK_VALUE (*foo, 42);
+}
-- 
2.26.2.7.g19db9cfb68.dirty

Re: [PATCH] libgccjit: Add support for TLS variable [PR95415]

[David Malcolm]

On Sat, 2021-11-20 at 17:34 -0500, Antoni Boucher wrote:
> Hi.
> Here's the updated patch.
> See comments below.
> Thanks for your reviews!
> 
> Le jeudi 20 mai 2021 à 16:11 -0400, David Malcolm a écrit :
> > On Tue, 2021-05-18 at 20:43 -0400, Antoni Boucher via Gcc-patches
> > wrote:
> > > Hello.
> > > This patch adds support for TLS variables.
> > > One thing to fix before we merge it is the libgccjit.map file
> > > which
> > > contains LIBGCCJIT_ABI_16 instead of LIBGCCJIT_ABI_17.
> > > LIBGCCJIT_ABI_16 was added in one of my other patches.
> > > Thanks for the review.
> > 
> > > diff --git a/gcc/jit/docs/topics/compatibility.rst
> > > b/gcc/jit/docs/topics/compatibility.rst
> > > index 239b6aa1a92..d10bc1df080 100644
> > > --- a/gcc/jit/docs/topics/compatibility.rst
> > > +++ b/gcc/jit/docs/topics/compatibility.rst
> > > @@ -243,3 +243,12 @@ embedding assembler instructions:
> > >    * :func:`gcc_jit_extended_asm_add_input_operand`
> > >    * :func:`gcc_jit_extended_asm_add_clobber`
> > >    * :func:`gcc_jit_context_add_top_level_asm`
> > > +
> > > +.. _LIBGCCJIT_ABI_17:
> > > +
> > > +``LIBGCCJIT_ABI_17``
> > > +-----------------------
> > > +``LIBGCCJIT_ABI_17`` covers the addition of an API entrypoint to
> > > set
> > > the
> > > +thread-local storage model of a variable:
> > > +
> > > +  * :func:`gcc_jit_lvalue_set_tls_model`
> > 
> > Sorry about the delay in reviewing patches.
> > 
> > Is there a summary somewhere of the various outstanding patches and
> > their associated ABI versions?  Are there dependencies between the
> > patches?
> 
> The list of patches is there:
> https://github.com/antoyo/libgccjit-patches but I don't keep them up-
> to-date.
> If that would help you, I could add a README to tell what is the new
> ABI version for each patch.
> I believe there might be some patches that depend on a previous one.

That's not needed; I think all I need to know is what the next patch
you need me to look at is (FWIW I'm about to go on vacation for a week)

[...snip...]

> 
> > 
> > > +
> > > +void
> > > +create_code (gcc_jit_context *ctxt, void *user_data)
> > > +{
> > > +  /* Let's try to inject the equivalent of:
> > > +
> > > +     _Thread_local int foo;
> > > +  */
> > > +  gcc_jit_type *int_type =
> > > +    gcc_jit_context_get_type (ctxt, GCC_JIT_TYPE_INT);
> > > +
> > > +  gcc_jit_lvalue *foo =
> > > +    gcc_jit_context_new_global (
> > > +      ctxt, NULL, GCC_JIT_GLOBAL_EXPORTED, int_type, "foo");
> > > +  gcc_jit_lvalue_set_tls_model (foo,
> > > GCC_JIT_TLS_MODEL_GLOBAL_DYNAMIC);
> > 
> > How many of the different enum values can be supported?  How
> > target-
> > dependent is this?
> 
> I'm not sure what you mean here. Are you asking that I test all the
> different enum values?

That would be ideal, but I don't think it's necessary.

> The tls_model enum is defined in gcc/coretypes.h and does not seem to
> change depending on the target. Maybe there are checks elsewhere for
> that, though.

It might be that some targets only support some modes; I don't know.


[...snip...]

Thanks for the updated patch.  It looks good to push to trunk once the
earlier ones are in place, though as usual please re-test it before
pushing.

Dave