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.cora...@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.cora...@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
