Re: [PATCH] libgccjit: Add support for setting the link section of global variables [PR100688]

[David Malcolm <dmalcolm@redhat.com>]

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