diff mbox series

libgccjit: check result_type in gcc_jit_context_new_binary_op

Message ID gkra7epx0lv.fsf@arm.com
State New
Headers show
Series libgccjit: check result_type in gcc_jit_context_new_binary_op | expand

Commit Message

Andrea Corallo June 10, 2019, 9:15 a.m. UTC 
Hi all,
I would like to propose this patch to check for the return type of
binary operators to be a numeric type.
Not doing so can lead the compiler into funny crashes.

Does not introduce regressions running make check-jit.

OK for trunk?

Bests
  Andrea

2019-06-09  Andrea Corallo  andrea.corallo@arm.com

* libgccjit.c (gcc_jit_context_new_binary_op): Check result_type to be a
numeric type.

Comments

David Malcolm June 18, 2019, 10:31 p.m. UTC | #1 
On Mon, 2019-06-10 at 09:15 +0000, Andrea Corallo wrote:
> Hi all,
> I would like to propose this patch to check for the return type of
> binary operators to be a numeric type.
> Not doing so can lead the compiler into funny crashes.
> 
> Does not introduce regressions running make check-jit.
> 
> OK for trunk?
> 
> Bests
>   Andrea
> 
> 2019-06-09  Andrea Corallo  andrea.corallo@arm.com
> 
> * libgccjit.c (gcc_jit_context_new_binary_op): Check result_type to
> be a
> numeric type.

Thanks for this patch.  Please can you add a test case that triggers
the error-handling case?  (I'm trying to imagine a situation where this
could have happened).

See gcc/testsuite/jit.dg/test-error-new-binary-op-bad-op.c or similar.

Dave
Andrea Corallo June 24, 2019, 3:30 p.m. UTC | #2 
Hi all,
second version for this patch.
Given the suggestion for the bit-field one I've tried to improve also
here the error message.
I've added a simple testcase as requested, here I'm trying to do
*void=int+int.
This without checking would normally crash verifying gimple.
More complex cases can be cause of crashes having the
result type structures etc...

Tested with make check-jit
OK for trunk?

Bests
  Andrea

2019-06-09  Andrea Corallo  andrea.corallo@arm.com

* libgccjit.c (gcc_jit_context_new_binary_op): Check result_type to be a
numeric type.


2019-06-20  Andrea Corallo andrea.corallo@arm.com

* jit.dg/test-error-gcc_jit_context_new_binary_op-bad-res-type.c:
New testcase.
David Malcolm June 24, 2019, 3:52 p.m. UTC | #3 
On Mon, 2019-06-24 at 15:30 +0000, Andrea Corallo wrote:
> Hi all,
> second version for this patch.
> Given the suggestion for the bit-field one I've tried to improve also
> here the error message.

Thanks.

> I've added a simple testcase as requested, here I'm trying to do
> *void=int+int.
> This without checking would normally crash verifying gimple.

Thanks.  FWIW, I think the testcase can be simplified slightly, in that
all that's needed is a bogus call to gcc_jit_context_new_binary_op, so
I don't think the testcase needs the calls to:
  gcc_jit_context_new_function,
  gcc_jit_function_new_block, and
  gcc_jit_block_end_with_return,
it just needs the types and the gcc_jit_context_new_binary_op call.

> More complex cases can be cause of crashes having the
> result type structures etc...
> 
> Tested with make check-jit
> OK for trunk?

Looks good as-is, or you may prefer to simplify the testcase.

Thanks for the patch.

BTW, I don't see you listed in the MAINTAINERS file; are you able to
commit patches yourself?

Dave

> Bests
>   Andrea
> 
> 2019-06-09  Andrea Corallo  andrea.corallo@arm.com
> 
> * libgccjit.c (gcc_jit_context_new_binary_op): Check result_type to
> be a
> numeric type.
> 
> 
> 2019-06-20  Andrea Corallo andrea.corallo@arm.com
> 
> * jit.dg/test-error-gcc_jit_context_new_binary_op-bad-res-type.c:
> New testcase.
Andrea Corallo June 24, 2019, 4:37 p.m. UTC | #4 
David Malcolm writes:

