From: David Malcolm <dmalcolm@redhat.com>
To: Antoni Boucher <bouanto@zoho.com>,
jit@gcc.gnu.org, gcc-patches@gcc.gnu.org
Subject: Re: [PATCH] libgccjit: Add support for setting the link section of global variables [PR100688]
Date: Sat, 20 Nov 2021 11:20:54 -0500 [thread overview]
Message-ID: <7530c6bca7aab2a29303957512be97cf090e159d.camel@redhat.com> (raw)
In-Reply-To: <e70ebba3fa4a6f27f560c9d05ba7cf76dd56838d.camel@zoho.com>
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
next prev parent reply other threads:[~2021-11-20 16:21 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-05-20 0:32 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 [this message]
2021-11-20 16:53 ` Antoni Boucher
2021-11-20 18:27 ` David Malcolm
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=7530c6bca7aab2a29303957512be97cf090e159d.camel@redhat.com \
--to=dmalcolm@redhat.com \
--cc=bouanto@zoho.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=jit@gcc.gnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).