* [PATCH][gcc] libgccjit: check result_type in gcc_jit_context_new_unary_op @ 2019-01-01 0:00 Andrea Corallo 2019-01-01 0:00 ` David Malcolm 2019-01-01 0:00 ` David Malcolm 0 siblings, 2 replies; 6+ messages in thread From: Andrea Corallo @ 2019-01-01 0:00 UTC (permalink / raw) To: jit, gcc-patches; +Cc: nd [-- Attachment #1: Type: text/plain, Size: 865 bytes --] 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. [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #2: unary_op_check_result_type.patch --] [-- Type: text/x-diff; name="unary_op_check_result_type.patch", Size: 3266 bytes --] 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 *"); +} ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH][gcc] libgccjit: check result_type in gcc_jit_context_new_unary_op 2019-01-01 0:00 [PATCH][gcc] libgccjit: check result_type in gcc_jit_context_new_unary_op Andrea Corallo @ 2019-01-01 0:00 ` David Malcolm 2019-01-01 0:00 ` Andrea Corallo 2019-01-01 0:00 ` Andrea Corallo 2019-01-01 0:00 ` David Malcolm 1 sibling, 2 replies; 6+ messages in thread From: David Malcolm @ 2019-01-01 0:00 UTC (permalink / raw) To: Andrea Corallo, jit, gcc-patches; +Cc: nd 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH][gcc] libgccjit: check result_type in gcc_jit_context_new_unary_op 2019-01-01 0:00 ` David Malcolm @ 2019-01-01 0:00 ` Andrea Corallo 2019-01-01 0:00 ` Andrea Corallo 1 sibling, 0 replies; 6+ messages in thread From: Andrea Corallo @ 2019-01-01 0:00 UTC (permalink / raw) To: David Malcolm; +Cc: Andrea Corallo, jit, gcc-patches, nd [-- Attachment #1: Type: text/plain, Size: 823 bytes --] 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. [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #2: unary_op_check_result_type.patch --] [-- Type: text/x-diff; name="unary_op_check_result_type.patch", Size: 4977 bytes --] diff --git a/gcc/jit/jit-recording.c b/gcc/jit/jit-recording.c index 495ac7f..2f75395 100644 --- a/gcc/jit/jit-recording.c +++ b/gcc/jit/jit-recording.c @@ -4888,7 +4888,7 @@ recording::unary_op::make_debug_string () m_a->get_debug_string ()); } -static const char * const unary_op_reproducer_strings[] = { +const char * const unary_op_reproducer_strings[] = { "GCC_JIT_UNARY_OP_MINUS", "GCC_JIT_UNARY_OP_BITWISE_NEGATE", "GCC_JIT_UNARY_OP_LOGICAL_NEGATE", @@ -4968,7 +4968,7 @@ recording::binary_op::make_debug_string () m_b->get_debug_string_parens (prec)); } -static const char * const binary_op_reproducer_strings[] = { +const char * const binary_op_reproducer_strings[] = { "GCC_JIT_BINARY_OP_PLUS", "GCC_JIT_BINARY_OP_MINUS", "GCC_JIT_BINARY_OP_MULT", diff --git a/gcc/jit/jit-recording.h b/gcc/jit/jit-recording.h index 13ec7ea..4bd346e 100644 --- a/gcc/jit/jit-recording.h +++ b/gcc/jit/jit-recording.h @@ -30,6 +30,9 @@ namespace gcc { namespace jit { +extern const char * const unary_op_reproducer_strings[]; +extern const char * const binary_op_reproducer_strings[]; + class result; class dump; class reproducer; diff --git a/gcc/jit/libgccjit.c b/gcc/jit/libgccjit.c index 23e83e2..eec2f00 100644 --- a/gcc/jit/libgccjit.c +++ b/gcc/jit/libgccjit.c @@ -1336,6 +1336,13 @@ 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 %s with operand %s " + "has non-numeric result_type: %s", + gcc::jit::unary_op_reproducer_strings[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); @@ -1387,9 +1394,10 @@ gcc_jit_context_new_binary_op (gcc_jit_context *ctxt, b->get_type ()->get_debug_string ()); 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", - op, a->get_debug_string (), b->get_debug_string (), + "gcc_jit_binary_op %s with operands a: %s b: %s " + "has non-numeric result_type: %s", + gcc::jit::binary_op_reproducer_strings[op], + a->get_debug_string (), b->get_debug_string (), result_type->get_debug_string ()); return (gcc_jit_rvalue *)ctxt->new_binary_op (loc, op, result_type, a, b); 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..fbbb2e7 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 @@ -35,7 +35,7 @@ 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 " - "result_type: void *"); + "gcc_jit_context_new_binary_op: gcc_jit_binary_op " + "GCC_JIT_BINARY_OP_MINUS with 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..fae722a --- /dev/null +++ b/gcc/testsuite/jit.dg/test-error-gcc_jit_context_new_unary_op-bad-res-type.c @@ -0,0 +1,38 @@ +#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 " + "GCC_JIT_UNARY_OP_LOGICAL_NEGATE with operand " + "(int)1 has non-numeric result_type: void *"); +} ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH][gcc] libgccjit: check result_type in gcc_jit_context_new_unary_op 2019-01-01 0:00 ` David Malcolm 2019-01-01 0:00 ` Andrea Corallo @ 2019-01-01 0:00 ` Andrea Corallo 1 sibling, 0 replies; 6+ messages in thread From: Andrea Corallo @ 2019-01-01 0:00 UTC (permalink / raw) To: David Malcolm; +Cc: Andrea Corallo, jit, gcc-patches, nd 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH][gcc] libgccjit: check result_type in gcc_jit_context_new_unary_op 2019-01-01 0:00 [PATCH][gcc] libgccjit: check result_type in gcc_jit_context_new_unary_op Andrea Corallo 2019-01-01 0:00 ` David Malcolm @ 2019-01-01 0:00 ` David Malcolm 2019-01-01 0:00 ` Andrea Corallo 1 sibling, 1 reply; 6+ messages in thread From: David Malcolm @ 2019-01-01 0:00 UTC (permalink / raw) To: Andrea Corallo, jit, gcc-patches; +Cc: nd 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH][gcc] libgccjit: check result_type in gcc_jit_context_new_unary_op 2019-01-01 0:00 ` David Malcolm @ 2019-01-01 0:00 ` Andrea Corallo 0 siblings, 0 replies; 6+ messages in thread From: Andrea Corallo @ 2019-01-01 0:00 UTC (permalink / raw) To: David Malcolm; +Cc: Andrea Corallo, jit, gcc-patches, nd 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-07-22 15:46 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-01-01 0:00 [PATCH][gcc] libgccjit: check result_type in gcc_jit_context_new_unary_op Andrea Corallo 2019-01-01 0:00 ` David Malcolm 2019-01-01 0:00 ` Andrea Corallo 2019-01-01 0:00 ` Andrea Corallo 2019-01-01 0:00 ` David Malcolm 2019-01-01 0:00 ` Andrea Corallo
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).