libgccjit: Add gcc_jit_global_set_readonly

Message ID 20a1db244d8234683b61c7d124999ba45462af90.camel@zoho.com
State Unresolved
Headers
Series libgccjit: Add gcc_jit_global_set_readonly |

Checks

Context Check Description
snail/gcc-patch-check warning Git am fail log

Commit Message

Antoni Boucher Jan. 19, 2024, 9:57 p.m. UTC
  Hi.
This patch adds a new API gcc_jit_global_set_readonly: it's equivalent
to having a const global variable, but it is useful in the case of
complex compilers where it is not convenient to use const.
Thanks for the review.
  

Comments

David Malcolm Jan. 24, 2024, 3:09 p.m. UTC | #1
On Fri, 2024-01-19 at 16:57 -0500, Antoni Boucher wrote:
> Hi.
> This patch adds a new API gcc_jit_global_set_readonly: it's
> equivalent
> to having a const global variable, but it is useful in the case of
> complex compilers where it is not convenient to use const.
> Thanks for the review.

Hi Antoni; thanks for the patch.

Can you give an example of where/why this might be used?
Presumably this is motivated by a use case you had inside the rustc
backend?

Thanks
Dave
  
Antoni Boucher Jan. 24, 2024, 3:36 p.m. UTC | #2
Yes, it is for a use case inside of rustc_codegen_gcc.
The compiler is structured in a way where we don't know if a global
variable might be constant when it is created.

On Wed, 2024-01-24 at 10:09 -0500, David Malcolm wrote:
> On Fri, 2024-01-19 at 16:57 -0500, Antoni Boucher wrote:
> > Hi.
> > This patch adds a new API gcc_jit_global_set_readonly: it's
> > equivalent
> > to having a const global variable, but it is useful in the case of
> > complex compilers where it is not convenient to use const.
> > Thanks for the review.
> 
> Hi Antoni; thanks for the patch.
> 
> Can you give an example of where/why this might be used?
> Presumably this is motivated by a use case you had inside the rustc
> backend?
> 
> Thanks
> Dave
>
  
Antoni Boucher Feb. 17, 2024, 4:56 p.m. UTC | #3
David: Ping.

On Wed, 2024-01-24 at 10:36 -0500, Antoni Boucher wrote:
> Yes, it is for a use case inside of rustc_codegen_gcc.
> The compiler is structured in a way where we don't know if a global
> variable might be constant when it is created.
> 
> On Wed, 2024-01-24 at 10:09 -0500, David Malcolm wrote:
> > On Fri, 2024-01-19 at 16:57 -0500, Antoni Boucher wrote:
> > > Hi.
> > > This patch adds a new API gcc_jit_global_set_readonly: it's
> > > equivalent
> > > to having a const global variable, but it is useful in the case
> > > of
> > > complex compilers where it is not convenient to use const.
> > > Thanks for the review.
> > 
> > Hi Antoni; thanks for the patch.
> > 
> > Can you give an example of where/why this might be used?
> > Presumably this is motivated by a use case you had inside the rustc
> > backend?
> > 
> > Thanks
> > Dave
> > 
>
  

Patch

From ff3aa19207a6cdaeff6fcb6521ad2ad92f5448ff Mon Sep 17 00:00:00 2001
From: Antoni Boucher <bouanto@zoho.com>
Date: Tue, 24 May 2022 17:45:01 -0400
Subject: [PATCH] libgccjit: Add gcc_jit_global_set_readonly

gcc/jit/ChangeLog:

	* docs/topics/compatibility.rst (LIBGCCJIT_ABI_26): New ABI tag.
	* docs/topics/expressions.rst: Document gcc_jit_global_set_readonly.
	* jit-playback.cc (global_new_decl, new_global,
	new_global_initialized): New parameter readonly.
	* jit-playback.h (global_new_decl, new_global,
	new_global_initialized): New parameter readonly.
	* jit-recording.cc (recording::global::replay_into): Use
	m_readonly.
	(recording::global::write_reproducer): Dump reproducer for
	gcc_jit_global_set_readonly.
	* jit-recording.h (get_readonly, set_readonly): New methods.
	(m_readonly): New attribute.
	* libgccjit.cc (gcc_jit_global_set_readonly): New function.
	(gcc_jit_block_add_assignment): Check that we don't assign to a
	readonly variable.
	* libgccjit.h (gcc_jit_global_set_readonly): New function.
	(LIBGCCJIT_HAVE_gcc_jit_global_set_readonly): New define.
	* libgccjit.map: New function.

