public inbox for jit@gcc.gnu.org
 help / color / mirror / Atom feed
* [PATCH] libgccjit: Add support for setting the link section of global variables [PR100688]
@ 2021-05-20  0:32 Antoni Boucher
  2021-05-20 19:29 ` David Malcolm
  0 siblings, 1 reply; 7+ messages in thread
From: Antoni Boucher @ 2021-05-20  0:32 UTC (permalink / raw)
  To: jit, gcc-patches

[-- Attachment #1: Type: text/plain, Size: 161 bytes --]

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.

[-- Attachment #2: 0001-Add-support-for-setting-the-link-section-of-global-v.patch --]
[-- Type: text/x-patch, Size: 9729 bytes --]

From c867732ee36003759d479497c85135ecfc4a0cf3 Mon Sep 17 00:00:00 2001
From: Antoni Boucher <bouanto@zoho.com>
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  <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.
            * 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 (m_link_section != NULL)
+  {
+    global->set_link_section (m_link_section->c_str ());
+  }
+  set_playback_obj (global);
 }
 
 /* Override the default implementation of
@@ -4675,6 +4684,14 @@ recording::global::write_reproducer (reproducer &r)
     r.get_identifier_as_type (get_type ()),
     m_name->get_debug_string ());
 
+  if (m_link_section != NULL)
+  {
+    r.write ("  gcc_jit_lvalue_set_link_section (%s, /* gcc_jit_lvalue *lvalue */\n"
+         "                                  \"%s\"); /* */\n",
+     id,
+     m_link_section->c_str ());
+  }
+
   if (m_initializer)
     switch (m_type->dereference ()->get_size ())
       {
diff --git a/gcc/jit/jit-recording.h b/gcc/jit/jit-recording.h
index 03fa1160cf0..8ca82c928b8 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;
 };
 
 class param : public lvalue
diff --git a/gcc/jit/libgccjit.c b/gcc/jit/libgccjit.c
index 7fa948007ad..bd4ca6dc18f 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.  */
+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);
+}
+
 /* Public entrypoint.  See description in libgccjit.h.
 
    After error-checking, the real work is done by the
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  */
+extern void
+gcc_jit_lvalue_set_link_section (gcc_jit_lvalue *lvalue,
+			    const char *name);
+
 extern gcc_jit_lvalue *
 gcc_jit_function_new_local (gcc_jit_function *func,
 			    gcc_jit_location *loc,
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;
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)
+{
+}
-- 
2.31.1


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] libgccjit: Add support for setting the link section of global variables [PR100688]
  2021-05-20  0:32 [PATCH] libgccjit: Add support for setting the link section of global variables [PR100688] Antoni Boucher
@ 2021-05-20 19:29 ` David Malcolm
  2021-05-20 19:59   ` David Malcolm
  2021-11-20  5:58   ` Antoni Boucher
  0 siblings, 2 replies; 7+ messages in thread
From: David Malcolm @ 2021-05-20 19:29 UTC (permalink / raw)
  To: Antoni Boucher, jit, gcc-patches

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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] libgccjit: Add support for setting the link section of global variables [PR100688]
  2021-05-20 19:29 ` David Malcolm
@ 2021-05-20 19:59   ` David Malcolm
  2021-11-20  5:58   ` Antoni Boucher
  1 sibling, 0 replies; 7+ messages in thread
From: David Malcolm @ 2021-05-20 19:59 UTC (permalink / raw)
  To: Antoni Boucher, jit, gcc-patches

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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] libgccjit: Add support for setting the link section of global variables [PR100688]
  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
  1 sibling, 1 reply; 7+ messages in thread
From: Antoni Boucher @ 2021-11-20  5:58 UTC (permalink / raw)
  To: David Malcolm, jit, gcc-patches

[-- Attachment #1: Type: text/plain, Size: 11105 bytes --]

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
> 


[-- Attachment #2: 0001-libgccjit-Add-support-for-setting-the-link-section-o.patch --]
[-- Type: text/x-patch, Size: 11815 bytes --]

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.
            * 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.
---
 gcc/jit/docs/topics/compatibility.rst         |  9 +++++
 gcc/jit/docs/topics/expressions.rst           | 21 +++++++++++
 gcc/jit/jit-playback.h                        |  6 +++
 gcc/jit/jit-recording.c                       | 35 +++++++++++++++---
 gcc/jit/jit-recording.h                       |  7 +++-
 gcc/jit/libgccjit.c                           | 12 ++++++
 gcc/jit/libgccjit.h                           | 14 +++++++
 gcc/jit/libgccjit.map                         |  8 ++++
 gcc/testsuite/jit.dg/jit.exp                  | 33 +++++++++++++++++
 .../jit.dg/test-link-section-assembler.c      | 37 +++++++++++++++++++
 10 files changed, 175 insertions(+), 7 deletions(-)
 create mode 100644 gcc/testsuite/jit.dg/test-link-section-assembler.c

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`
diff --git a/gcc/jit/docs/topics/expressions.rst b/gcc/jit/docs/topics/expressions.rst
index 396259ef07e..02de531f31a 100644
--- a/gcc/jit/docs/topics/expressions.rst
+++ b/gcc/jit/docs/topics/expressions.rst
@@ -539,6 +539,27 @@ 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 *section_name)
+
+   Set the link section of a variable.
+   The parameter ``section_name`` must be non-NULL and must contains the
+   leading dot. Analogous to:
+
+   .. code-block:: c
+
+     int variable __attribute__((section(".section")));
+
+   in C.
+
+   This entrypoint was added in :ref:`LIBGCCJIT_ABI_18`; you can test for
+   its presence using
+
+   .. code-block:: c
+
+      #ifdef LIBGCCJIT_HAVE_gcc_jit_lvalue_set_link_section
+
 Global variables
 ****************
 
