Re: [PATCH] libgccjit: Add support for setting the link section of global variables [PR100688]
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
Re: [PATCH] libgccjit: Add support for setting the link section of global variables [PR100688]
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 ) OVERRIDE;
> > > > virtual const char *access_as_lvalue (reproducer );
> > > > 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
Re: [PATCH] libgccjit: Add support for setting the link section of global variables [PR100688]
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 ) OVERRIDE;
> > > virtual const char *access_as_lvalue (reproducer );
> > > 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
Re: [PATCH] libgccjit: Add support for setting the link section of global variables [PR100688]
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 ) OVERRIDE;
> > virtual const char *access_as_lvalue (reproducer );
> > 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?
Re: [PATCH] libgccjit: Add support for setting the link section of global variables [PR100688]
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
Re: [PATCH] libgccjit: Add support for setting the link section of global variables [PR100688]
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 ) OVERRIDE;
>virtual const char *access_as_lvalue (reproducer );
>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. */
^^^
[PATCH] libgccjit: Add support for setting the link section of global variables [PR100688]
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.
From c867732ee36003759d479497c85135ecfc4a0cf3 Mon Sep 17 00:00:00 2001
From: Antoni Boucher
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
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