gcc/testsuite/ChangeLog:

	* jit.dg/all-non-failing-tests.h: Mention test-readonly.c.
	* jit.dg/test-error-assign-readonly.c: New test.
	* jit.dg/test-readonly.c: New test.
---
 gcc/jit/docs/topics/compatibility.rst         |  7 +++
 gcc/jit/docs/topics/expressions.rst           | 12 ++++
 gcc/jit/jit-playback.cc                       | 15 +++--
 gcc/jit/jit-playback.h                        |  9 ++-
 gcc/jit/jit-recording.cc                      | 10 ++-
 gcc/jit/jit-recording.h                       | 12 ++++
 gcc/jit/libgccjit.cc                          | 22 +++++++
 gcc/jit/libgccjit.h                           |  5 ++
 gcc/jit/libgccjit.map                         |  5 ++
 gcc/testsuite/jit.dg/all-non-failing-tests.h  |  3 +
 .../jit.dg/test-error-assign-readonly.c       | 62 +++++++++++++++++++
 gcc/testsuite/jit.dg/test-readonly.c          | 38 ++++++++++++
 12 files changed, 189 insertions(+), 11 deletions(-)
 create mode 100644 gcc/testsuite/jit.dg/test-error-assign-readonly.c
 create mode 100644 gcc/testsuite/jit.dg/test-readonly.c

diff --git a/gcc/jit/docs/topics/compatibility.rst b/gcc/jit/docs/topics/compatibility.rst
index ebede440ee4..e13581d0685 100644
--- a/gcc/jit/docs/topics/compatibility.rst
+++ b/gcc/jit/docs/topics/compatibility.rst
@@ -378,3 +378,10 @@  alignment of a variable:
 --------------------
 ``LIBGCCJIT_ABI_25`` covers the addition of
 :func:`gcc_jit_type_get_restrict`
+
+.. _LIBGCCJIT_ABI_26:
+
+``LIBGCCJIT_ABI_26``
+--------------------
+``LIBGCCJIT_ABI_26`` covers the addition of
+:func:`gcc_jit_global_set_readonly`
diff --git a/gcc/jit/docs/topics/expressions.rst b/gcc/jit/docs/topics/expressions.rst
index 42cfee36302..8d4aa96e64a 100644
--- a/gcc/jit/docs/topics/expressions.rst
+++ b/gcc/jit/docs/topics/expressions.rst
@@ -944,6 +944,18 @@  Global variables
 
       #ifdef LIBGCCJIT_HAVE_CTORS
 
+.. function:: void\
+              gcc_jit_global_set_readonly (gcc_jit_lvalue *global)
+
+   Set the global variable as read-only, meaning you cannot assign to this variable.
+
+   This entrypoint was added in :ref:`LIBGCCJIT_ABI_26`; you can test for its
+   presence using:
+
+   .. code-block:: c
+
+      #ifdef LIBGCCJIT_HAVE_gcc_jit_global_set_readonly
+
 Working with pointers, structs and unions
 -----------------------------------------
 
