| Message ID | gkr36j31l1s.fsf@arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | libgccjit: check result_type in gcc_jit_context_new_unary_op | expand |
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
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
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.
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
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
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 *"); +}