Re: [PATCH][gcc] libgccjit: check result_type in gcc_jit_context_new_binary_op

[Andrea Corallo <Andrea.Corallo@arm.com>]

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