diff --git a/gcc/jit/jit-playback.cc b/gcc/jit/jit-playback.cc
index dddd537f3b1..420f3a843cf 100644
--- a/gcc/jit/jit-playback.cc
+++ b/gcc/jit/jit-playback.cc
@@ -607,7 +607,8 @@  global_new_decl (location *loc,
 		 enum gcc_jit_global_kind kind,
 		 type *type,
 		 const char *name,
-		 enum global_var_flags flags)
+		 enum global_var_flags flags,
+		 bool readonly)
 {
   gcc_assert (type);
   gcc_assert (name);
@@ -646,7 +647,7 @@  global_new_decl (location *loc,
       break;
     }
 
-  if (TYPE_READONLY (type_tree))
+  if (TYPE_READONLY (type_tree) || readonly)
     TREE_READONLY (inner) = 1;
 
   if (loc)
@@ -674,10 +675,11 @@  new_global (location *loc,
 	    enum gcc_jit_global_kind kind,
 	    type *type,
 	    const char *name,
-	    enum global_var_flags flags)
+	    enum global_var_flags flags,
+	    bool readonly)
 {
   tree inner =
-    global_new_decl (loc, kind, type, name, flags);
+    global_new_decl (loc, kind, type, name, flags, readonly);
 
   return global_finalize_lvalue (inner);
 }
@@ -822,9 +824,10 @@  new_global_initialized (location *loc,
 			size_t initializer_num_elem,
 			const void *initializer,
 			const char *name,
-			enum global_var_flags flags)
+			enum global_var_flags flags,
+			bool readonly)
 {
-  tree inner = global_new_decl (loc, kind, type, name, flags);
+  tree inner = global_new_decl (loc, kind, type, name, flags, readonly);
 
   vec<constructor_elt, va_gc> *constructor_elements = NULL;
 
diff --git a/gcc/jit/jit-playback.h b/gcc/jit/jit-playback.h
index b0166f8f6ce..7c1d8d88b4d 100644
--- a/gcc/jit/jit-playback.h
+++ b/gcc/jit/jit-playback.h
@@ -111,7 +111,8 @@  public:
 	      enum gcc_jit_global_kind kind,
 	      type *type,
 	      const char *name,
-	      enum global_var_flags flags);
+	      enum global_var_flags flags,
+	      bool readonly);
 
   lvalue *
   new_global_initialized (location *loc,
@@ -121,7 +122,8 @@  public:
                           size_t initializer_num_elem,
                           const void *initializer,
 			  const char *name,
-			  enum global_var_flags flags);
+			  enum global_var_flags flags,
+			  bool readonly);
 
   rvalue *
   new_ctor (location *log,
@@ -306,7 +308,8 @@  private:
                    enum gcc_jit_global_kind kind,
                    type *type,
 		   const char *name,
-		   enum global_var_flags flags);
+		   enum global_var_flags flags,
+		   bool readonly);
   lvalue *
   global_finalize_lvalue (tree inner);
 
diff --git a/gcc/jit/jit-recording.cc b/gcc/jit/jit-recording.cc
index 9b5b8005ebe..f86e7439301 100644
--- a/gcc/jit/jit-recording.cc
+++ b/gcc/jit/jit-recording.cc
@@ -4879,12 +4879,14 @@  recording::global::replay_into (replayer *r)
 				 / m_type->dereference ()->get_size (),
 				 m_initializer,
 				 playback_string (m_name),
-				 m_flags)
+				 m_flags,
+				 m_readonly)
     : r->new_global (playback_location (r, m_loc),
 		     m_kind,
 		     m_type->playback_type (),
 		     playback_string (m_name),
-		     m_flags);
+		     m_flags,
+		     m_readonly);
 
   if (m_tls_model != GCC_JIT_TLS_MODEL_NONE)
     global->set_tls_model (recording::tls_models[m_tls_model]);
@@ -5042,6 +5044,10 @@  recording::global::write_reproducer (reproducer &r)
      id,
      m_link_section->c_str ());
 
+  if (m_readonly)
+    r.write ("  gcc_jit_global_set_readonly (%s, /* gcc_jit_lvalue *lvalue */);\n",
+     id);
+
   if (m_initializer)
     switch (m_type->dereference ()->get_size ())
       {
diff --git a/gcc/jit/jit-recording.h b/gcc/jit/jit-recording.h
index 4a8082991fb..5b3c855b7e8 100644
--- a/gcc/jit/jit-recording.h
+++ b/gcc/jit/jit-recording.h
@@ -1236,6 +1236,17 @@  public:
   as_rvalue () { return this; }
 
   const char *access_as_rvalue (reproducer &r) override;
+
+  bool get_readonly () const
+  {
+    return m_readonly;
+  }
+
+  void set_readonly ()
+  {
+    m_readonly = true;
+  }
+
   virtual const char *access_as_lvalue (reproducer &r);
   virtual bool is_global () const { return false; }
   void set_tls_model (enum gcc_jit_tls_model model);
@@ -1249,6 +1260,7 @@  protected:
   string *m_reg_name;
   enum gcc_jit_tls_model m_tls_model;
   unsigned m_alignment;
+  bool m_readonly = false;
 };
 
 class param : public lvalue
