From: Andrea Corallo <Andrea.Corallo@arm.com>
To: David Malcolm <dmalcolm@redhat.com>
Cc: Andrea Corallo <Andrea.Corallo@arm.com>,
"jit@gcc.gnu.org" <jit@gcc.gnu.org>,
"gcc-patches@gcc.gnu.org" <gcc-patches@gcc.gnu.org>,
nd <nd@arm.com>
Subject: Re: [PATCH][gcc] libgccjit: check result_type in gcc_jit_context_new_unary_op
Date: Tue, 01 Jan 2019 00:00:00 -0000 [thread overview]
Message-ID: <gkr1ryn1fdi.fsf@arm.com> (raw)
In-Reply-To: <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
next prev parent reply other threads:[~2019-07-18 16:23 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-01-01 0:00 Andrea Corallo
2019-01-01 0:00 ` David Malcolm
2019-01-01 0:00 ` Andrea Corallo
2019-01-01 0:00 ` Andrea Corallo [this message]
2019-01-01 0:00 ` David Malcolm
2019-01-01 0:00 ` Andrea Corallo
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=gkr1ryn1fdi.fsf@arm.com \
--to=andrea.corallo@arm.com \
--cc=dmalcolm@redhat.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=jit@gcc.gnu.org \
--cc=nd@arm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).