This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH][gcc] libgccjit: check result_type in gcc_jit_context_new_unary_op
- From: Andrea Corallo <Andrea dot Corallo at arm dot com>
- To: David Malcolm <dmalcolm at redhat dot com>
- Cc: Andrea Corallo <Andrea dot Corallo at arm dot com>, "jit at gcc dot gnu dot org" <jit at gcc dot gnu dot org>, "gcc-patches at gcc dot gnu dot org" <gcc-patches at gcc dot gnu dot org>, nd <nd at arm dot com>
- Date: Thu, 18 Jul 2019 16:23:13 +0000
- Subject: Re: [PATCH][gcc] libgccjit: check result_type in gcc_jit_context_new_unary_op
- Arc-authentication-results: i=1; mx.microsoft.com 1;spf=pass smtp.mailfrom=arm.com;dmarc=pass action=none header.from=arm.com;dkim=pass header.d=arm.com;arc=none
- Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=tm49C/tW3i5ZsHtn0JEIzwZw7VfbmvTECncEfiEfgmw=; b=UfO1z28UJumEjV3u6SxKlcUhuive9H2PV6Iqj0b0hWxGm+ckk3IWOUCzlVOK0ELb11utP2CjtYQmJ11gcCZOiPWsWQGT0WbSw2BCsGQiWdLM8dKXaQOw3mgDvbGv3dBXDY0QZOYMUPCYFkGhVmrStPfsfR2m31EqZskfbNHU67lkxgj5X+vXZyducSi+yhbF3Yr6t1jStwzBxqxJPVc4EHDIOeZOfDRC8mt39PiNJ11jlmihKwgYfcdOIFFJ5ozoUezdiamazUdp3H/PILvyXLYJi+iKIhraeb5edXzhOCFwnCvB/wtDyhwGiph1jSBDfsSvGwLjQYnViD5kfyUY5g==
- Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=H9sJwxiSV5rlukXG1lYdkS4KzbO4AUqzGIRjmbMjAWGUIaxrc0FfeHiEfhLVOtZ07PYJG21tFVGT5+bbpeLT9FU2s1dPSDs3vCwjz8ZkFJWEuh0lGwJiqN3eGNcKMLAdAQuR5NLO2N3n80P4WRG5gi2zEo5vUYwzY882LAyDI5J4nhrzLd9HXYZXRbWjt1WqCCcQnEwG04J7pO6tdvY5hYX3vRtJyV5N2Va28tIplmyJcV7SBEtlpBxVV8YbecVAkRfGaXoZCc2iDFFNftDsQvM/teJvPOlI4GAPjq7c+sA6b2VLqksK7fZbI9p6Ot4O+MqG5s8mucfAyYXtBiePYw==
- Original-authentication-results: spf=none (sender IP is ) smtp.mailfrom=Andrea dot Corallo at arm dot com;
- References: <gkr36j31l1s.fsf@arm.com> <1563466038.2530.92.camel@redhat.com>
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