diff --git a/gcc/jit/libgccjit.cc b/gcc/jit/libgccjit.cc
index 0451b4df7f9..1e96ee5559f 100644
--- a/gcc/jit/libgccjit.cc
+++ b/gcc/jit/libgccjit.cc
@@ -1868,6 +1868,23 @@  gcc_jit_global_set_initializer (gcc_jit_lvalue *global,
   return global;
 }
 
+/* Public entrypoint.  See description in libgccjit.h.
+
+   After error-checking, the real work is done by the
+   gcc::jit::recording::global::set_readonly method, in
+   jit-recording.cc.  */
+
+extern void
+gcc_jit_global_set_readonly (gcc_jit_lvalue *global)
+{
+  RETURN_IF_FAIL (global, NULL, NULL, "NULL global");
+  RETURN_IF_FAIL_PRINTF1 (global->is_global (), NULL, NULL,
+			       "lvalue \"%s\" not a global",
+			       global->get_debug_string ());
+
+  global->set_readonly ();
+}
+
 /* Public entrypoint.  See description in libgccjit.h.
 
    After error-checking, this calls the trivial
@@ -2844,6 +2861,11 @@  gcc_jit_block_add_assignment (gcc_jit_block *block,
     lvalue->get_type ()->get_debug_string (),
     rvalue->get_debug_string (),
     rvalue->get_type ()->get_debug_string ());
+  RETURN_IF_FAIL_PRINTF1 (
+    !lvalue->get_readonly (),
+    ctxt, loc,
+    "cannot assign to readonly variable: %s",
+    lvalue->get_debug_string ());
 
   gcc::jit::recording::statement *stmt = block->add_assignment (loc, lvalue, rvalue);
 
diff --git a/gcc/jit/libgccjit.h b/gcc/jit/libgccjit.h
index 749f6c24177..5ac587e5bfc 100644
--- a/gcc/jit/libgccjit.h
+++ b/gcc/jit/libgccjit.h
@@ -1045,6 +1045,11 @@  gcc_jit_global_set_initializer (gcc_jit_lvalue *global,
 				const void *blob,
 				size_t num_bytes);
 
+extern void
+gcc_jit_global_set_readonly (gcc_jit_lvalue *global);
+
+#define LIBGCCJIT_HAVE_gcc_jit_global_set_readonly
+
 /* Upcasting.  */
 extern gcc_jit_object *
 gcc_jit_lvalue_as_object (gcc_jit_lvalue *lvalue);
diff --git a/gcc/jit/libgccjit.map b/gcc/jit/libgccjit.map
index 8b90a0e2ff3..319e0e9b796 100644
--- a/gcc/jit/libgccjit.map
+++ b/gcc/jit/libgccjit.map
@@ -276,3 +276,8 @@  LIBGCCJIT_ABI_25 {
   global:
     gcc_jit_type_get_restrict;
 } LIBGCCJIT_ABI_24;
+
+LIBGCCJIT_ABI_26 {
+  global:
+    gcc_jit_global_set_readonly;
+} LIBGCCJIT_ABI_25;
diff --git a/gcc/testsuite/jit.dg/all-non-failing-tests.h b/gcc/testsuite/jit.dg/all-non-failing-tests.h
index e762563f9bd..1dd45accdc9 100644
--- a/gcc/testsuite/jit.dg/all-non-failing-tests.h
+++ b/gcc/testsuite/jit.dg/all-non-failing-tests.h
@@ -313,6 +313,9 @@ 
 #undef create_code
 #undef verify_code
 
