Re: [PATCH] libgccjit: Add type checks in gcc_jit_block_add_assignment_op

[Antoni Boucher <bouanto@zoho.com>]

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

Hi.
Here's the updated patch.
Thanks.

On Thu, 2023-12-07 at 20:15 -0500, David Malcolm wrote:
> On Thu, 2023-12-07 at 17:34 -0500, Antoni Boucher wrote:
> > Hi.
> > This patch adds checks gcc_jit_block_add_assignment_op to make sure
> > it
> > is only ever called on numeric types.
> > 
> > With the previous patch, this might require a change to also allow
> > vector types here.
> > 
> > Thanks for the review.
> 
> Thanks for the patch.
> 
> [...snip...]
> 
> > @@ -2890,6 +2900,17 @@ gcc_jit_block_add_assignment_op
> > (gcc_jit_block *block,
> >      lvalue->get_type ()->get_debug_string (),
> >      rvalue->get_debug_string (),
> >      rvalue->get_type ()->get_debug_string ());
> > +  // TODO: check if it is a numeric vector?
> > +  RETURN_IF_FAIL_PRINTF3 (
> > +    lvalue->get_type ()->is_numeric () && rvalue->get_type ()-
> > >is_numeric (), ctxt, loc,
> > +    "gcc_jit_block_add_assignment_op %s has non-numeric lvalue %s
> > (type: %s)",
> > +    gcc::jit::binary_op_reproducer_strings[op],
> > +    lvalue->get_debug_string (), lvalue->get_type ()-
> > >get_debug_string ());
> 
> The condition being tested here should probably just be:
> 
>    lvalue->get_type ()->is_numeric ()
> 
> since otherwise if the lvalue's type is numeric and the rvalue's type
> fails to be, then the user would incorrectly get a message about the
> lvalue.
> 
> > +  RETURN_IF_FAIL_PRINTF3 (
> > +    rvalue->get_type ()->is_numeric () && rvalue->get_type ()-
> > >is_numeric (), ctxt, loc,
> > +    "gcc_jit_block_add_assignment_op %s has non-numeric rvalue %s
> > (type: %s)",
> > +    gcc::jit::binary_op_reproducer_strings[op],
> > +    rvalue->get_debug_string (), rvalue->get_type ()-
> > >get_debug_string ());
> 
> The condition being tested here seems to have a redundant repeated:
>   && rvalue->get_type ()->is_numeric ()
> 
> Am I missing something, or is that a typo?
> 
> [...snip...]
> 
> The patch is OK otherwise.
> 
> Thanks
> Dave
> 
> 
> 


[-- Attachment #2: 0001-libgccjit-Add-type-checks-in-gcc_jit_block_add_assig.patch --]
[-- Type: text/x-patch, Size: 4363 bytes --]

From a93b029db4622ff6385715ff9cdaf1be5ffa5657 Mon Sep 17 00:00:00 2001
From: Antoni Boucher <bouanto@zoho.com>
Date: Wed, 18 Oct 2023 18:33:18 -0400
Subject: [PATCH] libgccjit: Add type checks in gcc_jit_block_add_assignment_op

gcc/jit/ChangeLog:

	* libgccjit.cc (RETURN_IF_FAIL_PRINTF3): New macro.
	(gcc_jit_block_add_assignment_op): Add numeric checks.

gcc/testsuite/ChangeLog:

	* jit.dg/test-error-bad-assignment-op.c: New test.
---
 gcc/jit/libgccjit.cc                          | 21 +++++++
 .../jit.dg/test-error-bad-assignment-op.c     | 57 +++++++++++++++++++
 2 files changed, 78 insertions(+)
 create mode 100644 gcc/testsuite/jit.dg/test-error-bad-assignment-op.c

diff --git a/gcc/jit/libgccjit.cc b/gcc/jit/libgccjit.cc
index 0451b4df7f9..10d23e7fcf6 100644
--- a/gcc/jit/libgccjit.cc
+++ b/gcc/jit/libgccjit.cc
@@ -267,6 +267,16 @@ struct gcc_jit_extended_asm : public gcc::jit::recording::extended_asm
       }								\
   JIT_END_STMT
 
