[PATCH] libgccjit: Add support for setting the link section of global variables [PR100688]
Antoni Boucher
bouanto@zoho.com
Sat Nov 20 05:58:26 GMT 2021
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
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-libgccjit-Add-support-for-setting-the-link-section-o.patch
Type: text/x-patch
Size: 11815 bytes
Desc: not available
URL: <https://gcc.gnu.org/pipermail/jit/attachments/20211120/d7120afc/attachment-0001.bin>
More information about the Jit
mailing list