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
Related show

Commit Message

Andrea Corallo June 10, 2019, 9:15 a.m.
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. | #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. | #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.
diff --git a/gcc/jit/libgccjit.c b/gcc/jit/libgccjit.c
index e4f17f8..3507d0b 100644
--- a/gcc/jit/libgccjit.c
+++ b/gcc/jit/libgccjit.c
@@ -1345,6 +1345,12 @@ 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_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 (),
+    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
new file mode 100644
index 0000000..1addc67
--- /dev/null
+++ b/gcc/testsuite/jit.dg/test-error-gcc_jit_context_new_binary_op-bad-res-type.c
@@ -0,0 +1,52 @@
+#include <stdlib.h>
+#include <stdio.h>
+
+#include "libgccjit.h"
+
+#include "harness.h"
+
+/* Try to create a binary 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_function *func =
+    gcc_jit_context_new_function (ctxt, NULL,
+				  GCC_JIT_FUNCTION_EXPORTED,
+				  void_ptr_type,
+				  "foo_func",
+				  0, NULL, 0);
+  gcc_jit_block *block = gcc_jit_function_new_block (func, NULL);
+  gcc_jit_block_end_with_return (
+    block,
+    NULL,
+    gcc_jit_context_new_binary_op (
+      ctxt,
+      NULL,
+      GCC_JIT_BINARY_OP_MINUS,
+      void_ptr_type,
+      gcc_jit_context_new_rvalue_from_int (ctxt,
+					   int_type,
+					   1),
+      gcc_jit_context_new_rvalue_from_int (ctxt,
+					   int_type,
+					   2)));
+
+}
+
+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_binary_op: gcc_jit_binary_op 1 with"
+		      " operands a: (int)1 b: (int)2 has non numeric "
+		      "result_type: void *");
+}
David Malcolm June 24, 2019, 3:52 p.m. | #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. | #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. | #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. | #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. | #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.
diff --git a/gcc/jit/libgccjit.c b/gcc/jit/libgccjit.c
index e4f17f8..3507d0b 100644
--- a/gcc/jit/libgccjit.c
+++ b/gcc/jit/libgccjit.c
@@ -1345,6 +1345,12 @@ 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_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 (),
+    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
new file mode 100644
index 0000000..abadc9f
--- /dev/null
+++ b/gcc/testsuite/jit.dg/test-error-gcc_jit_context_new_binary_op-bad-res-type.c
@@ -0,0 +1,41 @@
+#include <stdlib.h>
+#include <stdio.h>
+
+#include "libgccjit.h"
+
+#include "harness.h"
+
+/* Try to create a binary 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_binary_op (
+    ctxt,
+    NULL,
+    GCC_JIT_BINARY_OP_MINUS,
+    void_ptr_type,
+    gcc_jit_context_new_rvalue_from_int (ctxt,
+					 int_type,
+					 1),
+    gcc_jit_context_new_rvalue_from_int (ctxt,
+					 int_type,
+					 2));
+}
+
+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_binary_op: gcc_jit_binary_op 1 with"
+		      " operands a: (int)1 b: (int)2 has non numeric "
+		      "result_type: void *");
+}

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);
 }