From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by sourceware.org (Postfix) with ESMTP id 676F13980404 for ; Thu, 20 May 2021 20:11:09 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 676F13980404 Received: from mail-qt1-f200.google.com (mail-qt1-f200.google.com [209.85.160.200]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-155-7IqfJDKoN9e3yt1km4YBAw-1; Thu, 20 May 2021 16:11:06 -0400 X-MC-Unique: 7IqfJDKoN9e3yt1km4YBAw-1 Received: by mail-qt1-f200.google.com with SMTP id h2-20020a05622a1702b02901b9123889b0so13175236qtk.10 for ; Thu, 20 May 2021 13:11:06 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:message-id:subject:from:to:date:in-reply-to :references:user-agent:mime-version:content-transfer-encoding; bh=11ui97ujrd3heOCzl2OIs4nPrd5Dsu5uAqZvZHEuJl8=; b=kyGCVvD2lnVJ5why+62GB6nQvQXuxrCzuyidsGMP1IVm7TD3yTJpbB+pJMm0YkbPhM sJJSQKynYSYBhkJtEpWJQvtFvY3h3K78iHGrvopZ/ZuUnDb+8pUy/Q2H2+ZlmVPXDckv lmkk21g0EWqWrrYV6vvFjhusHNTbAPFNDcLbhKvd24Mrkwzaeq6OGfN4zvlrdYkSnKMq v8aLlBbhgSuElRrNQb+K39iqjd/lHIfxhgyH1XQ+60ZpUyvICAhcjHU1dUsS/oBOopw9 tpcbImY3DbS7/yxRIGLVUqD1k1lW8kR2qJQ6+DolwH6+EMMcrVeuJffxcy5bGHogbuNW WgwA== X-Gm-Message-State: AOAM530P336MXXmi8Mr1u+S9ACk5BPTzS2qspW4azqFpmsZd4EhVWSo/ rOFeuINnMfL8P6SaOVm2i+FHGFm1nOt9DYKzRkSPqUOkxQF/6qZTV0srhGvrE93oMWlZVFPbazW 72wb87wA= X-Received: by 2002:a0c:e951:: with SMTP id n17mr7852064qvo.49.1621541465486; Thu, 20 May 2021 13:11:05 -0700 (PDT) X-Google-Smtp-Source: ABdhPJzMT2UXxpHZ7wAb6oxy7E/QKtMJJ1wYbkm85prcQ5890vWaxoWA6HfNkPkwXQarEidouafhDQ== X-Received: by 2002:a0c:e951:: with SMTP id n17mr7852033qvo.49.1621541465190; Thu, 20 May 2021 13:11:05 -0700 (PDT) Received: from t14s.localdomain (c-73-69-212-193.hsd1.nh.comcast.net. [73.69.212.193]) by smtp.gmail.com with ESMTPSA id a27sm1707268qtn.97.2021.05.20.13.11.04 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 20 May 2021 13:11:04 -0700 (PDT) Message-ID: Subject: Re: [PATCH] libgccjit: Add support for TLS variable [PR95415] From: David Malcolm To: Antoni Boucher , jit@gcc.gnu.org, gcc-patches@gcc.gnu.org Date: Thu, 20 May 2021 16:11:03 -0400 In-Reply-To: <750892c90ccd75ae1df099b829ce2d51fe886195.camel@zoho.com> References: <750892c90ccd75ae1df099b829ce2d51fe886195.camel@zoho.com> User-Agent: Evolution 3.38.4 (3.38.4-1.fc33) MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-12.4 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_SHORT, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H4, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: jit@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Jit mailing list List-Unsubscribe: , List-Archive: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 20 May 2021 20:11:11 -0000 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 > +#include > +#include > +#include > + > +#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