> On Mon, 2019-06-24 at 15:30 +0000, Andrea Corallo wrote:
>> Hi all,
>> second version for this patch.
>> Given the suggestion for the bit-field one I've tried to improve also
>> here the error message.
>
> Thanks.
>
>> I've added a simple testcase as requested, here I'm trying to do
>> *void=int+int.
>> This without checking would normally crash verifying gimple.
>
> Thanks.  FWIW, I think the testcase can be simplified slightly, in that
> all that's needed is a bogus call to gcc_jit_context_new_binary_op, so
> I don't think the testcase needs the calls to:
>   gcc_jit_context_new_function,
>   gcc_jit_function_new_block, and
>   gcc_jit_block_end_with_return,
> it just needs the types and the gcc_jit_context_new_binary_op call.

Hi Dave,
thanks for your feedback.
I've tried that but the reproducer is then incomplete with no call to
gcc_jit_context_new_binary_op so I would keep it like it is if you are
ok with that.

>> More complex cases can be cause of crashes having the
>> result type structures etc...
>>
>> Tested with make check-jit
>> OK for trunk?
>
> Looks good as-is, or you may prefer to simplify the testcase.
>
> Thanks for the patch.
>
> BTW, I don't see you listed in the MAINTAINERS file; are you able to
> commit patches yourself?
>
> Dave

Sorry I realize my "OK for trunk?" was quite misleading.
I'm not a maintainer and till now I have now write access so I can't
apply patches myself.

Bests
  Andrea

>> Bests
>>   Andrea
>>
>> 2019-06-09  Andrea Corallo  andrea.corallo@arm.com
>>
>> * libgccjit.c (gcc_jit_context_new_binary_op): Check result_type to
>> be a
>> numeric type.
>>
>>
>> 2019-06-20  Andrea Corallo andrea.corallo@arm.com
>>
>> * jit.dg/test-error-gcc_jit_context_new_binary_op-bad-res-type.c:
>> New testcase.
David Malcolm June 24, 2019, 8:44 p.m. UTC | #5 
On Mon, 2019-06-24 at 16:37 +0000, Andrea Corallo wrote:
> David Malcolm writes:
> 
> > On Mon, 2019-06-24 at 15:30 +0000, Andrea Corallo wrote:
> > > Hi all,
> > > second version for this patch.
> > > Given the suggestion for the bit-field one I've tried to improve
> > > also
> > > here the error message.
> > 
> > Thanks.
> > 
> > > I've added a simple testcase as requested, here I'm trying to do
> > > *void=int+int.
> > > This without checking would normally crash verifying gimple.
> > 
> > Thanks.  FWIW, I think the testcase can be simplified slightly, in
> > that
> > all that's needed is a bogus call to gcc_jit_context_new_binary_op,
> > so
> > I don't think the testcase needs the calls to:
> >   gcc_jit_context_new_function,
> >   gcc_jit_function_new_block, and
> >   gcc_jit_block_end_with_return,
> > it just needs the types and the gcc_jit_context_new_binary_op call.
> 
> Hi Dave,
> thanks for your feedback.
> I've tried that but the reproducer is then incomplete with no call to
> gcc_jit_context_new_binary_op so I would keep it like it is if you
> are
> ok with that.

Sorry, I think I was unclear.

What I meant is that I think you can remove the calls I mentioned, but
keep the call to gcc_jit_context_new_binary_op, moving it to be a "top-
level" call within create_code (discarding the result).  That ought to
be enough to trigger the error within the gcc_jit_context.

Does that make more sense?

> > > More complex cases can be cause of crashes having the
> > > result type structures etc...
> > > 
> > > Tested with make check-jit
> > > OK for trunk?
> > 
> > Looks good as-is, or you may prefer to simplify the testcase.
> > 
> > Thanks for the patch.
> > 
> > BTW, I don't see you listed in the MAINTAINERS file; are you able
> > to
> > commit patches yourself?
> > 
> > Dave
> 
> Sorry I realize my "OK for trunk?" was quite misleading.
> I'm not a maintainer and till now I have now write access so I can't
> apply patches myself.

I believe ARM has a corporate copyright-assignment in place with the
FSF for GCC contributions.

I can commit the patch myself; alternatively, do you want to get commit
access?

Dave
Andrea Corallo June 24, 2019, 9:50 p.m. UTC | #6 
David Malcolm writes:

> On Mon, 2019-06-24 at 16:37 +0000, Andrea Corallo wrote:
>> David Malcolm writes:
>>
>> > On Mon, 2019-06-24 at 15:30 +0000, Andrea Corallo wrote:
>> > > Hi all,
>> > > second version for this patch.
>> > > Given the suggestion for the bit-field one I've tried to improve
>> > > also
>> > > here the error message.
>> >
>> > Thanks.
>> >
>> > > I've added a simple testcase as requested, here I'm trying to do
>> > > *void=int+int.
>> > > This without checking would normally crash verifying gimple.
>> >
>> > Thanks.  FWIW, I think the testcase can be simplified slightly, in
>> > that
>> > all that's needed is a bogus call to gcc_jit_context_new_binary_op,
>> > so
>> > I don't think the testcase needs the calls to:
>> >   gcc_jit_context_new_function,
>> >   gcc_jit_function_new_block, and
>> >   gcc_jit_block_end_with_return,
>> > it just needs the types and the gcc_jit_context_new_binary_op call.
>>
>> Hi Dave,
>> thanks for your feedback.
>> I've tried that but the reproducer is then incomplete with no call to
>> gcc_jit_context_new_binary_op so I would keep it like it is if you
>> are
>> ok with that.
>
> Sorry, I think I was unclear.
>
> What I meant is that I think you can remove the calls I mentioned, but
> keep the call to gcc_jit_context_new_binary_op, moving it to be a "top-
> level" call within create_code (discarding the result).  That ought to
> be enough to trigger the error within the gcc_jit_context.
>
> Does that make more sense?

Hi,
sorry yes it absolutely does.
What I meant is that in the test I did without these calls the produced
reproducer
test-error-gcc_jit_context_new_binary_op-bad-res-type.c.exe.reproducer.c
was without the call to gcc_jit_context_new_binary_op.
At the beginning I thought was due the removal of these other calls but
I've just realized the obvious fact that we do not record at all if we catch an
error there while recording... and that's the reason why the reproducer
is without the call itself.
By the way we could probably make this more clear in the
gcc_jit_context_dump_reproducer_to_file doc.

>> > > More complex cases can be cause of crashes having the
>> > > result type structures etc...
>> > >
>> > > Tested with make check-jit
>> > > OK for trunk?
>> >
>> > Looks good as-is, or you may prefer to simplify the testcase.
>> >
>> > Thanks for the patch.
>> >
>> > BTW, I don't see you listed in the MAINTAINERS file; are you able
>> > to
>> > commit patches yourself?
>> >
>> > Dave
>>
>> Sorry I realize my "OK for trunk?" was quite misleading.
>> I'm not a maintainer and till now I have now write access so I can't
>> apply patches myself.
>
> I believe ARM has a corporate copyright-assignment in place with the
> FSF for GCC contributions.

Correct, is thanks to that I was able to contribute in the past.

> I can commit the patch myself; alternatively, do you want to get commit
> access?
>
> Dave

I think if I could get commit access would be great and certainly
easier for everybody for the future.

Thanks for the feedbacks on both the patches, I'll be able to update
them most likely tomorrow.

Bests
  Andrea
Andrea Corallo June 25, 2019, 8:11 a.m. UTC | #7 
Hi,
third version for this patch with the simplified test.

make check-jit pass clean

Bests
  Andrea

2019-06-09  Andrea Corallo  andrea.corallo@arm.com

* libgccjit.c (gcc_jit_context_new_binary_op): Check result_type to be a
numeric type.


2019-06-20  Andrea Corallo andrea.corallo@arm.com

* jit.dg/test-error-gcc_jit_context_new_binary_op-bad-res-type.c:
New testcase.
David Malcolm June 26, 2019, 1:19 p.m. UTC | #8 
On Tue, 2019-06-25 at 08:11 +0000, Andrea Corallo wrote:
> Hi,
> third version for this patch with the simplified test.
> 
> make check-jit pass clean
> 
> Bests
>   Andrea
> 
> 2019-06-09  Andrea Corallo  andrea.corallo@arm.com
> 
> * libgccjit.c (gcc_jit_context_new_binary_op): Check result_type to
> be a
> numeric type.
> 
> 
> 2019-06-20  Andrea Corallo andrea.corallo@arm.com
> 
> * jit.dg/test-error-gcc_jit_context_new_binary_op-bad-res-type.c:
> New testcase.

Thanks for the updated patch.

This is good for trunk.

(Copying and pasting from my other review): are you working on getting
SVN commit access, or do you want me to commit your two patches on your
behalf?

Thanks
Dave
Andrea Corallo June 26, 2019, 3:05 p.m. UTC | #9 
David Malcolm writes:

> On Tue, 2019-06-25 at 08:11 +0000, Andrea Corallo wrote:
>> Hi,
>> third version for this patch with the simplified test.
>>
>> make check-jit pass clean
>>
>> Bests
>>   Andrea
>>
>> 2019-06-09  Andrea Corallo  andrea.corallo@arm.com
>>
>> * libgccjit.c (gcc_jit_context_new_binary_op): Check result_type to
>> be a
>> numeric type.
>>
>>
>> 2019-06-20  Andrea Corallo andrea.corallo@arm.com
>>
>> * jit.dg/test-error-gcc_jit_context_new_binary_op-bad-res-type.c:
>> New testcase.
>
> Thanks for the updated patch.
>
> This is good for trunk.
>
> (Copying and pasting from my other review): are you working on getting
> SVN commit access, or do you want me to commit your two patches on your
> behalf?
>
> Thanks
> Dave

Hi David,
I can work on to get the SVN commit access.
As a maintainer has to sponsor it would you mind being the one?

Thanks
  Andrea
Andrea Corallo July 1, 2019, 8:49 a.m. UTC | #10 
Andrea Corallo writes:

> David Malcolm writes:
>
>> On Tue, 2019-06-25 at 08:11 +0000, Andrea Corallo wrote:
>>> Hi,
>>> third version for this patch with the simplified test.
>>>
>>> make check-jit pass clean
>>>
>>> Bests
>>>   Andrea
>>>
>>> 2019-06-09  Andrea Corallo  andrea.corallo@arm.com
>>>
>>> * libgccjit.c (gcc_jit_context_new_binary_op): Check result_type to
>>> be a
>>> numeric type.
>>>
>>>
>>> 2019-06-20  Andrea Corallo andrea.corallo@arm.com
>>>
>>> * jit.dg/test-error-gcc_jit_context_new_binary_op-bad-res-type.c:
>>> New testcase.
>>
>> Thanks for the updated patch.
>>
>> This is good for trunk.
>>
>> (Copying and pasting from my other review): are you working on getting
>> SVN commit access, or do you want me to commit your two patches on your
>> behalf?
>>
>> Thanks
>> Dave
>
> Hi David,
> I can work on to get the SVN commit access.
> As a maintainer has to sponsor it would you mind being the one?
>
> Thanks
>   Andrea

Hi David,
kind ping on this :)

Best regards
  Andrea
David Malcolm July 2, 2019, 2:46 p.m. UTC | #11 
On Wed, 2019-06-26 at 15:05 +0000, Andrea Corallo wrote:
> David Malcolm writes:
> 
> > On Tue, 2019-06-25 at 08:11 +0000, Andrea Corallo wrote:
> > > Hi,
> > > third version for this patch with the simplified test.
> > > 
> > > make check-jit pass clean
> > > 
> > > Bests
> > >   Andrea
> > > 
> > > 2019-06-09  Andrea Corallo  andrea.corallo@arm.com
> > > 
> > > * libgccjit.c (gcc_jit_context_new_binary_op): Check result_type
> > > to
> > > be a
> > > numeric type.
> > > 
> > > 
> > > 2019-06-20  Andrea Corallo andrea.corallo@arm.com
> > > 
> > > * jit.dg/test-error-gcc_jit_context_new_binary_op-bad-res-type.c:
> > > New testcase.
> > 
> > Thanks for the updated patch.
> > 
> > This is good for trunk.
> > 
> > (Copying and pasting from my other review): are you working on
> > getting
> > SVN commit access, or do you want me to commit your two patches on
> > your
> > behalf?
> > 
> > Thanks
> > Dave
> 
> Hi David,
> I can work on to get the SVN commit access.
> As a maintainer has to sponsor it would you mind being the one?

https://www.gnu.org/software/gcc/svnwrite.html says:
"a well-established GCC maintainer (including reviewers) can approve
for write access any person with GNU copyright assignment papers in
place and known to submit good patches."

Thanks for your patches so far.

I approve of you getting "Write after approval" access to GCC svn,
provided the copyright assignment paperwork is in place (which I'm
assuming is, based on your @arm.com email address).

I'm not quite sure what the next steps are.

Dave
Kyrill Tkachov July 2, 2019, 2:55 p.m. UTC | #12 
Hi David, Andrea,