+#define RETURN_IF_FAIL_PRINTF3(TEST_EXPR, CTXT, LOC, ERR_FMT, A0, A1, A2) \
+  JIT_BEGIN_STMT							\
+    if (!(TEST_EXPR))							\
+      {								\
+	jit_error ((CTXT), (LOC), "%s: " ERR_FMT,			\
+		   __func__, (A0), (A1), (A2));			\
+	return;							\
+      }								\
+  JIT_END_STMT
+
 #define RETURN_IF_FAIL_PRINTF4(TEST_EXPR, CTXT, LOC, ERR_FMT, A0, A1, A2, A3) \
   JIT_BEGIN_STMT							\
     if (!(TEST_EXPR))							\
@@ -2890,6 +2900,17 @@ gcc_jit_block_add_assignment_op (gcc_jit_block *block,
     lvalue->get_type ()->get_debug_string (),
     rvalue->get_debug_string (),
     rvalue->get_type ()->get_debug_string ());
+  // TODO: check if it is a numeric vector?
+  RETURN_IF_FAIL_PRINTF3 (
+    lvalue->get_type ()->is_numeric (), ctxt, loc,
+    "gcc_jit_block_add_assignment_op %s has non-numeric lvalue %s (type: %s)",
+    gcc::jit::binary_op_reproducer_strings[op],
+    lvalue->get_debug_string (), lvalue->get_type ()->get_debug_string ());
+  RETURN_IF_FAIL_PRINTF3 (
+    rvalue->get_type ()->is_numeric (), ctxt, loc,
+    "gcc_jit_block_add_assignment_op %s has non-numeric rvalue %s (type: %s)",
+    gcc::jit::binary_op_reproducer_strings[op],
+    rvalue->get_debug_string (), rvalue->get_type ()->get_debug_string ());
 
   gcc::jit::recording::statement *stmt = block->add_assignment_op (loc, lvalue, op, rvalue);
 
diff --git a/gcc/testsuite/jit.dg/test-error-bad-assignment-op.c b/gcc/testsuite/jit.dg/test-error-bad-assignment-op.c
new file mode 100644
index 00000000000..683ebbfb1fe
--- /dev/null
+++ b/gcc/testsuite/jit.dg/test-error-bad-assignment-op.c
@@ -0,0 +1,57 @@
+#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:
+
+     void
+     test_fn ()
+     {
+        const char *variable;
+        variable += "test";
+     }
+
+     and verify that the API complains about the mismatching types
+     in the assignments.
+  */
+  gcc_jit_type *void_type =
+    gcc_jit_context_get_type (ctxt, GCC_JIT_TYPE_VOID);
+  gcc_jit_type *const_char_ptr_type =
+    gcc_jit_context_get_type (ctxt, GCC_JIT_TYPE_CONST_CHAR_PTR);
+
+  gcc_jit_function *func =
+    gcc_jit_context_new_function (ctxt, NULL,
+                                  GCC_JIT_FUNCTION_EXPORTED,
+                                  void_type,
+                                  "test_fn",
+                                  0, NULL,
+                                  0);
+
+  gcc_jit_lvalue *variable = gcc_jit_function_new_local (func, NULL, const_char_ptr_type, "variable");
+  gcc_jit_block *initial =
+    gcc_jit_function_new_block (func, "initial");
+  gcc_jit_rvalue *string =
+    gcc_jit_context_new_string_literal (ctxt, "test");
+  gcc_jit_block_add_assignment_op (initial, NULL, variable, GCC_JIT_BINARY_OP_PLUS, string);
+
+  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_op:"
+                      " gcc_jit_block_add_assignment_op GCC_JIT_BINARY_OP_PLUS"
+                      " has non-numeric lvalue variable (type: const char *)");
+}
+
-- 
2.43.0