libgccjit: check result_type in gcc_jit_context_new_unary_op
diff mbox series

Message ID gkr36j31l1s.fsf@arm.com
State New
Headers show
Series
  • libgccjit: check result_type in gcc_jit_context_new_unary_op
Related show

Commit Message

Andrea Corallo July 18, 2019, 2:20 p.m. UTC
Hi all,
I've just realized that what we has been done recently for
gcc_jit_context_new_binary_op should be done also for the unary
version.
This patch checks at record time for the result type of
gcc_jit_context_new_unary_op to be numeric type plus add a testcase
for the new check.

make check-jit runs clean

Is it okay for trunk?

Bests
  Andrea

gcc/jit/ChangeLog
2019-07-18  Andrea Corallo <andrea.corallo@arm.com>

	* libgccjit.c (gcc_jit_context_new_unary_op): Check result_type to be a
	numeric type.
	* libgccjit.c (gcc_jit_context_new_binary_op): Fix nit in error message.

gcc/testsuite/ChangeLog
2019-07-04  Andrea Corallo <andrea.corallo@arm.com>

	* jit.dg/test-error-gcc_jit_context_new_unary_op-bad-res-type.c:
	New testcase.
	* jit.dg/test-error-gcc_jit_context_new_binary_op-bad-res-type.c:
	Fix nit in error message.

Comments

David Malcolm July 18, 2019, 4:07 p.m. UTC | #1
On Thu, 2019-07-18 at 14:20 +0000, Andrea Corallo wrote:
> Hi all,
> I've just realized that what we has been done recently for
> gcc_jit_context_new_binary_op should be done also for the unary
> version.
> This patch checks at record time for the result type of
> gcc_jit_context_new_unary_op to be numeric type plus add a testcase
> for the new check.
> 
> make check-jit runs clean
> 
> Is it okay for trunk?
> 
> Bests
>   Andrea
> 
> gcc/jit/ChangeLog
> 2019-07-18  Andrea Corallo <andrea.corallo@arm.com>
> 
> 	* libgccjit.c (gcc_jit_context_new_unary_op): Check result_type
> to be a
> 	numeric type.
> 	* libgccjit.c (gcc_jit_context_new_binary_op): Fix nit in error
> message.
> 
> gcc/testsuite/ChangeLog
> 2019-07-04  Andrea Corallo <andrea.corallo@arm.com>
> 
> 	* jit.dg/test-error-gcc_jit_context_new_unary_op-bad-res-
> type.c:
> 	New testcase.
> 	* jit.dg/test-error-gcc_jit_context_new_binary_op-bad-res-
> type.c:
> 	Fix nit in error message.

Thanks for the patch.  What happens with the existing code if the user
tries to use such a unary op?

> diff --git a/gcc/jit/libgccjit.c b/gcc/jit/libgccjit.c
> index 23e83e2..bea840f 100644
> --- a/gcc/jit/libgccjit.c
> +++ b/gcc/jit/libgccjit.c
> @@ -1336,6 +1336,12 @@ gcc_jit_context_new_unary_op (gcc_jit_context *ctxt,
>      "unrecognized value for enum gcc_jit_unary_op: %i",
>      op);
>    RETURN_NULL_IF_FAIL (result_type, ctxt, loc, "NULL result_type");
> +  RETURN_NULL_IF_FAIL_PRINTF3 (
> +    result_type->is_numeric (), ctxt, loc,
> +    "gcc_jit_unary_op %i with operand %s "
> +    "has non-numeric result_type: %s",
> +    op, rvalue->get_debug_string (),
> +    result_type->get_debug_string ());
>    RETURN_NULL_IF_FAIL (rvalue, ctxt, loc, "NULL rvalue");

The use of "%i" for "op" here isn't as user-friendly as it could be; it
would be ideal to tell the user the enum value.

"op" has already been validated, so why not expose the currently-static 
unary_op_reproducer_strings from jit-recording.c in an internal header,
and use it here with a "%s"?

