[PATCH] libgccjit: Add gcc_jit_global_set_readonly

[PATCH] libgccjit: Add gcc_jit_global_set_readonly

[Antoni Boucher]

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

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.

[-- Attachment #2: 0001-libgccjit-Add-gcc_jit_global_set_readonly.patch --]
[-- Type: text/x-patch, Size: 13754 bytes --]

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

Re: [PATCH] libgccjit: Add gcc_jit_global_set_readonly

[David Malcolm]

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

Re: [PATCH] libgccjit: Add gcc_jit_global_set_readonly

[Antoni Boucher]

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
> 

Re: [PATCH] libgccjit: Add gcc_jit_global_set_readonly

[Antoni Boucher]

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