diff --git a/gcc/jit/jit-playback.h b/gcc/jit/jit-playback.h
index f670c9e81df..15ff4f91eb7 100644
--- a/gcc/jit/jit-playback.h
+++ b/gcc/jit/jit-playback.h
@@ -675,6 +675,12 @@ public:
   rvalue *
   get_address (location *loc);
 
+  void
+  set_link_section (const char* name)
+  {
+    set_decl_section_name (as_tree (), name);
+  }
+
 private:
   bool mark_addressable (location *loc);
 };
diff --git a/gcc/jit/jit-recording.c b/gcc/jit/jit-recording.c
index 117ff70114c..2ad9e90e397 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,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);
 }
 
 /* Override the default implementation of
@@ -4675,6 +4690,14 @@ recording::global::write_reproducer (reproducer &r)
     r.get_identifier_as_type (get_type ()),
     m_name->get_debug_string ());
 
+  if (m_link_section != NULL)
+  {
+    r.write ("  gcc_jit_lvalue_set_link_section (%s, /* gcc_jit_lvalue *lvalue */\n"
+	"                                  \"%s\"); /* */\n",
+     id,
+     m_link_section->c_str ());
+  }
+
   if (m_initializer)
     switch (m_type->dereference ()->get_size ())
       {
diff --git a/gcc/jit/jit-recording.h b/gcc/jit/jit-recording.h
index 4a994fe7094..2d08e043e10 100644
--- a/gcc/jit/jit-recording.h
+++ b/gcc/jit/jit-recording.h
@@ -1112,7 +1112,8 @@ public:
   lvalue (context *ctxt,
 	  location *loc,
 	  type *type_)
-    : rvalue (ctxt, loc, type_)
+    : rvalue (ctxt, loc, type_),
+      m_link_section (NULL)
     {}
 
   playback::lvalue *
@@ -1134,6 +1135,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;
 };
 
 class param : public lvalue
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);
+}
+
 /* Public entrypoint.  See description in libgccjit.h.
 
    After error-checking, the real work is done by the
diff --git a/gcc/jit/libgccjit.h b/gcc/jit/libgccjit.h
index a1c9436c545..646b5172b08 100644
--- a/gcc/jit/libgccjit.h
+++ b/gcc/jit/libgccjit.h
@@ -1089,6 +1089,20 @@ 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_link_section
+*/
+extern void
+gcc_jit_lvalue_set_link_section (gcc_jit_lvalue *lvalue,
+			    const char *section_name);
+
 extern gcc_jit_lvalue *
 gcc_jit_function_new_local (gcc_jit_function *func,
 			    gcc_jit_location *loc,
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;
+
+LIBGCCJIT_ABI_18 {
+  global:
+    gcc_jit_lvalue_set_link_section;
+} LIBGCCJIT_ABI_17;
diff --git a/gcc/testsuite/jit.dg/jit.exp b/gcc/testsuite/jit.dg/jit.exp
index 10b98bdc74b..3568dbb9d63 100644
--- a/gcc/testsuite/jit.dg/jit.exp
+++ b/gcc/testsuite/jit.dg/jit.exp
@@ -864,6 +864,39 @@ proc jit-verify-assembler { args } {
     jit-run-executable ${executable_from_asm} ${dg-output-text}
 }
 
+# Assuming that a .s file has been written out named
+# OUTPUT_FILENAME, check that the argument matches the
+# output file.
+# For use by the test-link-section-assembler.c testcase.
+proc jit-verify-assembler-output { args } {
+    verbose "jit-verify-assembler: $args"
+
+    set dg-output-text [lindex $args 0]
+    verbose "dg-output-text: ${dg-output-text}"
+
+    upvar 2 name name
+    verbose "name: $name"
+
+    upvar 2 prog prog
+    verbose "prog: $prog"
+    set asm_filename [jit-get-output-filename $prog]
+    verbose "  asm_filename: ${asm_filename}"
+
+    # Read the assembly file.
+    set f [open $asm_filename r]
+    set content [read $f]
+    close $f
+
+    # Verify that the assembly matches the regex.
+    if { ![regexp ${dg-output-text} $content] } {
+	fail "${asm_filename} output pattern test, is ${content}, should match ${dg-output-text}"
+	verbose "Failed test for output pattern ${dg-output-text}" 3
+    } else {
+	pass "${asm_filename} output pattern test, ${dg-output-text}"
+	verbose "Passed test for output pattern ${dg-output-text}" 3
+    }
+
+}
 # Assuming that a .o file has been written out named
 # OUTPUT_FILENAME, invoke the driver to try to turn it into
 # an executable, and try to run the result.
diff --git a/gcc/testsuite/jit.dg/test-link-section-assembler.c b/gcc/testsuite/jit.dg/test-link-section-assembler.c
new file mode 100644
index 00000000000..a90b00e9a82
--- /dev/null
+++ b/gcc/testsuite/jit.dg/test-link-section-assembler.c
@@ -0,0 +1,37 @@
+#include <stdlib.h>
+#include <stdio.h>
+
+#include "libgccjit.h"
+
+#define TEST_COMPILING_TO_FILE
+#define OUTPUT_KIND      GCC_JIT_OUTPUT_KIND_ASSEMBLER
+#define OUTPUT_FILENAME  "output-of-test-link-section-assembler.c.s"
+#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, ".my_section");
+
+  gcc_jit_function *func_main =
+    gcc_jit_context_new_function (ctxt, NULL,
+				  GCC_JIT_FUNCTION_EXPORTED,
+				  int_type,
+				  "main",
+				  0, NULL,
+				  0);
+  gcc_jit_rvalue *zero = gcc_jit_context_zero (ctxt, int_type);
+  gcc_jit_block *block = gcc_jit_function_new_block (func_main, NULL);
+  gcc_jit_block_end_with_return (block, NULL, zero);
+}
+
+/* { dg-final { jit-verify-output-file-was-created "" } } */
+/* { dg-final { jit-verify-assembler-output ".section	.my_section" } } */
-- 
2.26.2.7.g19db9cfb68.dirty


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] libgccjit: Add support for setting the link section of global variables [PR100688]
  2021-11-20  5:58   ` Antoni Boucher
@ 2021-11-20 16:20     ` David Malcolm
  2021-11-20 16:53       ` Antoni Boucher
  0 siblings, 1 reply; 7+ messages in thread
From: David Malcolm @ 2021-11-20 16:20 UTC (permalink / raw)
  To: Antoni Boucher, jit, gcc-patches

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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] libgccjit: Add support for setting the link section of global variables [PR100688]
  2021-11-20 16:20     ` David Malcolm