>    return (gcc_jit_rvalue *)ctxt->new_unary_op (loc, op, result_type,
rvalue);
> @@ -1388,7 +1394,7 @@ gcc_jit_context_new_binary_op (gcc_jit_context
*ctxt,
>    RETURN_NULL_IF_FAIL_PRINTF4 (
>      result_type->is_numeric (), ctxt, loc,
>      "gcc_jit_binary_op %i with operands a: %s b: %s "
> -    "has non numeric result_type: %s",
> +    "has non-numeric result_type: %s",
>      op, a->get_debug_string (), b->get_debug_string (),
>      result_type->get_debug_string ());

Ah, I see there's one of these "%i" for op already.  Given that you're
already fixing a nit here, please make this print "%s", using
binary_op_reproducer_strings from jit-recording.c ("op" has already
been validated).

Thanks
Dave
Andrea Corallo July 18, 2019, 4:23 p.m. UTC | #2
David Malcolm writes:

> On Thu, 2019-07-18 at 14:20 +0000, Andrea Corallo wrote:
>> Hi all,
>> I've just realized that what we has been done recently for
>> gcc_jit_context_new_binary_op should be done also for the unary
>> version.
>> This patch checks at record time for the result type of
>> gcc_jit_context_new_unary_op to be numeric type plus add a testcase
>> for the new check.
>>
>> make check-jit runs clean
>>
>> Is it okay for trunk?
>>
>> Bests
>>   Andrea
>>
>> gcc/jit/ChangeLog
>> 2019-07-18  Andrea Corallo <andrea.corallo@arm.com>
>>
>> 	* libgccjit.c (gcc_jit_context_new_unary_op): Check result_type
>> to be a
>> 	numeric type.
>> 	* libgccjit.c (gcc_jit_context_new_binary_op): Fix nit in error
>> message.
>>
>> gcc/testsuite/ChangeLog
>> 2019-07-04  Andrea Corallo <andrea.corallo@arm.com>
>>
>> 	* jit.dg/test-error-gcc_jit_context_new_unary_op-bad-res-
>> type.c:
>> 	New testcase.
>> 	* jit.dg/test-error-gcc_jit_context_new_binary_op-bad-res-
>> type.c:
>> 	Fix nit in error message.
>
> Thanks for the patch.  What happens with the existing code if the user
> tries to use such a unary op?

In case the res type is something "exotic" like a structure I've
encountered an ICE, if I'm not wrong again during gimplification.

>> diff --git a/gcc/jit/libgccjit.c b/gcc/jit/libgccjit.c
>> index 23e83e2..bea840f 100644
>> --- a/gcc/jit/libgccjit.c
>> +++ b/gcc/jit/libgccjit.c
>> @@ -1336,6 +1336,12 @@ gcc_jit_context_new_unary_op (gcc_jit_context *ctxt,
>>      "unrecognized value for enum gcc_jit_unary_op: %i",
>>      op);
>>    RETURN_NULL_IF_FAIL (result_type, ctxt, loc, "NULL result_type");
>> +  RETURN_NULL_IF_FAIL_PRINTF3 (
>> +    result_type->is_numeric (), ctxt, loc,
>> +    "gcc_jit_unary_op %i with operand %s "
>> +    "has non-numeric result_type: %s",
>> +    op, rvalue->get_debug_string (),
>> +    result_type->get_debug_string ());
>>    RETURN_NULL_IF_FAIL (rvalue, ctxt, loc, "NULL rvalue");
>
> The use of "%i" for "op" here isn't as user-friendly as it could be; it
> would be ideal to tell the user the enum value.
>
> "op" has already been validated, so why not expose the currently-static
> unary_op_reproducer_strings from jit-recording.c in an internal header,
> and use it here with a "%s"?
>
>>    return (gcc_jit_rvalue *)ctxt->new_unary_op (loc, op, result_type,
> rvalue);
>> @@ -1388,7 +1394,7 @@ gcc_jit_context_new_binary_op (gcc_jit_context
> *ctxt,
>>    RETURN_NULL_IF_FAIL_PRINTF4 (
>>      result_type->is_numeric (), ctxt, loc,
>>      "gcc_jit_binary_op %i with operands a: %s b: %s "
>> -    "has non numeric result_type: %s",
>> +    "has non-numeric result_type: %s",
>>      op, a->get_debug_string (), b->get_debug_string (),
>>      result_type->get_debug_string ());
>
> Ah, I see there's one of these "%i" for op already.  Given that you're
> already fixing a nit here, please make this print "%s", using
> binary_op_reproducer_strings from jit-recording.c ("op" has already
> been validated).
>
> Thanks
> Dave

That's a really good idea I'll update the patch.
Thanks for the comments.

Bests
  Andrea
Andrea Corallo July 22, 2019, 2:16 p.m. UTC | #3
Hi all,
second version of the patch here addressing comments.

make check-jit runs clean

Bests
  Andrea

gcc/jit/ChangeLog
2019-07-18  Andrea Corallo <andrea.corallo@arm.com>

	* jit-recording.c (unary_op_reproducer_strings): Make it extern.
	(binary_op_reproducer_strings): Likewise.
	* jit-recording.h (unary_op_reproducer_strings): Likewise.
	(binary_op_reproducer_strings): Likewise.
	* libgccjit.c (gcc_jit_context_new_unary_op): Check result_type to be a
	numeric type.
	* libgccjit.c (gcc_jit_context_new_binary_op): Improve error message.

gcc/testsuite/ChangeLog
2019-07-04  Andrea Corallo <andrea.corallo@arm.com>

	* jit.dg/test-error-gcc_jit_context_new_unary_op-bad-res-type.c:
	New testcase.
	* jit.dg/test-error-gcc_jit_context_new_binary_op-bad-res-type.c:
	Adjust error message.
David Malcolm July 22, 2019, 3:11 p.m. UTC | #4
On Thu, 2019-07-18 at 14:20 +0000, Andrea Corallo wrote:
> Hi all,
> I've just realized that what we has been done recently for
> gcc_jit_context_new_binary_op should be done also for the unary
> version.
> This patch checks at record time for the result type of
> gcc_jit_context_new_unary_op to be numeric type plus add a testcase
> for the new check.
> 
> make check-jit runs clean
> 
> Is it okay for trunk?

Thanks - this is good for trunk.

Dave
Andrea Corallo July 22, 2019, 3:46 p.m. UTC | #5
David Malcolm writes:

> On Thu, 2019-07-18 at 14:20 +0000, Andrea Corallo wrote:
>> Hi all,
>> I've just realized that what we has been done recently for
>> gcc_jit_context_new_binary_op should be done also for the unary
>> version.
>> This patch checks at record time for the result type of
>> gcc_jit_context_new_unary_op to be numeric type plus add a testcase
>> for the new check.
>>
>> make check-jit runs clean
>>
>> Is it okay for trunk?
>
> Thanks - this is good for trunk.
>
> Dave

Thanks,
last version committed as r273700

Bests
  Andrea

Patch
diff mbox series

diff --git a/gcc/jit/libgccjit.c b/gcc/jit/libgccjit.c
index 23e83e2..bea840f 100644
--- a/gcc/jit/libgccjit.c
+++ b/gcc/jit/libgccjit.c
@@ -1336,6 +1336,12 @@  gcc_jit_context_new_unary_op (gcc_jit_context *ctxt,
     "unrecognized value for enum gcc_jit_unary_op: %i",
     op);
   RETURN_NULL_IF_FAIL (result_type, ctxt, loc, "NULL result_type");
+  RETURN_NULL_IF_FAIL_PRINTF3 (
+    result_type->is_numeric (), ctxt, loc,
+    "gcc_jit_unary_op %i with operand %s "
+    "has non-numeric result_type: %s",
+    op, rvalue->get_debug_string (),
+    result_type->get_debug_string ());
   RETURN_NULL_IF_FAIL (rvalue, ctxt, loc, "NULL rvalue");
 
   return (gcc_jit_rvalue *)ctxt->new_unary_op (loc, op, result_type, rvalue);
@@ -1388,7 +1394,7 @@  gcc_jit_context_new_binary_op (gcc_jit_context *ctxt,
   RETURN_NULL_IF_FAIL_PRINTF4 (
     result_type->is_numeric (), ctxt, loc,
     "gcc_jit_binary_op %i with operands a: %s b: %s "
-    "has non numeric result_type: %s",
+    "has non-numeric result_type: %s",
     op, a->get_debug_string (), b->get_debug_string (),
     result_type->get_debug_string ());
 
diff --git a/gcc/testsuite/jit.dg/test-error-gcc_jit_context_new_binary_op-bad-res-type.c b/gcc/testsuite/jit.dg/test-error-gcc_jit_context_new_binary_op-bad-res-type.c
index abadc9f..d2a0963 100644
--- a/gcc/testsuite/jit.dg/test-error-gcc_jit_context_new_binary_op-bad-res-type.c
+++ b/gcc/testsuite/jit.dg/test-error-gcc_jit_context_new_binary_op-bad-res-type.c
@@ -36,6 +36,6 @@  verify_code (gcc_jit_context *ctxt, gcc_jit_result *result)
   /* Verify that the correct error message was emitted.	 */
   CHECK_STRING_VALUE (gcc_jit_context_get_first_error (ctxt),
 		      "gcc_jit_context_new_binary_op: gcc_jit_binary_op 1 with"
-		      " operands a: (int)1 b: (int)2 has non numeric "
+		      " operands a: (int)1 b: (int)2 has non-numeric "
 		      "result_type: void *");
 }
diff --git a/gcc/testsuite/jit.dg/test-error-gcc_jit_context_new_unary_op-bad-res-type.c b/gcc/testsuite/jit.dg/test-error-gcc_jit_context_new_unary_op-bad-res-type.c
new file mode 100644
index 0000000..f547974
--- /dev/null
+++ b/gcc/testsuite/jit.dg/test-error-gcc_jit_context_new_unary_op-bad-res-type.c
@@ -0,0 +1,37 @@ 
+#include <stdlib.h>
+#include <stdio.h>
+
+#include "libgccjit.h"
+
+#include "harness.h"
+
+/* Try to create an unary operator with invalid result type.  */
+
+void
+create_code (gcc_jit_context *ctxt, void *user_data)
+{
+  gcc_jit_type *int_type =
+    gcc_jit_context_get_type (ctxt, GCC_JIT_TYPE_INT);
+  gcc_jit_type *void_ptr_type =
+    gcc_jit_context_get_type (ctxt, GCC_JIT_TYPE_VOID_PTR);
+
+  gcc_jit_context_new_unary_op (
+    ctxt,
+    NULL,
+    GCC_JIT_UNARY_OP_LOGICAL_NEGATE,
+    void_ptr_type,
+    gcc_jit_context_new_rvalue_from_int (ctxt,
+					 int_type,
+					 1));
+}
+
+void
+verify_code (gcc_jit_context *ctxt, gcc_jit_result *result)
+{
+  CHECK_VALUE (result, NULL);
+
+  /* Verify that the correct error message was emitted.	 */
+  CHECK_STRING_VALUE (gcc_jit_context_get_first_error (ctxt),
+		      "gcc_jit_context_new_unary_op: gcc_jit_unary_op 2 with "
+		      "operand (int)1 has non-numeric result_type: void *");
+}