diff mbox series

libgccjit: Add type checks in gcc_jit_block_add_assignment_op

Message ID 447a6c24782a4275736dd38f35e17e42612ee80d.camel@zoho.com
State Accepted
Headers
Series libgccjit: Add type checks in gcc_jit_block_add_assignment_op |

Checks

Context Check Description
snail/gcc-patch-check success Github commit url

Commit Message

Antoni Boucher Dec. 7, 2023, 10:34 p.m. UTC 
  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.

Comments

David Malcolm Dec. 8, 2023, 1:15 a.m. UTC | #1 
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
Antoni Boucher Dec. 21, 2023, 1:36 p.m. UTC | #2 
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
> 
> 
>
Antoni Boucher Feb. 17, 2024, 4:54 p.m. UTC | #3 
David: Ping.

On Thu, 2023-12-21 at 08:36 -0500, Antoni Boucher wrote:
> 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
> > 
> > 
> > 
>
diff mbox series

Patch

From 932048619671c61af224708a3da484b9f54a30a3 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..eb6817a0a99 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 () && 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 ());
+  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 ());
 
   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