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
>