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

[David Malcolm <dmalcolm@redhat.com>]

On Tue, 2019-01-08 at 14:31 +0100, Marc Nieper-Wißkirchen wrote:
> Dear David,
> 
> thank you very much for your timely response and for talking a
> thorough 
> look at my proposed patch.
> 
> Am 07.01.19 um 21:34 schrieb David Malcolm:
> 
> > Have you done the legal paperwork with the FSF for contributing to
> > GCC?
> >   See https://gcc.gnu.org/contribute.html#legal
> 
> Not yet; this is my first patch I would like to contribute to GCC.
> You 
> should have received a private email to get the legal matters done.

Thanks; I've replied to that.

[...]


> I have applied your recent patch. With the patch, there are no more 
> failures.

Excellent.

[...]

> # of expected passes        10394


[...]


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

[...]

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

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.

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

Dave

> +   */
> +  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");
> +  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",
> +				  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",
> +				  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));
> +}
> +
> +static void *
> +test_thread_local_run (void *arg)
> +{
> +  void (*set_tl) (int) = arg;
> +  set_tl (43);
> +  return NULL;
> +}
> +
> +void
> +verify_code (gcc_jit_context *ctxt, gcc_jit_result *result)
> +{
> +  typedef void (*set_tl_fn_type) (int);
> +  typedef int (*get_tl_fn_type) (void);
> +
> +  CHECK_NON_NULL (result);
> +
> +  set_tl_fn_type set_tl =
> +    (set_tl_fn_type) gcc_jit_result_get_code (result, "set_tl");
> +  get_tl_fn_type get_tl =
> +    (get_tl_fn_type) gcc_jit_result_get_code (result, "get_tl");
> +
> +  CHECK_NON_NULL (set_tl);
> +  CHECK_NON_NULL (get_tl);
> +
> +  set_tl (42);
> +
> +  pthread_t thread;
> +  CHECK_VALUE (pthread_create(&thread, NULL, test_thread_local_run,
> set_tl), 0);
> +  CHECK_VALUE (pthread_join(thread, NULL), 0);
> +
> +  int val = get_tl ();
> +
> +  note ("get_tl returned: %d", val);
> +  CHECK_VALUE (val, 42);
> +}
>