* [PATCH] libgccjit: Add support for setting the link section of global variables [PR100688] @ 2021-05-20 0:32 Antoni Boucher 2021-05-20 19:29 ` David Malcolm 0 siblings, 1 reply; 7+ messages in thread From: Antoni Boucher @ 2021-05-20 0:32 UTC (permalink / raw) To: jit, gcc-patches [-- Attachment #1: Type: text/plain, Size: 161 bytes --] Hello. This patch adds support to set the link section of global variables. I used the ABI 18 because I submitted other patches up to 17. Thanks for the review. [-- Attachment #2: 0001-Add-support-for-setting-the-link-section-of-global-v.patch --] [-- Type: text/x-patch, Size: 9729 bytes --] From c867732ee36003759d479497c85135ecfc4a0cf3 Mon Sep 17 00:00:00 2001 From: Antoni Boucher <bouanto@zoho.com> Date: Wed, 12 May 2021 07:57:54 -0400 Subject: [PATCH] Add support for setting the link section of global variables [PR100688] 2021-05-19 Antoni Boucher <bouanto@zoho.com> gcc/jit/ PR target/100688 * docs/topics/compatibility.rst (LIBGCCJIT_ABI_18): New ABI tag. * docs/topics/expressions.rst: Add documentation for the function gcc_jit_lvalue_set_link_section. * jit-playback.h: New function (set_link_section) and rvalue::m_inner protected. * jit-recording.c: New function (set_link_section) and support for setting the link section. * jit-recording.h: New function (set_link_section) and new field m_link_section. * libgccjit.c: New function (gcc_jit_lvalue_set_link_section). * libgccjit.h: New function (gcc_jit_lvalue_set_link_section). * libgccjit.map (LIBGCCJIT_ABI_18): New ABI tag. gcc/testsuite/ PR target/100688 * jit.dg/all-non-failing-tests.h: Add test-link-section.c. * jit.dg/test-link_section.c: New test. --- gcc/jit/docs/topics/compatibility.rst | 9 +++++++ gcc/jit/docs/topics/expressions.rst | 12 ++++++++++ gcc/jit/jit-playback.h | 8 +++++++ gcc/jit/jit-recording.c | 23 +++++++++++++++--- gcc/jit/jit-recording.h | 7 +++++- gcc/jit/libgccjit.c | 12 ++++++++++ gcc/jit/libgccjit.h | 13 ++++++++++ gcc/jit/libgccjit.map | 11 +++++++++ gcc/testsuite/jit.dg/all-non-failing-tests.h | 7 ++++++ gcc/testsuite/jit.dg/test-link-section.c | 25 ++++++++++++++++++++ 10 files changed, 123 insertions(+), 4 deletions(-) create mode 100644 gcc/testsuite/jit.dg/test-link-section.c diff --git a/gcc/jit/docs/topics/compatibility.rst b/gcc/jit/docs/topics/compatibility.rst index 239b6aa1a92..94e3127f049 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_18: + +``LIBGCCJIT_ABI_18`` +----------------------- +``LIBGCCJIT_ABI_18`` covers the addition of an API entrypoint to set the link +section of a variable: + + * :func:`gcc_jit_lvalue_set_link_section` diff --git a/gcc/jit/docs/topics/expressions.rst b/gcc/jit/docs/topics/expressions.rst index 396259ef07e..b39f6c02527 100644 --- a/gcc/jit/docs/topics/expressions.rst +++ b/gcc/jit/docs/topics/expressions.rst @@ -539,6 +539,18 @@ where the rvalue is computed by reading from the storage area. in C. +.. function:: void + gcc_jit_lvalue_set_link_section (gcc_jit_lvalue *lvalue, + const char *name) + + Set the link section of a variable; analogous to: + + .. code-block:: c + + int variable __attribute__((section(".section"))); + + in C. + Global variables **************** diff --git a/gcc/jit/jit-playback.h b/gcc/jit/jit-playback.h index 825a3e172e9..8b0f65e87e8 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; }; @@ -670,6 +672,12 @@ public: rvalue * get_address (location *loc); + void + set_link_section (const char* name) + { + set_decl_section_name (m_inner, name); + } + private: bool mark_addressable (location *loc); }; diff --git a/gcc/jit/jit-recording.c b/gcc/jit/jit-recording.c index 117ff70114c..214e6f487fe 100644 --- a/gcc/jit/jit-recording.c +++ b/gcc/jit/jit-recording.c @@ -3713,6 +3713,11 @@ recording::lvalue::get_address (recording::location *loc) return result; } +void recording::lvalue::set_link_section (const char *name) +{ + m_link_section = new_string (name); +} + /* The implementation of class gcc::jit::recording::param. */ /* Implementation of pure virtual hook recording::memento::replay_into @@ -4547,8 +4552,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 +4564,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_link_section != NULL) + { + global->set_link_section (m_link_section->c_str ()); + } + set_playback_obj (global); } /* Override the default implementation of @@ -4675,6 +4684,14 @@ recording::global::write_reproducer (reproducer &r) r.get_identifier_as_type (get_type ()), m_name->get_debug_string ()); + if (m_link_section != NULL) + { + r.write (" gcc_jit_lvalue_set_link_section (%s, /* gcc_jit_lvalue *lvalue */\n" + " \"%s\"); /* */\n", + id, + m_link_section->c_str ()); + } + if (m_initializer) switch (m_type->dereference ()->get_size ()) { diff --git a/gcc/jit/jit-recording.h b/gcc/jit/jit-recording.h index 03fa1160cf0..8ca82c928b8 100644 --- a/gcc/jit/jit-recording.h +++ b/gcc/jit/jit-recording.h @@ -1105,7 +1105,8 @@ public: lvalue (context *ctxt, location *loc, type *type_) - : rvalue (ctxt, loc, type_) + : rvalue (ctxt, loc, type_), + m_link_section (NULL) {} playback::lvalue * @@ -1127,6 +1128,10 @@ public: const char *access_as_rvalue (reproducer &r) OVERRIDE; virtual const char *access_as_lvalue (reproducer &r); virtual bool is_global () const { return false; } + void set_link_section (const char *name); + +protected: + string *m_link_section; }; class param : public lvalue diff --git a/gcc/jit/libgccjit.c b/gcc/jit/libgccjit.c index 7fa948007ad..bd4ca6dc18f 100644 --- a/gcc/jit/libgccjit.c +++ b/gcc/jit/libgccjit.c @@ -1953,6 +1953,18 @@ 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_section method in jit-recording.c. */ +void +gcc_jit_lvalue_set_link_section (gcc_jit_lvalue *lvalue, + const char *name) +{ + RETURN_IF_FAIL (name, NULL, NULL, "NULL name"); + lvalue->set_link_section (name); +} + /* 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..21553ede3de 100644 --- a/gcc/jit/libgccjit.h +++ b/gcc/jit/libgccjit.h @@ -1072,6 +1072,19 @@ extern gcc_jit_rvalue * gcc_jit_lvalue_get_address (gcc_jit_lvalue *lvalue, gcc_jit_location *loc); +#define LIBGCCJIT_HAVE_gcc_jit_lvalue_set_link_section + +/* Set the link section of a global variable; analogous to: + __attribute__((section("section_name"))) + in C. + + This API entrypoint was added in LIBGCCJIT_ABI_18; you can test for its + presence using + #ifdef LIBGCCJIT_HAVE_gcc_jit_lvalue_set_tls_model */ +extern void +gcc_jit_lvalue_set_link_section (gcc_jit_lvalue *lvalue, + const char *name); + 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..9e722c2bde1 100644 --- a/gcc/jit/libgccjit.map +++ b/gcc/jit/libgccjit.map @@ -205,3 +205,14 @@ LIBGCCJIT_ABI_15 { gcc_jit_extended_asm_add_clobber; gcc_jit_context_add_top_level_asm; } LIBGCCJIT_ABI_14; + +LIBGCCJIT_ABI_16 { +} LIBGCCJIT_ABI_15; + +LIBGCCJIT_ABI_17 { +} LIBGCCJIT_ABI_16; + +LIBGCCJIT_ABI_18 { + global: + gcc_jit_lvalue_set_link_section; +} LIBGCCJIT_ABI_17; diff --git a/gcc/testsuite/jit.dg/all-non-failing-tests.h b/gcc/testsuite/jit.dg/all-non-failing-tests.h index 4202eb7798b..7e3b59dee0d 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-link-section.c */ +#define create_code create_code_link_section +#define verify_code verify_code_link_section +#include "test-link-section.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 diff --git a/gcc/testsuite/jit.dg/test-link-section.c b/gcc/testsuite/jit.dg/test-link-section.c new file mode 100644 index 00000000000..546c1e95b92 --- /dev/null +++ b/gcc/testsuite/jit.dg/test-link-section.c @@ -0,0 +1,25 @@ +#include <stdlib.h> +#include <stdio.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: + int foo __attribute__((section(".section"))); + */ + 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_link_section(foo, "section"); +} + +extern void +verify_code (gcc_jit_context *ctxt, gcc_jit_result *result) +{ +} -- 2.31.1 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] libgccjit: Add support for setting the link section of global variables [PR100688] 2021-05-20 0:32 [PATCH] libgccjit: Add support for setting the link section of global variables [PR100688] Antoni Boucher @ 2021-05-20 19:29 ` David Malcolm 2021-05-20 19:59 ` David Malcolm 2021-11-20 5:58 ` Antoni Boucher 0 siblings, 2 replies; 7+ messages in thread From: David Malcolm @ 2021-05-20 19:29 UTC (permalink / raw) To: Antoni Boucher, jit, gcc-patches On Wed, 2021-05-19 at 20:32 -0400, Antoni Boucher via Jit wrote: > Hello. > This patch adds support to set the link section of global variables. > I used the ABI 18 because I submitted other patches up to 17. > Thanks for the review. I didn't see this email until now, and put the review in bugzilla instead; sorry. Here's a copy-and-paste of what I put in bugzilla: Thanks for the patch; I like the idea; various nits below: > diff --git a/gcc/jit/docs/topics/expressions.rst b/gcc/jit/docs/topics/expressions.rst > index 396259ef07e..b39f6c02527 100644 > --- a/gcc/jit/docs/topics/expressions.rst > +++ b/gcc/jit/docs/topics/expressions.rst > @@ -539,6 +539,18 @@ where the rvalue is computed by reading from the storage area. > > in C. > > +.. function:: void > + gcc_jit_lvalue_set_link_section (gcc_jit_lvalue *lvalue, > + const char *name) > + > + Set the link section of a variable; analogous to: > + > + .. code-block:: c > + > + int variable __attribute__((section(".section"))); > + > + in C. Please rename param "name" to "section_name". Your implementation requires that it be non-NULL (rather than having NULL unset the section), so please specify that it must be non-NULL in the docs. Please add the usual "This entrypoint was added in" text to state which API version it was added in. > + > Global variables > **************** > > diff --git a/gcc/jit/jit-playback.h b/gcc/jit/jit-playback.h > index 825a3e172e9..8b0f65e87e8 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; > }; I think you only use this... > > @@ -670,6 +672,12 @@ public: > rvalue * > get_address (location *loc); > > + void > + set_link_section (const char* name) > + { > + set_decl_section_name (m_inner, name); > + } ...here, and you can get at rvalue::m_inner using as_tree (), so I don't think we need to make m_inner protected. > diff --git a/gcc/jit/jit-recording.c b/gcc/jit/jit-recording.c > index 117ff70114c..d54f878cc6b 100644 > --- a/gcc/jit/jit-recording.c > +++ b/gcc/jit/jit-recording.c > @@ -3713,6 +3713,11 @@ recording::lvalue::get_address (recording::location *loc) > return result; > } > > +void recording::lvalue::set_link_section (const char *name) > +{ > + m_link_section = new_string (name); > +} > + > /* The implementation of class gcc::jit::recording::param. */ > > /* Implementation of pure virtual hook recording::memento::replay_into > @@ -4547,8 +4552,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 +4564,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_link_section != NULL) > + { > + global->set_link_section(m_link_section->c_str()); > + } Coding convention nits: don't use {} when it's just one statement (which I think is a bad convention, but it is the project's convention). Missing spaces between function name and open-paren in both calls here. > + set_playback_obj (global); > } > [...snip....] > diff --git a/gcc/jit/jit-recording.h b/gcc/jit/jit-recording.h > index 03fa1160cf0..0691fac579d 100644 > --- a/gcc/jit/jit-recording.h > +++ b/gcc/jit/jit-recording.h > @@ -1105,7 +1105,8 @@ public: > lvalue (context *ctxt, > location *loc, > type *type_) > - : rvalue (ctxt, loc, type_) > + : rvalue (ctxt, loc, type_), > + m_link_section(NULL) > {} > > playback::lvalue * > @@ -1127,6 +1128,10 @@ public: > const char *access_as_rvalue (reproducer &r) OVERRIDE; > virtual const char *access_as_lvalue (reproducer &r); > virtual bool is_global () const { return false; } > + void set_link_section (const char *name); > + > +protected: > + string *m_link_section; > }; Can it be private, rather than protected? > diff --git a/gcc/jit/libgccjit.c b/gcc/jit/libgccjit.c > index 7fa948007ad..8cfa48aae24 100644 > --- a/gcc/jit/libgccjit.c > +++ b/gcc/jit/libgccjit.c > @@ -1953,6 +1953,18 @@ 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_section method in jit- recording.c. */ ^^^^^^^^^^^ set_link_section Also, a newline here please for consistency with the other entrypoints. > +void > +gcc_jit_lvalue_set_link_section (gcc_jit_lvalue *lvalue, > + const char *name) > +{ > + RETURN_IF_FAIL (name, NULL, NULL, "NULL name"); > + lvalue->set_link_section(name); Missing a space between function name and open-paren. > diff --git a/gcc/jit/libgccjit.h b/gcc/jit/libgccjit.h > index 5c722c2c57f..21553ede3de 100644 > --- a/gcc/jit/libgccjit.h > +++ b/gcc/jit/libgccjit.h > @@ -1072,6 +1072,19 @@ extern gcc_jit_rvalue * > gcc_jit_lvalue_get_address (gcc_jit_lvalue *lvalue, > gcc_jit_location *loc); > > +#define LIBGCCJIT_HAVE_gcc_jit_lvalue_set_link_section > + > +/* Set the link section of a global variable; analogous to: > + __attribute__((section("section_name"))) > + in C. > + > + This API entrypoint was added in LIBGCCJIT_ABI_18; you can test for its > + presence using > + #ifdef LIBGCCJIT_HAVE_gcc_jit_lvalue_set_tls_model */ Wrong #ifdef in the comment. > +extern void > +gcc_jit_lvalue_set_link_section (gcc_jit_lvalue *lvalue, > + const char *name); Rename param "name" to "section_name" to match the comment. > diff --git a/gcc/jit/libgccjit.map b/gcc/jit/libgccjit.map > index 337ea6c7fe4..9e722c2bde1 100644 > --- a/gcc/jit/libgccjit.map > +++ b/gcc/jit/libgccjit.map > @@ -205,3 +205,14 @@ LIBGCCJIT_ABI_15 { > gcc_jit_extended_asm_add_clobber; > gcc_jit_context_add_top_level_asm; > } LIBGCCJIT_ABI_14; > + > +LIBGCCJIT_ABI_16 { > +} LIBGCCJIT_ABI_15; > + > +LIBGCCJIT_ABI_17 { > +} LIBGCCJIT_ABI_16; > + > +LIBGCCJIT_ABI_18 { > + global: > + gcc_jit_lvalue_set_link_section; > +} LIBGCCJIT_ABI_17; I have some other patches of yours to review (presumably where the other ABI things are); sorry about that. I'll try to get to them today. > diff --git a/gcc/testsuite/jit.dg/all-non-failing-tests.h b/gcc/testsuite/jit.dg/all-non-failing-tests.h > index 4202eb7798b..7e3b59dee0d 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-link-section.c */ > +#define create_code create_code_link_section > +#define verify_code verify_code_link_section > +#include "test-link-section.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 > diff --git a/gcc/testsuite/jit.dg/test-link-section.c b/gcc/testsuite/jit.dg/test-link-section.c > new file mode 100644 > index 00000000000..546c1e95b92 > --- /dev/null > +++ b/gcc/testsuite/jit.dg/test-link-section.c > @@ -0,0 +1,25 @@ > +#include <stdlib.h> > +#include <stdio.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: > + int foo __attribute__((section(".section"))); > + */ > + 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_link_section(foo, "section"); > +} > + > +extern void > +verify_code (gcc_jit_context *ctxt, gcc_jit_result *result) > +{ > +} This is OK, but ideally it would test that the section name made it into the generated assembler. test-compile-to-assembler.c has a testcase for this which does something similar, with a DejaGnu directive looking for a substring in the generated asm if you want to attempt it. One other thing: the docs should make it clear about the leading ".". If I want to create the equivalent of: __attribute__((section(".section"))) do I call it with: gcc_jit_lvalue_set_link_section(foo, "section"); or with: gcc_jit_lvalue_set_link_section(foo, ".section"); It's a bit unclear to me from just reading the patch. The example suggests it's the former. In either case, the documentation should be clearer about this. Hope this is constructive Dave ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] libgccjit: Add support for setting the link section of global variables [PR100688] 2021-05-20 19:29 ` David Malcolm @ 2021-05-20 19:59 ` David Malcolm 2021-11-20 5:58 ` Antoni Boucher 1 sibling, 0 replies; 7+ messages in thread From: David Malcolm @ 2021-05-20 19:59 UTC (permalink / raw) To: Antoni Boucher, jit, gcc-patches On Thu, 2021-05-20 at 15:29 -0400, David Malcolm wrote: > On Wed, 2021-05-19 at 20:32 -0400, Antoni Boucher via Jit wrote: > > Hello. > > This patch adds support to set the link section of global > > variables. > > I used the ABI 18 because I submitted other patches up to 17. > > Thanks for the review. > > I didn't see this email until now, and put the review in bugzilla > instead; sorry. > > Here's a copy-and-paste of what I put in bugzilla: > [..snip...] > > > diff --git a/gcc/testsuite/jit.dg/all-non-failing-tests.h > b/gcc/testsuite/jit.dg/all-non-failing-tests.h > > index 4202eb7798b..7e3b59dee0d 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-link-section.c */ > > +#define create_code create_code_link_section > > +#define verify_code verify_code_link_section > > +#include "test-link-section.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 Something else I just noticed when looking at another of your patches: you also need to add a new entry to the "testcases" array to use the new {create|verify}_code_link_section functions. > [...snip...] Dave ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] libgccjit: Add support for setting the link section of global variables [PR100688] 2021-05-20 19:29 ` David Malcolm 2021-05-20 19:59 ` David Malcolm @ 2021-11-20 5:58 ` Antoni Boucher 2021-11-20 16:20 ` David Malcolm 1 sibling, 1 reply; 7+ messages in thread From: Antoni Boucher @ 2021-11-20 5:58 UTC (permalink / raw) To: David Malcolm, jit, gcc-patches [-- Attachment #1: Type: text/plain, Size: 11105 bytes --] Thanks for your reviews! Here's the updated patch, ready for another review. See comments/questions below. I'll update the other patches over the weekend. Le jeudi 20 mai 2021 à 15:29 -0400, David Malcolm a écrit : > On Wed, 2021-05-19 at 20:32 -0400, Antoni Boucher via Jit wrote: > > Hello. > > This patch adds support to set the link section of global > > variables. > > I used the ABI 18 because I submitted other patches up to 17. > > Thanks for the review. > > I didn't see this email until now, and put the review in bugzilla > instead; sorry. > > Here's a copy-and-paste of what I put in bugzilla: > > > Thanks for the patch; I like the idea; various nits below: > > > diff --git a/gcc/jit/docs/topics/expressions.rst > b/gcc/jit/docs/topics/expressions.rst > > index 396259ef07e..b39f6c02527 100644 > > --- a/gcc/jit/docs/topics/expressions.rst > > +++ b/gcc/jit/docs/topics/expressions.rst > > @@ -539,6 +539,18 @@ where the rvalue is computed by reading from > > the > storage area. > > > > in C. > > > > +.. function:: void > > + gcc_jit_lvalue_set_link_section (gcc_jit_lvalue > *lvalue, > > + const char *name) > > + > > + Set the link section of a variable; analogous to: > > + > > + .. code-block:: c > > + > > + int variable __attribute__((section(".section"))); > > + > > + in C. > > Please rename param "name" to "section_name". Your implementation > requires that it be non-NULL (rather than having NULL unset the > section), so please specify that it must be non-NULL in the docs. > > Please add the usual "This entrypoint was added in" text to state > which > API version it was added in. > > > + > > Global variables > > **************** > > > > diff --git a/gcc/jit/jit-playback.h b/gcc/jit/jit-playback.h > > index 825a3e172e9..8b0f65e87e8 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; > > }; > > I think you only use this... > > > > > @@ -670,6 +672,12 @@ public: > > rvalue * > > get_address (location *loc); > > > > + void > > + set_link_section (const char* name) > > + { > > + set_decl_section_name (m_inner, name); > > + } > > ...here, and you can get at rvalue::m_inner using as_tree (), so I > don't think we need to make m_inner protected. > > > diff --git a/gcc/jit/jit-recording.c b/gcc/jit/jit-recording.c > > index 117ff70114c..d54f878cc6b 100644 > > --- a/gcc/jit/jit-recording.c > > +++ b/gcc/jit/jit-recording.c > > @@ -3713,6 +3713,11 @@ recording::lvalue::get_address > (recording::location *loc) > > return result; > > } > > > > +void recording::lvalue::set_link_section (const char *name) > > +{ > > + m_link_section = new_string (name); > > +} > > + > > /* The implementation of class gcc::jit::recording::param. */ > > > > /* Implementation of pure virtual hook > recording::memento::replay_into > > @@ -4547,8 +4552,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 +4564,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_link_section != NULL) > > + { > > + global->set_link_section(m_link_section->c_str()); > > + } > > Coding convention nits: don't use {} when it's just one statement > (which I think is a bad convention, but it is the project's > convention). > Missing spaces between function name and open-paren in both calls > here. > > > > + set_playback_obj (global); > > } > > > > [...snip....] > > > diff --git a/gcc/jit/jit-recording.h b/gcc/jit/jit-recording.h > > index 03fa1160cf0..0691fac579d 100644 > > --- a/gcc/jit/jit-recording.h > > +++ b/gcc/jit/jit-recording.h > > @@ -1105,7 +1105,8 @@ public: > > lvalue (context *ctxt, > > location *loc, > > type *type_) > > - : rvalue (ctxt, loc, type_) > > + : rvalue (ctxt, loc, type_), > > + m_link_section(NULL) > > {} > > > > playback::lvalue * > > @@ -1127,6 +1128,10 @@ public: > > const char *access_as_rvalue (reproducer &r) OVERRIDE; > > virtual const char *access_as_lvalue (reproducer &r); > > virtual bool is_global () const { return false; } > > + void set_link_section (const char *name); > > + > > +protected: > > + string *m_link_section; > > }; > > Can it be private, rather than protected? m_link_section can't be private because it's used in recording::global::replay_into. > > > > diff --git a/gcc/jit/libgccjit.c b/gcc/jit/libgccjit.c > > index 7fa948007ad..8cfa48aae24 100644 > > --- a/gcc/jit/libgccjit.c > > +++ b/gcc/jit/libgccjit.c > > @@ -1953,6 +1953,18 @@ 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_section method in jit- > recording.c. */ > ^^^^^^^^^^^ > set_link_section > > Also, a newline here please for consistency with the other > entrypoints. Where should I add a newline? > > > +void > > +gcc_jit_lvalue_set_link_section (gcc_jit_lvalue *lvalue, > > + const char *name) > > +{ > > + RETURN_IF_FAIL (name, NULL, NULL, "NULL name"); > > + lvalue->set_link_section(name); > > Missing a space between function name and open-paren. > > > > diff --git a/gcc/jit/libgccjit.h b/gcc/jit/libgccjit.h > > index 5c722c2c57f..21553ede3de 100644 > > --- a/gcc/jit/libgccjit.h > > +++ b/gcc/jit/libgccjit.h > > @@ -1072,6 +1072,19 @@ extern gcc_jit_rvalue * > > gcc_jit_lvalue_get_address (gcc_jit_lvalue *lvalue, > > gcc_jit_location *loc); > > > > +#define LIBGCCJIT_HAVE_gcc_jit_lvalue_set_link_section > > + > > +/* Set the link section of a global variable; analogous to: > > + __attribute__((section("section_name"))) > > + in C. > > + > > + This API entrypoint was added in LIBGCCJIT_ABI_18; you can test > for its > > + presence using > > + #ifdef LIBGCCJIT_HAVE_gcc_jit_lvalue_set_tls_model */ > > Wrong #ifdef in the comment. > > > +extern void > > +gcc_jit_lvalue_set_link_section (gcc_jit_lvalue *lvalue, > > + const char *name); > > Rename param "name" to "section_name" to match the comment. > > > diff --git a/gcc/jit/libgccjit.map b/gcc/jit/libgccjit.map > > index 337ea6c7fe4..9e722c2bde1 100644 > > --- a/gcc/jit/libgccjit.map > > +++ b/gcc/jit/libgccjit.map > > @@ -205,3 +205,14 @@ LIBGCCJIT_ABI_15 { > > gcc_jit_extended_asm_add_clobber; > > gcc_jit_context_add_top_level_asm; > > } LIBGCCJIT_ABI_14; > > + > > +LIBGCCJIT_ABI_16 { > > +} LIBGCCJIT_ABI_15; > > + > > +LIBGCCJIT_ABI_17 { > > +} LIBGCCJIT_ABI_16; > > + > > +LIBGCCJIT_ABI_18 { > > + global: > > + gcc_jit_lvalue_set_link_section; > > +} LIBGCCJIT_ABI_17; > > I have some other patches of yours to review (presumably where the > other ABI things are); sorry about that. I'll try to get to them > today. > > > > diff --git a/gcc/testsuite/jit.dg/all-non-failing-tests.h > b/gcc/testsuite/jit.dg/all-non-failing-tests.h > > index 4202eb7798b..7e3b59dee0d 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-link-section.c */ > > +#define create_code create_code_link_section > > +#define verify_code verify_code_link_section > > +#include "test-link-section.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 > > diff --git a/gcc/testsuite/jit.dg/test-link-section.c > b/gcc/testsuite/jit.dg/test-link-section.c > > new file mode 100644 > > index 00000000000..546c1e95b92 > > --- /dev/null > > +++ b/gcc/testsuite/jit.dg/test-link-section.c > > @@ -0,0 +1,25 @@ > > +#include <stdlib.h> > > +#include <stdio.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: > > + int foo __attribute__((section(".section"))); > > + */ > > + 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_link_section(foo, "section"); > > +} > > + > > +extern void > > +verify_code (gcc_jit_context *ctxt, gcc_jit_result *result) > > +{ > > +} > > This is OK, but ideally it would test that the section name made it > into the generated assembler. test-compile-to-assembler.c has a > testcase for this which does something similar, with a DejaGnu > directive looking for a substring in the generated asm if you want to > attempt it. > > One other thing: the docs should make it clear about the leading ".". > > If I want to create the equivalent of: > > __attribute__((section(".section"))) > > do I call it with: > > gcc_jit_lvalue_set_link_section(foo, "section"); > > or with: > > gcc_jit_lvalue_set_link_section(foo, ".section"); > > It's a bit unclear to me from just reading the patch. The example > suggests it's the former. In either case, the documentation should > be > clearer about this. > > > Hope this is constructive > Dave > [-- Attachment #2: 0001-libgccjit-Add-support-for-setting-the-link-section-o.patch --] [-- Type: text/x-patch, Size: 11815 bytes --] From a454cb9ce14aa6903f7be9f2a56c35ab8251e678 Mon Sep 17 00:00:00 2001 From: Antoni Boucher <bouanto@zoho.com> Date: Wed, 12 May 2021 07:57:54 -0400 Subject: [PATCH] libgccjit: Add support for setting the link section of global variables [PR100688] 2021-11-20 Antoni Boucher <bouanto@zoho.com> gcc/jit/ PR target/100688 * docs/topics/compatibility.rst (LIBGCCJIT_ABI_18): New ABI tag. * docs/topics/expressions.rst: Add documentation for the function gcc_jit_lvalue_set_link_section. * jit-playback.h: New function (set_link_section) and rvalue::m_inner protected. * jit-recording.c: New function (set_link_section) and support for setting the link section. * jit-recording.h: New function (set_link_section) and new field m_link_section. * libgccjit.c: New function (gcc_jit_lvalue_set_link_section). * libgccjit.h: New function (gcc_jit_lvalue_set_link_section). * libgccjit.map (LIBGCCJIT_ABI_18): New ABI tag. gcc/testsuite/ PR target/100688 * jit.dg/all-non-failing-tests.h: Add test-link-section.c. * jit.dg/test-link_section.c: New test. * jit.dg/jit.exp: New helper function to test that the assembly contains a pattern. --- gcc/jit/docs/topics/compatibility.rst | 9 +++++ gcc/jit/docs/topics/expressions.rst | 21 +++++++++++ gcc/jit/jit-playback.h | 6 +++ gcc/jit/jit-recording.c | 35 +++++++++++++++--- gcc/jit/jit-recording.h | 7 +++- gcc/jit/libgccjit.c | 12 ++++++ gcc/jit/libgccjit.h | 14 +++++++ gcc/jit/libgccjit.map | 8 ++++ gcc/testsuite/jit.dg/jit.exp | 33 +++++++++++++++++ .../jit.dg/test-link-section-assembler.c | 37 +++++++++++++++++++ 10 files changed, 175 insertions(+), 7 deletions(-) create mode 100644 gcc/testsuite/jit.dg/test-link-section-assembler.c diff --git a/gcc/jit/docs/topics/compatibility.rst b/gcc/jit/docs/topics/compatibility.rst index 52ee3f860a7..b852f205fd7 100644 --- a/gcc/jit/docs/topics/compatibility.rst +++ b/gcc/jit/docs/topics/compatibility.rst @@ -284,3 +284,12 @@ entrypoints: * :func:`gcc_jit_struct_get_field` * :func:`gcc_jit_struct_get_field_count` + +.. _LIBGCCJIT_ABI_18: + +``LIBGCCJIT_ABI_18`` +----------------------- +``LIBGCCJIT_ABI_18`` covers the addition of an API entrypoint to set the link +section of a variable: + + * :func:`gcc_jit_lvalue_set_link_section` diff --git a/gcc/jit/docs/topics/expressions.rst b/gcc/jit/docs/topics/expressions.rst index 396259ef07e..02de531f31a 100644 --- a/gcc/jit/docs/topics/expressions.rst +++ b/gcc/jit/docs/topics/expressions.rst @@ -539,6 +539,27 @@ where the rvalue is computed by reading from the storage area. in C. +.. function:: void + gcc_jit_lvalue_set_link_section (gcc_jit_lvalue *lvalue, + const char *section_name) + + Set the link section of a variable. + The parameter ``section_name`` must be non-NULL and must contains the + leading dot. Analogous to: + + .. code-block:: c + + int variable __attribute__((section(".section"))); + + in C. + + This entrypoint was added in :ref:`LIBGCCJIT_ABI_18`; you can test for + its presence using + + .. code-block:: c + + #ifdef LIBGCCJIT_HAVE_gcc_jit_lvalue_set_link_section + Global variables **************** diff --git a/gcc/jit/jit-playback.h b/gcc/jit/jit-playback.h index f670c9e81df..15ff4f91eb7 100644 --- a/gcc/jit/jit-playback.h +++ b/gcc/jit/jit-playback.h @@ -675,6 +675,12 @@ public: rvalue * get_address (location *loc); + void + set_link_section (const char* name) + { + set_decl_section_name (as_tree (), name); + } + private: bool mark_addressable (location *loc); }; diff --git a/gcc/jit/jit-recording.c b/gcc/jit/jit-recording.c index 117ff70114c..2ad9e90e397 100644 --- a/gcc/jit/jit-recording.c +++ b/gcc/jit/jit-recording.c @@ -3713,6 +3713,11 @@ recording::lvalue::get_address (recording::location *loc) return result; } +void recording::lvalue::set_link_section (const char *name) +{ + m_link_section = new_string (name); +} + /* The implementation of class gcc::jit::recording::param. */ /* Implementation of pure virtual hook recording::memento::replay_into @@ -4547,20 +4552,30 @@ recording::block::dump_edges_to_dot (pretty_printer *pp) void recording::global::replay_into (replayer *r) { - set_playback_obj ( - m_initializer - ? r->new_global_initialized (playback_location (r, m_loc), + playback::lvalue *global; + if (m_initializer) + { + global = r->new_global_initialized (playback_location (r, m_loc), m_kind, m_type->playback_type (), m_type->dereference ()->get_size (), m_initializer_num_bytes / m_type->dereference ()->get_size (), m_initializer, - playback_string (m_name)) - : r->new_global (playback_location (r, m_loc), + playback_string (m_name)); + } + else + { + global = r->new_global (playback_location (r, m_loc), m_kind, m_type->playback_type (), - playback_string (m_name))); + playback_string (m_name)); + } + + if (m_link_section != NULL) + global->set_link_section (m_link_section->c_str ()); + + set_playback_obj (global); } /* Override the default implementation of @@ -4675,6 +4690,14 @@ recording::global::write_reproducer (reproducer &r) r.get_identifier_as_type (get_type ()), m_name->get_debug_string ()); + if (m_link_section != NULL) + { + r.write (" gcc_jit_lvalue_set_link_section (%s, /* gcc_jit_lvalue *lvalue */\n" + " \"%s\"); /* */\n", + id, + m_link_section->c_str ()); + } + if (m_initializer) switch (m_type->dereference ()->get_size ()) { diff --git a/gcc/jit/jit-recording.h b/gcc/jit/jit-recording.h index 4a994fe7094..2d08e043e10 100644 --- a/gcc/jit/jit-recording.h +++ b/gcc/jit/jit-recording.h @@ -1112,7 +1112,8 @@ public: lvalue (context *ctxt, location *loc, type *type_) - : rvalue (ctxt, loc, type_) + : rvalue (ctxt, loc, type_), + m_link_section (NULL) {} playback::lvalue * @@ -1134,6 +1135,10 @@ public: const char *access_as_rvalue (reproducer &r) OVERRIDE; virtual const char *access_as_lvalue (reproducer &r); virtual bool is_global () const { return false; } + void set_link_section (const char *name); + +protected: + string *m_link_section; }; class param : public lvalue diff --git a/gcc/jit/libgccjit.c b/gcc/jit/libgccjit.c index c744b634f4b..5b413d41e2c 100644 --- a/gcc/jit/libgccjit.c +++ b/gcc/jit/libgccjit.c @@ -2217,6 +2217,18 @@ 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_link_section method in jit-recording.c. */ +void +gcc_jit_lvalue_set_link_section (gcc_jit_lvalue *lvalue, + const char *section_name) +{ + RETURN_IF_FAIL (name, NULL, NULL, "NULL section_name"); + lvalue->set_link_section (name); +} + /* 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 a1c9436c545..646b5172b08 100644 --- a/gcc/jit/libgccjit.h +++ b/gcc/jit/libgccjit.h @@ -1089,6 +1089,20 @@ extern gcc_jit_rvalue * gcc_jit_lvalue_get_address (gcc_jit_lvalue *lvalue, gcc_jit_location *loc); +#define LIBGCCJIT_HAVE_gcc_jit_lvalue_set_link_section + +/* Set the link section of a global variable; analogous to: + __attribute__((section(".section_name"))) + in C. + + This API entrypoint was added in LIBGCCJIT_ABI_18; you can test for its + presence using + #ifdef LIBGCCJIT_HAVE_gcc_jit_lvalue_set_link_section +*/ +extern void +gcc_jit_lvalue_set_link_section (gcc_jit_lvalue *lvalue, + const char *section_name); + 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 64e790949e8..e5bdafd0156 100644 --- a/gcc/jit/libgccjit.map +++ b/gcc/jit/libgccjit.map @@ -226,3 +226,11 @@ LIBGCCJIT_ABI_16 { gcc_jit_type_is_struct; gcc_jit_struct_get_field_count; } LIBGCCJIT_ABI_15; + +LIBGCCJIT_ABI_17 { +} LIBGCCJIT_ABI_16; + +LIBGCCJIT_ABI_18 { + global: + gcc_jit_lvalue_set_link_section; +} LIBGCCJIT_ABI_17; diff --git a/gcc/testsuite/jit.dg/jit.exp b/gcc/testsuite/jit.dg/jit.exp index 10b98bdc74b..3568dbb9d63 100644 --- a/gcc/testsuite/jit.dg/jit.exp +++ b/gcc/testsuite/jit.dg/jit.exp @@ -864,6 +864,39 @@ proc jit-verify-assembler { args } { jit-run-executable ${executable_from_asm} ${dg-output-text} } +# Assuming that a .s file has been written out named +# OUTPUT_FILENAME, check that the argument matches the +# output file. +# For use by the test-link-section-assembler.c testcase. +proc jit-verify-assembler-output { args } { + verbose "jit-verify-assembler: $args" + + set dg-output-text [lindex $args 0] + verbose "dg-output-text: ${dg-output-text}" + + upvar 2 name name + verbose "name: $name" + + upvar 2 prog prog + verbose "prog: $prog" + set asm_filename [jit-get-output-filename $prog] + verbose " asm_filename: ${asm_filename}" + + # Read the assembly file. + set f [open $asm_filename r] + set content [read $f] + close $f + + # Verify that the assembly matches the regex. + if { ![regexp ${dg-output-text} $content] } { + fail "${asm_filename} output pattern test, is ${content}, should match ${dg-output-text}" + verbose "Failed test for output pattern ${dg-output-text}" 3 + } else { + pass "${asm_filename} output pattern test, ${dg-output-text}" + verbose "Passed test for output pattern ${dg-output-text}" 3 + } + +} # Assuming that a .o file has been written out named # OUTPUT_FILENAME, invoke the driver to try to turn it into # an executable, and try to run the result. diff --git a/gcc/testsuite/jit.dg/test-link-section-assembler.c b/gcc/testsuite/jit.dg/test-link-section-assembler.c new file mode 100644 index 00000000000..a90b00e9a82 --- /dev/null +++ b/gcc/testsuite/jit.dg/test-link-section-assembler.c @@ -0,0 +1,37 @@ +#include <stdlib.h> +#include <stdio.h> + +#include "libgccjit.h" + +#define TEST_COMPILING_TO_FILE +#define OUTPUT_KIND GCC_JIT_OUTPUT_KIND_ASSEMBLER +#define OUTPUT_FILENAME "output-of-test-link-section-assembler.c.s" +#include "harness.h" + +void +create_code (gcc_jit_context *ctxt, void *user_data) +{ + /* Let's try to inject the equivalent of: + int foo __attribute__((section(".section"))); + */ + 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_link_section(foo, ".my_section"); + + gcc_jit_function *func_main = + gcc_jit_context_new_function (ctxt, NULL, + GCC_JIT_FUNCTION_EXPORTED, + int_type, + "main", + 0, NULL, + 0); + gcc_jit_rvalue *zero = gcc_jit_context_zero (ctxt, int_type); + gcc_jit_block *block = gcc_jit_function_new_block (func_main, NULL); + gcc_jit_block_end_with_return (block, NULL, zero); +} + +/* { dg-final { jit-verify-output-file-was-created "" } } */ +/* { dg-final { jit-verify-assembler-output ".section .my_section" } } */ -- 2.26.2.7.g19db9cfb68.dirty ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] libgccjit: Add support for setting the link section of global variables [PR100688] 2021-11-20 5:58 ` Antoni Boucher @ 2021-11-20 16:20 ` David Malcolm 2021-11-20 16:53 ` Antoni Boucher 0 siblings, 1 reply; 7+ messages in thread From: David Malcolm @ 2021-11-20 16:20 UTC (permalink / raw) To: Antoni Boucher, jit, gcc-patches On Sat, 2021-11-20 at 00:58 -0500, Antoni Boucher wrote: > Thanks for your reviews! > > Here's the updated patch, ready for another review. > See comments/questions below. Thanks for the updated patch... > > I'll update the other patches over the weekend. > > Le jeudi 20 mai 2021 à 15:29 -0400, David Malcolm a écrit : > > On Wed, 2021-05-19 at 20:32 -0400, Antoni Boucher via Jit wrote: > > > Hello. > > > This patch adds support to set the link section of global > > > variables. > > > I used the ABI 18 because I submitted other patches up to 17. > > > Thanks for the review. > > > > I didn't see this email until now, and put the review in bugzilla > > instead; sorry. > > > > Here's a copy-and-paste of what I put in bugzilla: > > > > > > Thanks for the patch; I like the idea; various nits below: > > [...snip...] > > > > > Global variables > > > **************** > > > > > > diff --git a/gcc/jit/jit-playback.h b/gcc/jit/jit-playback.h > > > index 825a3e172e9..8b0f65e87e8 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; > > > }; > > > > I think you only use this... > > > > > > > > @@ -670,6 +672,12 @@ public: > > > rvalue * > > > get_address (location *loc); > > > > > > + void > > > + set_link_section (const char* name) > > > + { > > > + set_decl_section_name (m_inner, name); > > > + } > > > > ...here, and you can get at rvalue::m_inner using as_tree (), so I > > don't think we need to make m_inner protected. Thanks for dropping the "protected" in the updated patch; you need to update the ChangeLog entry. > > > > > > > + set_playback_obj (global); > > > } > > > > > > > [...snip....] > > > > > diff --git a/gcc/jit/jit-recording.h b/gcc/jit/jit-recording.h > > > index 03fa1160cf0..0691fac579d 100644 > > > --- a/gcc/jit/jit-recording.h > > > +++ b/gcc/jit/jit-recording.h > > > @@ -1105,7 +1105,8 @@ public: > > > lvalue (context *ctxt, > > > location *loc, > > > type *type_) > > > - : rvalue (ctxt, loc, type_) > > > + : rvalue (ctxt, loc, type_), > > > + m_link_section(NULL) > > > {} > > > > > > playback::lvalue * > > > @@ -1127,6 +1128,10 @@ public: > > > const char *access_as_rvalue (reproducer &r) OVERRIDE; > > > virtual const char *access_as_lvalue (reproducer &r); > > > virtual bool is_global () const { return false; } > > > + void set_link_section (const char *name); > > > + > > > +protected: > > > + string *m_link_section; > > > }; > > > > Can it be private, rather than protected? > > m_link_section can't be private because it's used in > recording::global::replay_into. Fair enough; thanks. > > > > > diff --git a/gcc/jit/libgccjit.c b/gcc/jit/libgccjit.c > > > index 7fa948007ad..8cfa48aae24 100644 > > > --- a/gcc/jit/libgccjit.c > > > +++ b/gcc/jit/libgccjit.c > > > @@ -1953,6 +1953,18 @@ 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_section method in jit- > > recording.c. */ > > ^^^^^^^^^^^ > > set_link_section > > > > Also, a newline here please for consistency with the other > > entrypoints. > > Where should I add a newline? Between the closing of the comment and the "void" of the fndecl (all the other entrypoints have a blank line separating them). Thanks for fixing my other nitpicks. [...snip...] > > > diff --git a/gcc/testsuite/jit.dg/all-non-failing-tests.h > > b/gcc/testsuite/jit.dg/all-non-failing-tests.h > > > index 4202eb7798b..7e3b59dee0d 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-link-section.c */ > > > +#define create_code create_code_link_section > > > +#define verify_code verify_code_link_section > > > +#include "test-link-section.c" > > > +#undef create_code > > > +#undef verify_code The updated version of the testcase doesn't have a verify_code, so it can't be in the "testcases" array. Please replace this with something like: /* test-link-section.c: This can't be in the testcases array as it doesn't have a verify_code implementation. */ (I'm trying to have all-nonfailing-tests.h refer to each non-failing test, either adding it to the testcases array, or documenting why it isn't in the array; listing them in alphabetical order) [...snip...] > > > > > > + > > > +extern void > > > +verify_code (gcc_jit_context *ctxt, gcc_jit_result *result) > > > +{ > > > +} > > > > This is OK, but ideally it would test that the section name made it > > into the generated assembler. test-compile-to-assembler.c has a > > testcase for this which does something similar, with a DejaGnu > > directive looking for a substring in the generated asm if you want to > > attempt it. Thanks for implementing the .exp directive. > > > > One other thing: the docs should make it clear about the leading ".". > > > > If I want to create the equivalent of: > > > > __attribute__((section(".section"))) > > > > do I call it with: > > > > gcc_jit_lvalue_set_link_section(foo, "section"); > > > > or with: > > > > gcc_jit_lvalue_set_link_section(foo, ".section"); > > > > It's a bit unclear to me from just reading the patch. The example > > suggests it's the former. In either case, the documentation should > > be > > clearer about this. Thanks for clarifying this in the docs. > Notes on the updated patch follow: > From a454cb9ce14aa6903f7be9f2a56c35ab8251e678 Mon Sep 17 00:00:00 2001 > From: Antoni Boucher <bouanto@zoho.com> > Date: Wed, 12 May 2021 07:57:54 -0400 > Subject: [PATCH] libgccjit: Add support for setting the link section of global > variables [PR100688] > > 2021-11-20 Antoni Boucher <bouanto@zoho.com> > > gcc/jit/ > PR target/100688 > * docs/topics/compatibility.rst (LIBGCCJIT_ABI_18): New ABI > tag. > * docs/topics/expressions.rst: Add documentation for the > function gcc_jit_lvalue_set_link_section. > * jit-playback.h: New function (set_link_section) and > rvalue::m_inner protected. As noted above, this part of the ChangeLog at least needs updating. > * jit-recording.c: New function (set_link_section) and > support for setting the link section. > * jit-recording.h: New function (set_link_section) and new > field m_link_section. > * libgccjit.c: New function (gcc_jit_lvalue_set_link_section). > * libgccjit.h: New function (gcc_jit_lvalue_set_link_section). > * libgccjit.map (LIBGCCJIT_ABI_18): New ABI tag. > > gcc/testsuite/ > PR target/100688 > * jit.dg/all-non-failing-tests.h: Add test-link-section.c. > * jit.dg/test-link_section.c: New test. > * jit.dg/jit.exp: New helper function to test that the > assembly contains a pattern. [...snip...] > diff --git a/gcc/jit/docs/topics/compatibility.rst b/gcc/jit/docs/topics/compatibility.rst > index 52ee3f860a7..b852f205fd7 100644 > --- a/gcc/jit/docs/topics/compatibility.rst > +++ b/gcc/jit/docs/topics/compatibility.rst > @@ -284,3 +284,12 @@ entrypoints: > * :func:`gcc_jit_struct_get_field` > > * :func:`gcc_jit_struct_get_field_count` > + > +.. _LIBGCCJIT_ABI_18: > + > +``LIBGCCJIT_ABI_18`` > +----------------------- > +``LIBGCCJIT_ABI_18`` covers the addition of an API entrypoint to set the link > +section of a variable: > + > + * :func:`gcc_jit_lvalue_set_link_section` As noted in earlier review, presumably there's a different patch that adds ABI_17? I can potentially approve this link section patch now, but that other patch would need to be pushed first. [...snip...] > @@ -4547,20 +4552,30 @@ recording::block::dump_edges_to_dot (pretty_printer *pp) > void > recording::global::replay_into (replayer *r) > { > - set_playback_obj ( > - m_initializer > - ? r->new_global_initialized (playback_location (r, m_loc), > + playback::lvalue *global; > + if (m_initializer) > + { > + global = r->new_global_initialized (playback_location (r, m_loc), > m_kind, > m_type->playback_type (), > m_type->dereference ()->get_size (), > m_initializer_num_bytes > / m_type->dereference ()->get_size (), > m_initializer, > - playback_string (m_name)) > - : r->new_global (playback_location (r, m_loc), > + playback_string (m_name)); > + } > + else > + { > + global = r->new_global (playback_location (r, m_loc), > m_kind, > m_type->playback_type (), > - playback_string (m_name))); > + playback_string (m_name)); > + } > + > + if (m_link_section != NULL) > + global->set_link_section (m_link_section->c_str ()); > + > + set_playback_obj (global); > } Looks like the patch as posted has some interaction with the initializers patch, presumably that's the ABI_17? But presumably the intent here is this hunk: + if (m_link_section != NULL) + global->set_link_section (m_link_section->c_str ()); and that indeed looks good. > diff --git a/gcc/jit/libgccjit.c b/gcc/jit/libgccjit.c > index c744b634f4b..5b413d41e2c 100644 > --- a/gcc/jit/libgccjit.c > +++ b/gcc/jit/libgccjit.c > @@ -2217,6 +2217,18 @@ 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_link_section method in jit-recording.c. */ > +void > +gcc_jit_lvalue_set_link_section (gcc_jit_lvalue *lvalue, > + const char *section_name) > +{ > + RETURN_IF_FAIL (name, NULL, NULL, "NULL section_name"); > + lvalue->set_link_section (name); > +} Thanks for renaming the param from "name" to "section_name", but it looks like you didn't rename the uses of it within the function - I'm guessing this code as-is doesn't compile. [...snip...] > diff --git a/gcc/jit/libgccjit.map b/gcc/jit/libgccjit.map > index 64e790949e8..e5bdafd0156 100644 > --- a/gcc/jit/libgccjit.map > +++ b/gcc/jit/libgccjit.map > @@ -226,3 +226,11 @@ LIBGCCJIT_ABI_16 { > gcc_jit_type_is_struct; > gcc_jit_struct_get_field_count; > } LIBGCCJIT_ABI_15; > + > +LIBGCCJIT_ABI_17 { > +} LIBGCCJIT_ABI_16; > + Same notes here as above. [...snip...] Overall the patch looks good, my comments are all just nit-picks at this point, except that obviously you need to finish updating the symbol name to "section_name" in gcc_jit_lvalue_set_link_section so that it compiles. Please check it compiles and that the testsuite passes before pushing - though I think this is waiting on another patch, right? This is OK for trunk, once those fixes and prerequisites are taken care of. Thanks again Dave ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] libgccjit: Add support for setting the link section of global variables [PR100688] 2021-11-20 16:20 ` David Malcolm @ 2021-11-20 16:53 ` Antoni Boucher 2021-11-20 18:27 ` David Malcolm 0 siblings, 1 reply; 7+ messages in thread From: Antoni Boucher @ 2021-11-20 16:53 UTC (permalink / raw) To: David Malcolm, jit, gcc-patches [-- Attachment #1: Type: text/plain, Size: 14370 bytes --] Hi. Here's the updated patch. See comments below. Thanks for the review! Le samedi 20 novembre 2021 à 11:20 -0500, David Malcolm a écrit : > On Sat, 2021-11-20 at 00:58 -0500, Antoni Boucher wrote: > > Thanks for your reviews! > > > > Here's the updated patch, ready for another review. > > See comments/questions below. > > Thanks for the updated patch... > > > > > I'll update the other patches over the weekend. > > > > Le jeudi 20 mai 2021 à 15:29 -0400, David Malcolm a écrit : > > > On Wed, 2021-05-19 at 20:32 -0400, Antoni Boucher via Jit wrote: > > > > Hello. > > > > This patch adds support to set the link section of global > > > > variables. > > > > I used the ABI 18 because I submitted other patches up to 17. > > > > Thanks for the review. > > > > > > I didn't see this email until now, and put the review in bugzilla > > > instead; sorry. > > > > > > Here's a copy-and-paste of what I put in bugzilla: > > > > > > > > > Thanks for the patch; I like the idea; various nits below: > > > > > [...snip...] > > > > > > > > Global variables > > > > **************** > > > > > > > > diff --git a/gcc/jit/jit-playback.h b/gcc/jit/jit-playback.h > > > > index 825a3e172e9..8b0f65e87e8 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; > > > > }; > > > > > > I think you only use this... > > > > > > > > > > > @@ -670,6 +672,12 @@ public: > > > > rvalue * > > > > get_address (location *loc); > > > > > > > > + void > > > > + set_link_section (const char* name) > > > > + { > > > > + set_decl_section_name (m_inner, name); > > > > + } > > > > > > ...here, and you can get at rvalue::m_inner using as_tree (), so > > > I > > > don't think we need to make m_inner protected. > > Thanks for dropping the "protected" in the updated patch; you need to > update the ChangeLog entry. > > > > > > > > > > > + set_playback_obj (global); > > > > } > > > > > > > > > > [...snip....] > > > > > > > diff --git a/gcc/jit/jit-recording.h b/gcc/jit/jit-recording.h > > > > index 03fa1160cf0..0691fac579d 100644 > > > > --- a/gcc/jit/jit-recording.h > > > > +++ b/gcc/jit/jit-recording.h > > > > @@ -1105,7 +1105,8 @@ public: > > > > lvalue (context *ctxt, > > > > location *loc, > > > > type *type_) > > > > - : rvalue (ctxt, loc, type_) > > > > + : rvalue (ctxt, loc, type_), > > > > + m_link_section(NULL) > > > > {} > > > > > > > > playback::lvalue * > > > > @@ -1127,6 +1128,10 @@ public: > > > > const char *access_as_rvalue (reproducer &r) OVERRIDE; > > > > virtual const char *access_as_lvalue (reproducer &r); > > > > virtual bool is_global () const { return false; } > > > > + void set_link_section (const char *name); > > > > + > > > > +protected: > > > > + string *m_link_section; > > > > }; > > > > > > Can it be private, rather than protected? > > > > m_link_section can't be private because it's used in > > recording::global::replay_into. > > Fair enough; thanks. > > > > > > > > diff --git a/gcc/jit/libgccjit.c b/gcc/jit/libgccjit.c > > > > index 7fa948007ad..8cfa48aae24 100644 > > > > --- a/gcc/jit/libgccjit.c > > > > +++ b/gcc/jit/libgccjit.c > > > > @@ -1953,6 +1953,18 @@ 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_section method in jit- > > > recording.c. */ > > > ^^^^^^^^^^^ > > > set_link_section > > > > > > Also, a newline here please for consistency with the other > > > entrypoints. > > > > Where should I add a newline? > > Between the closing of the comment and the "void" of the fndecl (all > the other entrypoints have a blank line separating them). Thanks for > fixing my other nitpicks. > > > [...snip...] > > > > > diff --git a/gcc/testsuite/jit.dg/all-non-failing-tests.h > > > b/gcc/testsuite/jit.dg/all-non-failing-tests.h > > > > index 4202eb7798b..7e3b59dee0d 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-link-section.c */ > > > > +#define create_code create_code_link_section > > > > +#define verify_code verify_code_link_section > > > > +#include "test-link-section.c" > > > > +#undef create_code > > > > +#undef verify_code > > The updated version of the testcase doesn't have a verify_code, so it > can't be in the "testcases" array. > Please replace this with something like: > > > /* test-link-section.c: This can't be in the testcases array as it > doesn't have a verify_code implementation. */ > > > (I'm trying to have all-nonfailing-tests.h refer to each non-failing > test, either adding it to the testcases array, or documenting why it > isn't in the array; listing them in alphabetical order) Hum, that code seems to refer to the previous patch. I removed that part and changed the test to test-link-section- assembler.c which checks that the assembly contains the link section. As far as I know, it does not need to be listed here to be ran. > > [...snip...] > > > > > > > > > + > > > > +extern void > > > > +verify_code (gcc_jit_context *ctxt, gcc_jit_result *result) > > > > +{ > > > > +} > > > > > > This is OK, but ideally it would test that the section name made > > > it > > > into the generated assembler. test-compile-to-assembler.c has a > > > testcase for this which does something similar, with a DejaGnu > > > directive looking for a substring in the generated asm if you > > > want to > > > attempt it. > > Thanks for implementing the .exp directive. > > > > > > > One other thing: the docs should make it clear about the leading > > > ".". > > > > > > If I want to create the equivalent of: > > > > > > __attribute__((section(".section"))) > > > > > > do I call it with: > > > > > > gcc_jit_lvalue_set_link_section(foo, "section"); > > > > > > or with: > > > > > > gcc_jit_lvalue_set_link_section(foo, ".section"); > > > > > > It's a bit unclear to me from just reading the patch. The > > > example > > > suggests it's the former. In either case, the documentation > > > should > > > be > > > clearer about this. > > Thanks for clarifying this in the docs. > > > > Notes on the updated patch follow: > > > > From a454cb9ce14aa6903f7be9f2a56c35ab8251e678 Mon Sep 17 00:00:00 > > 2001 > > From: Antoni Boucher <bouanto@zoho.com> > > Date: Wed, 12 May 2021 07:57:54 -0400 > > Subject: [PATCH] libgccjit: Add support for setting the link > > section of global > > variables [PR100688] > > > > 2021-11-20 Antoni Boucher <bouanto@zoho.com> > > > > gcc/jit/ > > PR target/100688 > > * docs/topics/compatibility.rst (LIBGCCJIT_ABI_18): New > > ABI > > tag. > > * docs/topics/expressions.rst: Add documentation for > > the > > function gcc_jit_lvalue_set_link_section. > > * jit-playback.h: New function (set_link_section) and > > rvalue::m_inner protected. > > As noted above, this part of the ChangeLog at least needs updating. > > > * jit-recording.c: New function (set_link_section) and > > support for setting the link section. > > * jit-recording.h: New function (set_link_section) and > > new > > field m_link_section. > > * libgccjit.c: New function > > (gcc_jit_lvalue_set_link_section). > > * libgccjit.h: New function > > (gcc_jit_lvalue_set_link_section). > > * libgccjit.map (LIBGCCJIT_ABI_18): New ABI tag. > > > > gcc/testsuite/ > > PR target/100688 > > * jit.dg/all-non-failing-tests.h: Add test-link- > > section.c. > > * jit.dg/test-link_section.c: New test. > > * jit.dg/jit.exp: New helper function to test that the > > assembly contains a pattern. > > [...snip...] > > > diff --git a/gcc/jit/docs/topics/compatibility.rst > > b/gcc/jit/docs/topics/compatibility.rst > > index 52ee3f860a7..b852f205fd7 100644 > > --- a/gcc/jit/docs/topics/compatibility.rst > > +++ b/gcc/jit/docs/topics/compatibility.rst > > @@ -284,3 +284,12 @@ entrypoints: > > * :func:`gcc_jit_struct_get_field` > > > > * :func:`gcc_jit_struct_get_field_count` > > + > > +.. _LIBGCCJIT_ABI_18: > > + > > +``LIBGCCJIT_ABI_18`` > > +----------------------- > > +``LIBGCCJIT_ABI_18`` covers the addition of an API entrypoint to > > set the link > > +section of a variable: > > + > > + * :func:`gcc_jit_lvalue_set_link_section` > > As noted in earlier review, presumably there's a different patch that > adds ABI_17? > > I can potentially approve this link section patch now, but that other > patch would need to be pushed first. Yes, there's a patch before this one that will need to be reviewed and merged first. > > [...snip...] > > > @@ -4547,20 +4552,30 @@ recording::block::dump_edges_to_dot > > (pretty_printer *pp) > > void > > recording::global::replay_into (replayer *r) > > { > > - set_playback_obj ( > > - m_initializer > > - ? r->new_global_initialized (playback_location (r, m_loc), > > + playback::lvalue *global; > > + if (m_initializer) > > + { > > + global = r->new_global_initialized (playback_location (r, > > m_loc), > > m_kind, > > m_type->playback_type (), > > m_type->dereference ()->get_size > > (), > > m_initializer_num_bytes > > / m_type->dereference ()->get_size > > (), > > m_initializer, > > - playback_string (m_name)) > > - : r->new_global (playback_location (r, m_loc), > > + playback_string (m_name)); > > + } > > + else > > + { > > + global = r->new_global (playback_location (r, m_loc), > > m_kind, > > m_type->playback_type (), > > - playback_string (m_name))); > > + playback_string (m_name)); > > + } > > + > > + if (m_link_section != NULL) > > + global->set_link_section (m_link_section->c_str ()); > > + > > + set_playback_obj (global); > > } > > Looks like the patch as posted has some interaction with the > initializers patch, presumably that's the ABI_17? Yes, but that's gonna be in a future patch that is not ABI 17. Since I need to update that code to have the variable global to call set_link_section, I'll let this code as is if this is OK. > > But presumably the intent here is this hunk: > > + if (m_link_section != NULL) > + global->set_link_section (m_link_section->c_str ()); > > and that indeed looks good. > > > > diff --git a/gcc/jit/libgccjit.c b/gcc/jit/libgccjit.c > > index c744b634f4b..5b413d41e2c 100644 > > --- a/gcc/jit/libgccjit.c > > +++ b/gcc/jit/libgccjit.c > > @@ -2217,6 +2217,18 @@ 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_link_section method in jit- > > recording.c. */ > > +void > > +gcc_jit_lvalue_set_link_section (gcc_jit_lvalue *lvalue, > > + const char *section_name) > > +{ > > + RETURN_IF_FAIL (name, NULL, NULL, "NULL section_name"); > > + lvalue->set_link_section (name); > > +} > > Thanks for renaming the param from "name" to "section_name", but it > looks like you didn't rename the uses of it within the function - I'm > guessing this code as-is doesn't compile. > > [...snip...] > > > diff --git a/gcc/jit/libgccjit.map b/gcc/jit/libgccjit.map > > index 64e790949e8..e5bdafd0156 100644 > > --- a/gcc/jit/libgccjit.map > > +++ b/gcc/jit/libgccjit.map > > @@ -226,3 +226,11 @@ LIBGCCJIT_ABI_16 { > > gcc_jit_type_is_struct; > > gcc_jit_struct_get_field_count; > > } LIBGCCJIT_ABI_15; > > + > > +LIBGCCJIT_ABI_17 { > > +} LIBGCCJIT_ABI_16; > > + > > Same notes here as above. > > [...snip...] > > Overall the patch looks good, my comments are all just nit-picks at > this point, except that obviously you need to finish updating the > symbol name to "section_name" in gcc_jit_lvalue_set_link_section so > that it compiles. Please check it compiles and that the testsuite > passes before pushing - though I think this is waiting on another > patch, right? This is OK for trunk, once those fixes and > prerequisites > are taken care of. > > Thanks again > Dave > [-- Attachment #2: 0001-libgccjit-Add-support-for-setting-the-link-section-o.patch --] [-- Type: text/x-patch, Size: 11731 bytes --] From d334fbaca8fa40bebcef88cba2f05329d677c69c Mon Sep 17 00:00:00 2001 From: Antoni Boucher <bouanto@zoho.com> Date: Wed, 12 May 2021 07:57:54 -0400 Subject: [PATCH] libgccjit: Add support for setting the link section of global variables [PR100688] 2021-11-20 Antoni Boucher <bouanto@zoho.com> gcc/jit/ PR target/100688 * docs/topics/compatibility.rst (LIBGCCJIT_ABI_18): New ABI tag. * docs/topics/expressions.rst: Add documentation for the function gcc_jit_lvalue_set_link_section. * jit-playback.h: New function (set_link_section). * jit-recording.c: New function (set_link_section) and support for setting the link section. * jit-recording.h: New function (set_link_section) and new field m_link_section. * libgccjit.c: New function (gcc_jit_lvalue_set_link_section). * libgccjit.h: New function (gcc_jit_lvalue_set_link_section). * libgccjit.map (LIBGCCJIT_ABI_18): New ABI tag. gcc/testsuite/ PR target/100688 * jit.dg/test-link-section-assembler.c: New test. * jit.dg/jit.exp: New helper function to test that the assembly contains a pattern. --- gcc/jit/docs/topics/compatibility.rst | 9 +++++ gcc/jit/docs/topics/expressions.rst | 21 +++++++++++ gcc/jit/jit-playback.h | 6 +++ gcc/jit/jit-recording.c | 35 +++++++++++++++--- gcc/jit/jit-recording.h | 7 +++- gcc/jit/libgccjit.c | 13 +++++++ gcc/jit/libgccjit.h | 14 +++++++ gcc/jit/libgccjit.map | 8 ++++ gcc/testsuite/jit.dg/jit.exp | 33 +++++++++++++++++ .../jit.dg/test-link-section-assembler.c | 37 +++++++++++++++++++ 10 files changed, 176 insertions(+), 7 deletions(-) create mode 100644 gcc/testsuite/jit.dg/test-link-section-assembler.c diff --git a/gcc/jit/docs/topics/compatibility.rst b/gcc/jit/docs/topics/compatibility.rst index 52ee3f860a7..b852f205fd7 100644 --- a/gcc/jit/docs/topics/compatibility.rst +++ b/gcc/jit/docs/topics/compatibility.rst @@ -284,3 +284,12 @@ entrypoints: * :func:`gcc_jit_struct_get_field` * :func:`gcc_jit_struct_get_field_count` + +.. _LIBGCCJIT_ABI_18: + +``LIBGCCJIT_ABI_18`` +----------------------- +``LIBGCCJIT_ABI_18`` covers the addition of an API entrypoint to set the link +section of a variable: + + * :func:`gcc_jit_lvalue_set_link_section` diff --git a/gcc/jit/docs/topics/expressions.rst b/gcc/jit/docs/topics/expressions.rst index 396259ef07e..02de531f31a 100644 --- a/gcc/jit/docs/topics/expressions.rst +++ b/gcc/jit/docs/topics/expressions.rst @@ -539,6 +539,27 @@ where the rvalue is computed by reading from the storage area. in C. +.. function:: void + gcc_jit_lvalue_set_link_section (gcc_jit_lvalue *lvalue, + const char *section_name) + + Set the link section of a variable. + The parameter ``section_name`` must be non-NULL and must contains the + leading dot. Analogous to: + + .. code-block:: c + + int variable __attribute__((section(".section"))); + + in C. + + This entrypoint was added in :ref:`LIBGCCJIT_ABI_18`; you can test for + its presence using + + .. code-block:: c + + #ifdef LIBGCCJIT_HAVE_gcc_jit_lvalue_set_link_section + Global variables **************** diff --git a/gcc/jit/jit-playback.h b/gcc/jit/jit-playback.h index f670c9e81df..15ff4f91eb7 100644 --- a/gcc/jit/jit-playback.h +++ b/gcc/jit/jit-playback.h @@ -675,6 +675,12 @@ public: rvalue * get_address (location *loc); + void + set_link_section (const char* name) + { + set_decl_section_name (as_tree (), name); + } + private: bool mark_addressable (location *loc); }; diff --git a/gcc/jit/jit-recording.c b/gcc/jit/jit-recording.c index 117ff70114c..2ad9e90e397 100644 --- a/gcc/jit/jit-recording.c +++ b/gcc/jit/jit-recording.c @@ -3713,6 +3713,11 @@ recording::lvalue::get_address (recording::location *loc) return result; } +void recording::lvalue::set_link_section (const char *name) +{ + m_link_section = new_string (name); +} + /* The implementation of class gcc::jit::recording::param. */ /* Implementation of pure virtual hook recording::memento::replay_into @@ -4547,20 +4552,30 @@ recording::block::dump_edges_to_dot (pretty_printer *pp) void recording::global::replay_into (replayer *r) { - set_playback_obj ( - m_initializer - ? r->new_global_initialized (playback_location (r, m_loc), + playback::lvalue *global; + if (m_initializer) + { + global = r->new_global_initialized (playback_location (r, m_loc), m_kind, m_type->playback_type (), m_type->dereference ()->get_size (), m_initializer_num_bytes / m_type->dereference ()->get_size (), m_initializer, - playback_string (m_name)) - : r->new_global (playback_location (r, m_loc), + playback_string (m_name)); + } + else + { + global = r->new_global (playback_location (r, m_loc), m_kind, m_type->playback_type (), - playback_string (m_name))); + playback_string (m_name)); + } + + if (m_link_section != NULL) + global->set_link_section (m_link_section->c_str ()); + + set_playback_obj (global); } /* Override the default implementation of @@ -4675,6 +4690,14 @@ recording::global::write_reproducer (reproducer &r) r.get_identifier_as_type (get_type ()), m_name->get_debug_string ()); + if (m_link_section != NULL) + { + r.write (" gcc_jit_lvalue_set_link_section (%s, /* gcc_jit_lvalue *lvalue */\n" + " \"%s\"); /* */\n", + id, + m_link_section->c_str ()); + } + if (m_initializer) switch (m_type->dereference ()->get_size ()) { diff --git a/gcc/jit/jit-recording.h b/gcc/jit/jit-recording.h index 4a994fe7094..2d08e043e10 100644 --- a/gcc/jit/jit-recording.h +++ b/gcc/jit/jit-recording.h @@ -1112,7 +1112,8 @@ public: lvalue (context *ctxt, location *loc, type *type_) - : rvalue (ctxt, loc, type_) + : rvalue (ctxt, loc, type_), + m_link_section (NULL) {} playback::lvalue * @@ -1134,6 +1135,10 @@ public: const char *access_as_rvalue (reproducer &r) OVERRIDE; virtual const char *access_as_lvalue (reproducer &r); virtual bool is_global () const { return false; } + void set_link_section (const char *name); + +protected: + string *m_link_section; }; class param : public lvalue diff --git a/gcc/jit/libgccjit.c b/gcc/jit/libgccjit.c index c744b634f4b..7da1fab7438 100644 --- a/gcc/jit/libgccjit.c +++ b/gcc/jit/libgccjit.c @@ -2217,6 +2217,19 @@ 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_link_section method in jit-recording.c. */ + +void +gcc_jit_lvalue_set_link_section (gcc_jit_lvalue *lvalue, + const char *section_name) +{ + RETURN_IF_FAIL (section_name, NULL, NULL, "NULL section_name"); + lvalue->set_link_section (section_name); +} + /* 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 a1c9436c545..646b5172b08 100644 --- a/gcc/jit/libgccjit.h +++ b/gcc/jit/libgccjit.h @@ -1089,6 +1089,20 @@ extern gcc_jit_rvalue * gcc_jit_lvalue_get_address (gcc_jit_lvalue *lvalue, gcc_jit_location *loc); +#define LIBGCCJIT_HAVE_gcc_jit_lvalue_set_link_section + +/* Set the link section of a global variable; analogous to: + __attribute__((section(".section_name"))) + in C. + + This API entrypoint was added in LIBGCCJIT_ABI_18; you can test for its + presence using + #ifdef LIBGCCJIT_HAVE_gcc_jit_lvalue_set_link_section +*/ +extern void +gcc_jit_lvalue_set_link_section (gcc_jit_lvalue *lvalue, + const char *section_name); + 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 64e790949e8..e5bdafd0156 100644 --- a/gcc/jit/libgccjit.map +++ b/gcc/jit/libgccjit.map @@ -226,3 +226,11 @@ LIBGCCJIT_ABI_16 { gcc_jit_type_is_struct; gcc_jit_struct_get_field_count; } LIBGCCJIT_ABI_15; + +LIBGCCJIT_ABI_17 { +} LIBGCCJIT_ABI_16; + +LIBGCCJIT_ABI_18 { + global: + gcc_jit_lvalue_set_link_section; +} LIBGCCJIT_ABI_17; diff --git a/gcc/testsuite/jit.dg/jit.exp b/gcc/testsuite/jit.dg/jit.exp index 10b98bdc74b..3568dbb9d63 100644 --- a/gcc/testsuite/jit.dg/jit.exp +++ b/gcc/testsuite/jit.dg/jit.exp @@ -864,6 +864,39 @@ proc jit-verify-assembler { args } { jit-run-executable ${executable_from_asm} ${dg-output-text} } +# Assuming that a .s file has been written out named +# OUTPUT_FILENAME, check that the argument matches the +# output file. +# For use by the test-link-section-assembler.c testcase. +proc jit-verify-assembler-output { args } { + verbose "jit-verify-assembler: $args" + + set dg-output-text [lindex $args 0] + verbose "dg-output-text: ${dg-output-text}" + + upvar 2 name name + verbose "name: $name" + + upvar 2 prog prog + verbose "prog: $prog" + set asm_filename [jit-get-output-filename $prog] + verbose " asm_filename: ${asm_filename}" + + # Read the assembly file. + set f [open $asm_filename r] + set content [read $f] + close $f + + # Verify that the assembly matches the regex. + if { ![regexp ${dg-output-text} $content] } { + fail "${asm_filename} output pattern test, is ${content}, should match ${dg-output-text}" + verbose "Failed test for output pattern ${dg-output-text}" 3 + } else { + pass "${asm_filename} output pattern test, ${dg-output-text}" + verbose "Passed test for output pattern ${dg-output-text}" 3 + } + +} # Assuming that a .o file has been written out named # OUTPUT_FILENAME, invoke the driver to try to turn it into # an executable, and try to run the result. diff --git a/gcc/testsuite/jit.dg/test-link-section-assembler.c b/gcc/testsuite/jit.dg/test-link-section-assembler.c new file mode 100644 index 00000000000..a90b00e9a82 --- /dev/null +++ b/gcc/testsuite/jit.dg/test-link-section-assembler.c @@ -0,0 +1,37 @@ +#include <stdlib.h> +#include <stdio.h> + +#include "libgccjit.h" + +#define TEST_COMPILING_TO_FILE +#define OUTPUT_KIND GCC_JIT_OUTPUT_KIND_ASSEMBLER +#define OUTPUT_FILENAME "output-of-test-link-section-assembler.c.s" +#include "harness.h" + +void +create_code (gcc_jit_context *ctxt, void *user_data) +{ + /* Let's try to inject the equivalent of: + int foo __attribute__((section(".section"))); + */ + 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_link_section(foo, ".my_section"); + + gcc_jit_function *func_main = + gcc_jit_context_new_function (ctxt, NULL, + GCC_JIT_FUNCTION_EXPORTED, + int_type, + "main", + 0, NULL, + 0); + gcc_jit_rvalue *zero = gcc_jit_context_zero (ctxt, int_type); + gcc_jit_block *block = gcc_jit_function_new_block (func_main, NULL); + gcc_jit_block_end_with_return (block, NULL, zero); +} + +/* { dg-final { jit-verify-output-file-was-created "" } } */ +/* { dg-final { jit-verify-assembler-output ".section .my_section" } } */ -- 2.26.2.7.g19db9cfb68.dirty ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] libgccjit: Add support for setting the link section of global variables [PR100688] 2021-11-20 16:53 ` Antoni Boucher @ 2021-11-20 18:27 ` David Malcolm 0 siblings, 0 replies; 7+ messages in thread From: David Malcolm @ 2021-11-20 18:27 UTC (permalink / raw) To: Antoni Boucher, jit, gcc-patches On Sat, 2021-11-20 at 11:53 -0500, Antoni Boucher wrote: > Hi. > Here's the updated patch. > See comments below. > Thanks for the review! > > Le samedi 20 novembre 2021 à 11:20 -0500, David Malcolm a écrit : > > On Sat, 2021-11-20 at 00:58 -0500, Antoni Boucher wrote: [...snip...] > > > > > > > > > diff --git a/gcc/testsuite/jit.dg/all-non-failing-tests.h > > > > b/gcc/testsuite/jit.dg/all-non-failing-tests.h > > > > > index 4202eb7798b..7e3b59dee0d 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-link-section.c */ > > > > > +#define create_code create_code_link_section > > > > > +#define verify_code verify_code_link_section > > > > > +#include "test-link-section.c" > > > > > +#undef create_code > > > > > +#undef verify_code > > > > The updated version of the testcase doesn't have a verify_code, so it > > can't be in the "testcases" array. > > Please replace this with something like: > > > > > > /* test-link-section.c: This can't be in the testcases array as it > > doesn't have a verify_code implementation. */ > > > > > > (I'm trying to have all-nonfailing-tests.h refer to each non-failing > > test, either adding it to the testcases array, or documenting why it > > isn't in the array; listing them in alphabetical order) > > Hum, that code seems to refer to the previous patch. > I removed that part and changed the test to test-link-section- > assembler.c which checks that the assembly contains the link section. > As far as I know, it does not need to be listed here to be ran. Right, it's just a comment. It's not to be run here, I'm just trying to document those tests that aren't being as part of the threads/combination tests, since it's so easy to accidentally omit them when adding a new test - ideally all-non-failing-tests.h will mention all of the tests that don't have "error" in their titles, either adding them to the "testcases" array, or documenting why they're not in the array, with a comment like the one I suggested above (feel free to directly use that). [...snip...] > > > > Overall the patch looks good, my comments are all just nit-picks at > > this point, except that obviously you need to finish updating the > > symbol name to "section_name" in gcc_jit_lvalue_set_link_section so > > that it compiles. Please check it compiles and that the testsuite > > passes before pushing - though I think this is waiting on another > > patch, right? This is OK for trunk, once those fixes and > > prerequisites > > are taken care of. > > > > Thanks again > > Dave Looks good for trunk, once the earlier patch is in. As before, please do a test build and run the testsuite before pushing. Thanks for the updated patch. Dave ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-11-20 18:27 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-05-20 0:32 [PATCH] libgccjit: Add support for setting the link section of global variables [PR100688] Antoni Boucher 2021-05-20 19:29 ` David Malcolm 2021-05-20 19:59 ` David Malcolm 2021-11-20 5:58 ` Antoni Boucher 2021-11-20 16:20 ` David Malcolm 2021-11-20 16:53 ` Antoni Boucher 2021-11-20 18:27 ` David Malcolm
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).