@ 2021-11-20 16:53       ` Antoni Boucher
  2021-11-20 18:27         ` David Malcolm
  0 siblings, 1 reply; 7+ messages in thread
From: Antoni Boucher @ 2021-11-20 16:53 UTC (permalink / raw)
  To: David Malcolm, jit, gcc-patches

[-- Attachment #1: Type: text/plain, Size: 14370 bytes --]

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
> 


[-- Attachment #2: 0001-libgccjit-Add-support-for-setting-the-link-section-o.patch --]
[-- Type: text/x-patch, Size: 11731 bytes --]

From d334fbaca8fa40bebcef88cba2f05329d677c69c 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).
            * 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/test-link-section-assembler.c: New test.
            * jit.dg/jit.exp: New helper function to test that the
            assembly contains a pattern.
---
 gcc/jit/docs/topics/compatibility.rst         |  9 +++++
 gcc/jit/docs/topics/expressions.rst           | 21 +++++++++++
 gcc/jit/jit-playback.h                        |  6 +++
 gcc/jit/jit-recording.c                       | 35 +++++++++++++++---
 gcc/jit/jit-recording.h                       |  7 +++-
 gcc/jit/libgccjit.c                           | 13 +++++++
 gcc/jit/libgccjit.h                           | 14 +++++++
 gcc/jit/libgccjit.map                         |  8 ++++
 gcc/testsuite/jit.dg/jit.exp                  | 33 +++++++++++++++++
 .../jit.dg/test-link-section-assembler.c      | 37 +++++++++++++++++++
 10 files changed, 176 insertions(+), 7 deletions(-)
 create mode 100644 gcc/testsuite/jit.dg/test-link-section-assembler.c

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`
diff --git a/gcc/jit/docs/topics/expressions.rst b/gcc/jit/docs/topics/expressions.rst
index 396259ef07e..02de531f31a 100644
--- a/gcc/jit/docs/topics/expressions.rst
+++ b/gcc/jit/docs/topics/expressions.rst
@@ -539,6 +539,27 @@ 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 *section_name)
+
+   Set the link section of a variable.
+   The parameter ``section_name`` must be non-NULL and must contains the
+   leading dot. Analogous to:
+
+   .. code-block:: c
+
+     int variable __attribute__((section(".section")));
+
+   in C.
+
+   This entrypoint was added in :ref:`LIBGCCJIT_ABI_18`; you can test for
+   its presence using
+
+   .. code-block:: c
+
+      #ifdef LIBGCCJIT_HAVE_gcc_jit_lvalue_set_link_section
+
 Global variables
 ****************
 
diff --git a/gcc/jit/jit-playback.h b/gcc/jit/jit-playback.h
index f670c9e81df..15ff4f91eb7 100644
--- a/gcc/jit/jit-playback.h
+++ b/gcc/jit/jit-playback.h
@@ -675,6 +675,12 @@ public:
   rvalue *
   get_address (location *loc);
 
+  void
+  set_link_section (const char* name)
+  {
+    set_decl_section_name (as_tree (), name);
+  }
+
 private:
   bool mark_addressable (location *loc);
 };
diff --git a/gcc/jit/jit-recording.c b/gcc/jit/jit-recording.c
index 117ff70114c..2ad9e90e397 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,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);
 }
 
 /* Override the default implementation of
@@ -4675,6 +4690,14 @@ recording::global::write_reproducer (reproducer &r)
     r.get_identifier_as_type (get_type ()),
     m_name->get_debug_string ());
 
+  if (m_link_section != NULL)
+  {
+    r.write ("  gcc_jit_lvalue_set_link_section (%s, /* gcc_jit_lvalue *lvalue */\n"
+	"                                  \"%s\"); /* */\n",
+     id,
+     m_link_section->c_str ());
+  }
+
   if (m_initializer)
     switch (m_type->dereference ()->get_size ())
       {
diff --git a/gcc/jit/jit-recording.h b/gcc/jit/jit-recording.h
index 4a994fe7094..2d08e043e10 100644
--- a/gcc/jit/jit-recording.h
+++ b/gcc/jit/jit-recording.h
@@ -1112,7 +1112,8 @@ public:
   lvalue (context *ctxt,
 	  location *loc,
 	  type *type_)
-    : rvalue (ctxt, loc, type_)
+    : rvalue (ctxt, loc, type_),
+      m_link_section (NULL)
     {}
 
   playback::lvalue *
@@ -1134,6 +1135,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;
 };
 
 class param : public lvalue
diff --git a/gcc/jit/libgccjit.c b/gcc/jit/libgccjit.c
index c744b634f4b..7da1fab7438 100644
--- a/gcc/jit/libgccjit.c
+++ b/gcc/jit/libgccjit.c
@@ -2217,6 +2217,19 @@ 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 (section_name, NULL, NULL, "NULL section_name");
+  lvalue->set_link_section (section_name);
+}
+
 /* Public entrypoint.  See description in libgccjit.h.
 
    After error-checking, the real work is done by the
diff --git a/gcc/jit/libgccjit.h b/gcc/jit/libgccjit.h
index a1c9436c545..646b5172b08 100644
--- a/gcc/jit/libgccjit.h
+++ b/gcc/jit/libgccjit.h
@@ -1089,6 +1089,20 @@ 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_link_section
+*/
+extern void
+gcc_jit_lvalue_set_link_section (gcc_jit_lvalue *lvalue,
+			    const char *section_name);
+
 extern gcc_jit_lvalue *
 gcc_jit_function_new_local (gcc_jit_function *func,
 			    gcc_jit_location *loc,
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;
+
+LIBGCCJIT_ABI_18 {
+  global:
+    gcc_jit_lvalue_set_link_section;
+} LIBGCCJIT_ABI_17;
diff --git a/gcc/testsuite/jit.dg/jit.exp b/gcc/testsuite/jit.dg/jit.exp
index 10b98bdc74b..3568dbb9d63 100644
--- a/gcc/testsuite/jit.dg/jit.exp
+++ b/gcc/testsuite/jit.dg/jit.exp
@@ -864,6 +864,39 @@ proc jit-verify-assembler { args } {
     jit-run-executable ${executable_from_asm} ${dg-output-text}
 }
 
+# Assuming that a .s file has been written out named
+# OUTPUT_FILENAME, check that the argument matches the
+# output file.
+# For use by the test-link-section-assembler.c testcase.
+proc jit-verify-assembler-output { args } {
+    verbose "jit-verify-assembler: $args"
+
+    set dg-output-text [lindex $args 0]
+    verbose "dg-output-text: ${dg-output-text}"
+
+    upvar 2 name name
+    verbose "name: $name"
+
+    upvar 2 prog prog
+    verbose "prog: $prog"
+    set asm_filename [jit-get-output-filename $prog]
+    verbose "  asm_filename: ${asm_filename}"
+
+    # Read the assembly file.
+    set f [open $asm_filename r]
+    set content [read $f]
+    close $f
+
+    # Verify that the assembly matches the regex.
+    if { ![regexp ${dg-output-text} $content] } {
+	fail "${asm_filename} output pattern test, is ${content}, should match ${dg-output-text}"
+	verbose "Failed test for output pattern ${dg-output-text}" 3
+    } else {
+	pass "${asm_filename} output pattern test, ${dg-output-text}"
+	verbose "Passed test for output pattern ${dg-output-text}" 3
+    }
+
+}
 # Assuming that a .o file has been written out named
 # OUTPUT_FILENAME, invoke the driver to try to turn it into
 # an executable, and try to run the result.
diff --git a/gcc/testsuite/jit.dg/test-link-section-assembler.c b/gcc/testsuite/jit.dg/test-link-section-assembler.c
new file mode 100644
index 00000000000..a90b00e9a82
--- /dev/null
+++ b/gcc/testsuite/jit.dg/test-link-section-assembler.c
@@ -0,0 +1,37 @@
+#include <stdlib.h>
+#include <stdio.h>
+
+#include "libgccjit.h"
+
+#define TEST_COMPILING_TO_FILE
+#define OUTPUT_KIND      GCC_JIT_OUTPUT_KIND_ASSEMBLER
+#define OUTPUT_FILENAME  "output-of-test-link-section-assembler.c.s"
+#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, ".my_section");
+
+  gcc_jit_function *func_main =
+    gcc_jit_context_new_function (ctxt, NULL,
+				  GCC_JIT_FUNCTION_EXPORTED,
+				  int_type,
+				  "main",
+				  0, NULL,
+				  0);
+  gcc_jit_rvalue *zero = gcc_jit_context_zero (ctxt, int_type);
+  gcc_jit_block *block = gcc_jit_function_new_block (func_main, NULL);
+  gcc_jit_block_end_with_return (block, NULL, zero);
+}
+
+/* { dg-final { jit-verify-output-file-was-created "" } } */
+/* { dg-final { jit-verify-assembler-output ".section	.my_section" } } */
-- 
2.26.2.7.g19db9cfb68.dirty


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] libgccjit: Add support for setting the link section of global variables [PR100688]
  2021-11-20 16:53       ` Antoni Boucher
@ 2021-11-20 18:27         ` David Malcolm
  0 siblings, 0 replies; 7+ messages in thread
From: David Malcolm @ 2021-11-20 18:27 UTC (permalink / raw)
  To: Antoni Boucher, jit, gcc-patches

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


^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2021-11-20 18:27 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-20  0:32 [PATCH] libgccjit: Add support for setting the link section of global variables [PR100688] 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
2021-11-20 16:53       ` Antoni Boucher
2021-11-20 18:27         ` David Malcolm

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).