Re: [PATCH][jit] Add thread-local globals to the libgccjit frontend

["Marc Nieper-Wißkirchen" <marc@nieper-wisskirchen.de>]

[...]

>> 2019-01-08  Marc Nieper-Wißkirchen  <marc@nieper-wisskirchen.de>
>>
>>           * docs/topics/compatibility.rst: Add LIBGCCJIT_ABI_11.
>>           * docs/topics/expressions.rst (Global variables): Add
>>           documentation of gcc_jit_lvalue_set_bool_thread_local.
>> 	* docs/_build/texinfo/libgccjit.texi: Regenerate.
>> 	* jit-playback.c: Include "varasm.h".
>> 	Within namespace gcc::jit::playback...
>> 	(context::new_global) Add "thread_local_p" param and use it
>> 	to set DECL_TLS_MODEL.
>> 	* jit-playback.h: Within namespace gcc::jit::playback...
>> 	(context::new_global): Add "thread_local_p" param.
>> 	* jit-recording.c: Within namespace gcc::jit::recording...
>> 	(global::replay_into): Provide m_thread_local to call to
>> 	new_global.
>> 	(global::write_reproducer): Call write_reproducer_thread_local.
>> 	(global::write_reproducer_thread_local): New method.
>> 	* jit-recording.h: Within namespace gcc::jit::recording...
>> 	(lvalue::dyn_cast_global): New virtual function.
>> 	(global::m_thread_local): New field.
>> 	* libgccjit.c (gcc_jit_lvalue_set_bool_thread_local): New
>> 	function.
>> 	* libgccjit.h
>> 	(LIBGCCJIT_HAVE_gcc_jit_lvalue_set_bool_thread_local): New
>> 	macro.
>> 	(gcc_jit_lvalue_set_bool_thread_local): New function.
>> 	* libgccjit.map (LIBGCCJIT_ABI_11): New.
>> 	(gcc_jit_lvalue_set_bool_thread_local): Add.
>> 	* ../testsuite/jit.dg/all-non-failing-tests.h: Include new
>> test.
>> 	* ../testsuite/jit.dg/jit.exp: Load pthread for tests involving
>> 	thread-local globals.
>> 	* ../testsuite/jit.dg/test-thread-local.c: New test case for
>> 	thread-local globals.
> 
> BTW, the convention here is to split out the ChangeLog entries by
> directory based on the presence of ChangeLog files.
> 
> There's a gcc/jit/ChangeLog and a gcc/testsuite/ChangeLog, so for this
> patch there should be two sets of ChangeLog entries, one for each of
> these, with the paths expressed relative to the directory holding the
> ChangeLog.
> 
> So the testsuite entries would go into gcc/testsuite/ChangeLog, and
> look like:
> 
> 	* jit.dg/all-non-failing-tests.h: Include new test.
> 
> ...etc.
> 

Thanks for explaining the policy.  Corrected ChangeLogs:

gcc/jit/ChangeLog
=================

2019-01-08  Marc Nieper-Wißkirchen  <marc@nieper-wisskirchen.de>

         * docs/topics/compatibility.rst: Add LIBGCCJIT_ABI_11.
         * docs/topics/expressions.rst (Global variables): Add
         documentation of gcc_jit_lvalue_set_bool_thread_local.
	* docs/_build/texinfo/libgccjit.texi: Regenerate.
	* jit-playback.c: Include "varasm.h".
	Within namespace gcc::jit::playback...
	(context::new_global) Add "thread_local_p" param and use it
	to set DECL_TLS_MODEL.
	* jit-playback.h: Within namespace gcc::jit::playback...
	(context::new_global): Add "thread_local_p" param.
	* jit-recording.c: Within namespace gcc::jit::recording...
	(global::replay_into): Provide m_thread_local to call to
	new_global.
	(global::write_reproducer): Call write_reproducer_thread_local.
	(global::write_reproducer_thread_local): New method.
	* jit-recording.h: Within namespace gcc::jit::recording...
	(lvalue::dyn_cast_global): New virtual function.
	(global::m_thread_local): New field.
	* libgccjit.c (gcc_jit_lvalue_set_bool_thread_local): New
	function.
	* libgccjit.h
	(LIBGCCJIT_HAVE_gcc_jit_lvalue_set_bool_thread_local): New
	macro.
	(gcc_jit_lvalue_set_bool_thread_local): New function.
	* libgccjit.map (LIBGCCJIT_ABI_11): New.
	(gcc_jit_lvalue_set_bool_thread_local): Add.

