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
