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

[David Malcolm <dmalcolm@redhat.com>]

On Sat, 2019-01-05 at 23:10 +0100, Marc Nieper-Wißkirchen wrote:
> This patch adds thread-local globals to the libgccjit frontend. The
> library user can mark a global as being thread-local by calling
> `gcc_jit_lvalue_set_bool_thread_local'. It is implemented by calling
> `set_decl_tls_model (inner, decl_default_tls_model (inner))', where
> `inner' is the GENERIC tree corresponding to the global.

Thanks for creating this patch.

Have you done the legal paperwork with the FSF for contributing to GCC?
 See https://gcc.gnu.org/contribute.html#legal

Overall, this looks pretty good (though we'd want to get release
manager approval for late-breaking changes given where we are in the
release cycle).

Various comments inline below.

> ChangeLog
> 
> 2019-01-05  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.
> 
> Testing
> 
> The result of `make check-jit' is (the failing test in
> `test-sum-squares.c` is also failing without this patch on my
> machine):
> 
> Native configuration is x86_64-pc-linux-gnu

[...]

> FAIL:  test-combination.c.exe iteration 1 of 5:
> verify_code_sum_of_squares: dump_vrp1: actual: "
> FAIL: test-combination.c.exe killed: 20233 exp6 0 0 CHILDKILLED
> SIGABRT SIGABRT
> FAIL:  test-sum-of-squares.c.exe iteration 1 of 5: verify_code:
> dump_vrp1: actual: "
> FAIL: test-sum-of-squares.c.exe killed: 22698 exp6 0 0 CHILDKILLED
> SIGABRT SIGABRT
> FAIL:  test-threads.c.exe: verify_code_sum_of_squares: dump_vrp1:
> actual: "
> FAIL: test-threads.c.exe killed: 22840 exp6 0 0 CHILDKILLED SIGABRT
> SIGABRT

That one's failing for me as well.  I'm investigating (I've filed it as
PR jit/88747).

[...]

diff --git a/gcc/testsuite/jit.dg/all-non-failing-tests.h
> b/gcc/testsuite/jit.dg/all-non-failing-tests.h
> index bf02e1258..c2654ff09 100644
> --- a/gcc/testsuite/jit.dg/all-non-failing-tests.h
> +++ b/gcc/testsuite/jit.dg/all-non-failing-tests.h
> @@ -224,6 +224,13 @@
>  #undef create_code
>  #undef verify_code
> 
> +/* test-factorial-must-tail-call.c */

Looks like a cut&paste error: presumably the above comment is meant to
refer to the new test...

> +#define create_code create_code_thread_local
> +#define verify_code verify_code_thread_local
> +#include "test-thread-local.c"

...but it looks like the new test file is missing from the patch.

> +#undef create_code
> +#undef verify_code
> +
>  /* test-types.c */
>  #define create_code create_code_types
>  #define verify_code verify_code_types
> @@ -353,6 +360,9 @@ const struct testcase testcases[] = {
>    {"switch",
>     create_code_switch,
>     verify_code_switch},
> +  {"thread_local",
> +   create_code_thread_local,
> +   verify_code_thread_local},
>    {"types",
>     create_code_types,
>     verify_code_types},

[...]

Thanks
Dave