gcc/testsuite/ChangeLog
=======================

2019-01-08  Marc Nieper-Wißkirchen  <marc@nieper-wisskirchen.de>

	* jit.dg/all-non-failing-tests.h: Include new test.
	* jit.dg/jit.exp: Load pthread for tests involving
	thread-local globals.
	* jit.dg/test-thread-local.c: New test case for
	thread-local globals.

[...]

>> diff --git a/gcc/testsuite/jit.dg/test-thread-local.c
>> b/gcc/testsuite/jit.dg/test-thread-local.c
>> new file mode 100644
>> index 000000000..287ba85e4
>> --- /dev/null
>> +++ b/gcc/testsuite/jit.dg/test-thread-local.c
>> @@ -0,0 +1,99 @@
>> +#include <stdlib.h>
>> +#include <stdio.h>
>> +#include <string.h>
>> +#include <pthread.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:
>> +
>> +      static thread_local int tl;
>> +      set_tl (int v)
>> +      {
>> +        tl = v;
>> +      }
>> +
>> +      int
>> +      get_tl (void)
>> +      {
>> +        return tl;
>> +      }
> 
> Thanks for posting this.
> 
> This test is OK as far as it goes, but there are some gaps in test
> coverage: the test verifies that jit-generated code can create a new
> thread-local variable, and writes to it, but it doesn't seem to test
> reading from it; e.g. there doesn't seem to be a CHECK_VALUE that the
> thread-local var is 43 after the set_tl (43);

The test does check reading the thread-local variable in the main 
thread.  If the variable were non thread-local, the value 42 set in the 
main thread would have been overwritten by the subordinate thread.

> 
> It would probably also be good to verify that jit-generated code can
> work with a thread-local variable declared and defined in C-generated
> code.
> 
> Thinking aloud, how about the following changes:
> - split out the thread-local vars there's e.g. a
>      extern thread_local int tl_c;
>    in the C code and the "tl" becomes "tl_jit", and access it via a
>    GCC_JIT_GLOBAL_EXTERNAL, or somesuch.
> - in the test code, run two new threads, passing them two different
> "int" values; verify that the set_tl(value); can be matched with a
> CHECK_VALUE get_tl() == the value for that thread on the two threads.

Is the following extended test-case along the lines you have been thinking?