+/* test-readonly.c: This can't be in the testcases array as it
+   is target-specific.  */
+
 /* test-restrict.c: This can't be in the testcases array as it needs
    the `-O3` flag.  */
 
diff --git a/gcc/testsuite/jit.dg/test-error-assign-readonly.c b/gcc/testsuite/jit.dg/test-error-assign-readonly.c
new file mode 100644
index 00000000000..628bdb86d1c
--- /dev/null
+++ b/gcc/testsuite/jit.dg/test-error-assign-readonly.c
@@ -0,0 +1,62 @@ 
+#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:
+
+     const long integer = 10;
+
+     void
+     test_fn ()
+     {
+        integer = 12;
+     }
+
+     and verify that the API complains about assigning to a read-only
+     variable.
+  */
+  gcc_jit_type *void_type =
+    gcc_jit_context_get_type (ctxt, GCC_JIT_TYPE_VOID);
+  gcc_jit_type *long_type =
+    gcc_jit_context_get_type (ctxt, GCC_JIT_TYPE_LONG);
+
+  gcc_jit_function *func =
+    gcc_jit_context_new_function (ctxt, NULL,
+                                  GCC_JIT_FUNCTION_EXPORTED,
+                                  void_type,
+                                  "test_fn",
+                                  0, NULL,
+                                  0);
+  gcc_jit_block *initial =
+    gcc_jit_function_new_block (func, "initial");
+
+  gcc_jit_lvalue *integer =
+    gcc_jit_context_new_global (
+      ctxt, NULL, GCC_JIT_GLOBAL_EXPORTED, long_type, "integer");
+
+  gcc_jit_rvalue *ten = gcc_jit_context_new_rvalue_from_int (ctxt, long_type, 10);
+  gcc_jit_global_set_initializer_rvalue (integer, ten);
+  gcc_jit_global_set_readonly(integer);
+
+  gcc_jit_rvalue *twelve = gcc_jit_context_new_rvalue_from_int (ctxt, long_type, 12);
+  gcc_jit_block_add_assignment(initial, NULL, integer, twelve);
+
+  gcc_jit_block_end_with_void_return (initial, NULL);
+}
+
+void
+verify_code (gcc_jit_context *ctxt, gcc_jit_result *result)
+{
+  CHECK_VALUE (result, NULL);
+
+  /* Verify that the correct error messages were emitted.  */
+  CHECK_STRING_VALUE (gcc_jit_context_get_first_error (ctxt),
+		      "gcc_jit_block_add_assignment:"
+		      " cannot assign to readonly variable: integer");
+}
diff --git a/gcc/testsuite/jit.dg/test-readonly.c b/gcc/testsuite/jit.dg/test-readonly.c
new file mode 100644
index 00000000000..554fa0bad78
--- /dev/null
+++ b/gcc/testsuite/jit.dg/test-readonly.c
@@ -0,0 +1,38 @@ 
+/* { dg-do compile { target x86_64-*-* } } */
+
+#include <stdlib.h>
+#include <stdio.h>
+
+#include "libgccjit.h"
+
+/* We don't want set_options() in harness.h to set -O3 so our little local
+   is optimized away. */
+#define TEST_ESCHEWS_SET_OPTIONS
+static void set_options (gcc_jit_context *ctxt, const char *argv0)
+{
+}
+
+#define TEST_COMPILING_TO_FILE
+#define OUTPUT_KIND      GCC_JIT_OUTPUT_KIND_ASSEMBLER
+#define OUTPUT_FILENAME  "output-of-test-readonly.c.s"
+#include "harness.h"
+
+void
+create_code (gcc_jit_context *ctxt, void *user_data)
+{
+  /* Let's try to inject the equivalent of:
+     const int foo;
+  */
+  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_rvalue *ten = gcc_jit_context_new_rvalue_from_int (ctxt, int_type, 10);
+  gcc_jit_global_set_initializer_rvalue (foo, ten);
+  gcc_jit_global_set_readonly(foo);
+}
+
+/* { dg-final { jit-verify-output-file-was-created "" } } */
+/* { dg-final { jit-verify-assembler-output ".section\t.rodata" } } */
-- 
2.43.0