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