diff --git a/gcc/testsuite/jit.dg/test-thread-local.c 
b/gcc/testsuite/jit.dg/test-thread-local.c
new file mode 100644
index 000000000..e21f144c4
--- /dev/null
+++ b/gcc/testsuite/jit.dg/test-thread-local.c
@@ -0,0 +1,161 @@
+#include <stdlib.h>
+#include <stdio.h>
+#include <string.h>
+#include <pthread.h>
+
+#include "libgccjit.h"
+
+#include "harness.h"
+
+int _Thread_local tl_c;
+
+void
+create_code (gcc_jit_context *ctxt, void *user_data)
+{
+  /* Let's try to inject the equivalent of:
+
+      extern thread_local int tl_c;
+      static thread_ int tl_jit;
+      set_tl_jit (int v)
+      {
+        tl_jit = v;
+      }
+
+      int
+      get_tl_jit (void)
+      {
+        return tl_jit;
+      }
+
+      set_tl_c (int v)
+      {
+        tl_c = v;
+      }
+
+      int
+      get_tl_c (void)
+      {
+        return tl_c;
+      }
+   */
+  gcc_jit_type *the_type =
+    gcc_jit_context_get_type (ctxt, GCC_JIT_TYPE_INT);
+  gcc_jit_type *void_type =
+    gcc_jit_context_get_type (ctxt, GCC_JIT_TYPE_VOID);
+
+  gcc_jit_lvalue *tl =
+    gcc_jit_context_new_global (ctxt, NULL, GCC_JIT_GLOBAL_INTERNAL,
+				the_type, "tl_jit");
+  gcc_jit_lvalue_set_bool_thread_local (tl, 1);
+
+  gcc_jit_param *v =
+    gcc_jit_context_new_param (ctxt, NULL, the_type, "v");
+  gcc_jit_param *params[1] = {v};
+  gcc_jit_function *func =
+    gcc_jit_context_new_function (ctxt, NULL,
+				  GCC_JIT_FUNCTION_EXPORTED,
+				  void_type,
+				  "set_tl_jit",
+				  1, params, 0);
+  gcc_jit_block *block =
+    gcc_jit_function_new_block (func, "initial");
+  gcc_jit_block_add_assignment (block, NULL, tl,
+				gcc_jit_param_as_rvalue (v));
+  gcc_jit_block_end_with_void_return (block, NULL);
+
+  func =
+    gcc_jit_context_new_function (ctxt, NULL,
+				  GCC_JIT_FUNCTION_EXPORTED,
+				  the_type,
+				  "get_tl_jit",
+				  0, NULL, 0);
+  block =
+    gcc_jit_function_new_block (func, "initial");
+  gcc_jit_block_end_with_return (block, NULL,
+				 gcc_jit_lvalue_as_rvalue (tl));
+
+  tl =
+    gcc_jit_context_new_global (ctxt, NULL, GCC_JIT_GLOBAL_IMPORTED,
+				the_type, "tl_c");
+  gcc_jit_lvalue_set_bool_thread_local (tl, 1);
+
+  v =
+    gcc_jit_context_new_param (ctxt, NULL, the_type, "v");
+  params[0] = v;
+  func =
+    gcc_jit_context_new_function (ctxt, NULL,
+				  GCC_JIT_FUNCTION_EXPORTED,
+				  void_type,
+				  "set_tl_c",
+				  1, params, 0);
+  block =
+    gcc_jit_function_new_block (func, "initial");
+  gcc_jit_block_add_assignment (block, NULL, tl,
+				gcc_jit_param_as_rvalue (v));
+  gcc_jit_block_end_with_void_return (block, NULL);
+
+  func =
+    gcc_jit_context_new_function (ctxt, NULL,
+				  GCC_JIT_FUNCTION_EXPORTED,
+				  the_type,
+				  "get_tl_c",
+				  0, NULL, 0);
+  block =
+    gcc_jit_function_new_block (func, "initial");
+  gcc_jit_block_end_with_return (block, NULL,
+				 gcc_jit_lvalue_as_rvalue (tl));
+}
+
+typedef void (*set_tl_fn_type) (int);
+typedef int (*get_tl_fn_type) (void);
+
+static set_tl_fn_type set_tl_jit, set_tl_c;
+static get_tl_fn_type get_tl_jit, get_tl_c;
+
+static void *
+test_thread_local_run (void *arg)
+{
+  set_tl_jit (43);
+  set_tl_c (101);
+  int val = get_tl_jit ();
+  CHECK_VALUE (val, 43);
+  val = get_tl_c ();
+  CHECK_VALUE (val, 101);
+  return NULL;
+}
+
+void
+verify_code (gcc_jit_context *ctxt, gcc_jit_result *result)
+{
+
+  CHECK_NON_NULL (result);
+
+  set_tl_jit =
+    (set_tl_fn_type) gcc_jit_result_get_code (result, "set_tl_jit");
+  get_tl_jit =
+    (get_tl_fn_type) gcc_jit_result_get_code (result, "get_tl_jit");
+  set_tl_c =
+    (set_tl_fn_type) gcc_jit_result_get_code (result, "set_tl_c");
+  get_tl_c =
+    (get_tl_fn_type) gcc_jit_result_get_code (result, "get_tl_c");
+
+  CHECK_NON_NULL (set_tl_jit);
+  CHECK_NON_NULL (get_tl_jit);
+  CHECK_NON_NULL (set_tl_c);
+  CHECK_NON_NULL (get_tl_c);
+
+  set_tl_jit (42);
+  set_tl_c (100);
+
+  pthread_t thread;
+  CHECK_VALUE (pthread_create(&thread, NULL, test_thread_local_run, 
NULL), 0);
+  CHECK_VALUE (pthread_join(thread, NULL), 0);
+
+  int val = get_tl_jit ();
+  note ("get_tl_jit returned: %d", val);
+  CHECK_VALUE (val, 42);
+
+  val = get_tl_c ();
+  note ("get_tl_c returned: %d", val);
+  CHECK_VALUE (val, 100);
+}

> ...or somesuch, though the patch is in pretty good shape already.

Thanks! Your information at 
https://gcc.gnu.org/onlinedocs/jit/internals/index.html has been 
extremely helpful.

[...]