On 7/2/19 3:46 PM, David Malcolm wrote:
> On Wed, 2019-06-26 at 15:05 +0000, Andrea Corallo wrote:
> > David Malcolm writes:
> >
> > > On Tue, 2019-06-25 at 08:11 +0000, Andrea Corallo wrote:
> > > > Hi,
> > > > third version for this patch with the simplified test.
> > > >
> > > > make check-jit pass clean
> > > >
> > > > Bests
> > > >   Andrea
> > > >
> > > > 2019-06-09  Andrea Corallo andrea.corallo@arm.com
> > > >
> > > > * libgccjit.c (gcc_jit_context_new_binary_op): Check result_type
> > > > to
> > > > be a
> > > > numeric type.
> > > >
> > > >
> > > > 2019-06-20  Andrea Corallo andrea.corallo@arm.com
> > > >
> > > > * jit.dg/test-error-gcc_jit_context_new_binary_op-bad-res-type.c:
> > > > New testcase.
> > >
> > > Thanks for the updated patch.
> > >
> > > This is good for trunk.
> > >
> > > (Copying and pasting from my other review): are you working on
> > > getting
> > > SVN commit access, or do you want me to commit your two patches on
> > > your
> > > behalf?
> > >
> > > Thanks
> > > Dave
> >
> > Hi David,
> > I can work on to get the SVN commit access.
> > As a maintainer has to sponsor it would you mind being the one?
>
> https://www.gnu.org/software/gcc/svnwrite.html says:
> "a well-established GCC maintainer (including reviewers) can approve
> for write access any person with GNU copyright assignment papers in
> place and known to submit good patches."
>
> Thanks for your patches so far.
>
> I approve of you getting "Write after approval" access to GCC svn,
> provided the copyright assignment paperwork is in place (which I'm
> assuming is, based on your @arm.com email address).
>
> I'm not quite sure what the next steps are.
>
It means Andrea needs to fill out the form at 
https://sourceware.org/cgi-bin/pdw/ps_form.cgi with his SSH key and 
putting your address as an approver. You'll then get a request to 
approve it.

Cheers,

Kyrill



> Dave
Andrea Corallo July 2, 2019, 3:27 p.m. UTC | #13 
David Malcolm writes:

>> Hi David,
>> I can work on to get the SVN commit access.
>> As a maintainer has to sponsor it would you mind being the one?
>
> https://www.gnu.org/software/gcc/svnwrite.html says:
> "a well-established GCC maintainer (including reviewers) can approve
> for write access any person with GNU copyright assignment papers in
> place and known to submit good patches."
>
> Thanks for your patches so far.
>
> I approve of you getting "Write after approval" access to GCC svn,
> provided the copyright assignment paperwork is in place (which I'm
> assuming is, based on your @arm.com email address).
>
> I'm not quite sure what the next steps are.
>
> Dave

Hi David,
great thanks.
I'll proceed with the following steps.

Bests
  Andrea
Andrea Corallo July 4, 2019, 4:34 p.m. UTC | #14 
Andrea Corallo writes:

> David Malcolm writes:
>
>>> Hi David,
>>> I can work on to get the SVN commit access.
>>> As a maintainer has to sponsor it would you mind being the one?
>>
>> https://www.gnu.org/software/gcc/svnwrite.html says:
>> "a well-established GCC maintainer (including reviewers) can approve
>> for write access any person with GNU copyright assignment papers in
>> place and known to submit good patches."
>>
>> Thanks for your patches so far.
>>
>> I approve of you getting "Write after approval" access to GCC svn,
>> provided the copyright assignment paperwork is in place (which I'm
>> assuming is, based on your @arm.com email address).
>>
>> I'm not quite sure what the next steps are.
>>
>> Dave
>
> Hi David,
> great thanks.
> I'll proceed with the following steps.
>
> Bests
>   Andrea

Hi,
I've pushed this patch into trunk as revision r273089.
Thanks again for all the feedbacks on both patches.

Bests
  Andrea
diff mbox series

Patch

diff --git a/gcc/jit/libgccjit.c b/gcc/jit/libgccjit.c
index e4f17f8..95a2c02 100644
--- a/gcc/jit/libgccjit.c
+++ b/gcc/jit/libgccjit.c
@@ -1345,6 +1345,10 @@  gcc_jit_context_new_binary_op (gcc_jit_context *ctxt,
     a->get_type ()->get_debug_string (),
     b->get_debug_string (),
     b->get_type ()->get_debug_string ());
+  RETURN_NULL_IF_FAIL_PRINTF1 (
+    result_type->is_numeric (), ctxt, loc,
+    "result_type type: %s is not numeric",
+    result_type->get_debug_string ());
 
   return (gcc_jit_rvalue *)ctxt->new_binary_op (loc, op, result_type, a, b);
 }