[PATCH, PR 67133] Always change gimple fntype in cgraph_edge::redirect_call_stmt_to_callee

[PATCH, PR 67133] Always change gimple fntype in cgraph_edge::redirect_call_stmt_to_callee

[Martin Jambor]

Hi,

even though PR 67133 has been avoided by a different patch, I believe
the patch below is the correct fix.  It modifies the function that
changes call statements according to call graph edges so that it
changes the fntype of the call statements also when
combined_args_to_skip is NULL.  This code path is taken for example
when a call is redirected to __builtin_unreachable and then the type
of the callee function is likely to mismatch with fntype of the
statement, which can confuse the compiler later on.

If we agree it is a good idea, I'd like to also propose a patch
making the gimple verifier check whether fntypes of direct call
statements match the types of the callee (or at least that they have
the same number of same-typed arguments).

The patch has been bootstrapped and tested on x86_64-linux, the
testcase is already checked in.  OK for trunk?

Thanks,

Martin


2015-08-17  Martin Jambor  <mjambor@suse.cz>

	PR middle-end/67133
	* cgraph.c (redirect_call_stmt_to_callee): Set gimple call fntype also
	when redirecting without removing any parameters.

diff --git a/gcc/cgraph.c b/gcc/cgraph.c
index 22a9852..5e5b308 100644
--- a/gcc/cgraph.c
+++ b/gcc/cgraph.c
@@ -1461,6 +1461,7 @@ cgraph_edge::redirect_call_stmt_to_callee (void)
     {
       new_stmt = e->call_stmt;
       gimple_call_set_fndecl (new_stmt, e->callee->decl);
+      gimple_call_set_fntype (new_stmt, TREE_TYPE (e->callee->decl));
       update_stmt_fn (DECL_STRUCT_FUNCTION (e->caller->decl), new_stmt);
     }
 

Re: [PATCH, PR 67133] Always change gimple fntype in cgraph_edge::redirect_call_stmt_to_callee

[Jan Hubicka]

> Hi,
> 
> even though PR 67133 has been avoided by a different patch, I believe
> the patch below is the correct fix.  It modifies the function that
> changes call statements according to call graph edges so that it
> changes the fntype of the call statements also when
> combined_args_to_skip is NULL.  This code path is taken for example
> when a call is redirected to __builtin_unreachable and then the type
> of the callee function is likely to mismatch with fntype of the
> statement, which can confuse the compiler later on.
> 
> If we agree it is a good idea, I'd like to also propose a patch
> making the gimple verifier check whether fntypes of direct call
> statements match the types of the callee (or at least that they have
> the same number of same-typed arguments).
> 
> The patch has been bootstrapped and tested on x86_64-linux, the
> testcase is already checked in.  OK for trunk?
> 
> Thanks,
> 
> Martin
> 
> 
> 2015-08-17  Martin Jambor  <mjambor@suse.cz>
> 
> 	PR middle-end/67133
> 	* cgraph.c (redirect_call_stmt_to_callee): Set gimple call fntype also
> 	when redirecting without removing any parameters.

This makes sense in the case of __builtin_unreachable.  I wonder if we have
some problems in cases where LTO or indirect call code places in incompatible
declaration.

Patch is fine with me, but I would like Richi to have final word on this one.

Honza
> 
> diff --git a/gcc/cgraph.c b/gcc/cgraph.c
> index 22a9852..5e5b308 100644
> --- a/gcc/cgraph.c
> +++ b/gcc/cgraph.c
> @@ -1461,6 +1461,7 @@ cgraph_edge::redirect_call_stmt_to_callee (void)
>      {
>        new_stmt = e->call_stmt;
>        gimple_call_set_fndecl (new_stmt, e->callee->decl);
> +      gimple_call_set_fntype (new_stmt, TREE_TYPE (e->callee->decl));
>        update_stmt_fn (DECL_STRUCT_FUNCTION (e->caller->decl), new_stmt);
>      }
>  

Re: [PATCH, PR 67133] Always change gimple fntype in cgraph_edge::redirect_call_stmt_to_callee

[Richard Biener]

On Mon, Aug 17, 2015 at 3:47 PM, Jan Hubicka <hubicka@ucw.cz> wrote:
>> Hi,
>>
>> even though PR 67133 has been avoided by a different patch, I believe
>> the patch below is the correct fix.  It modifies the function that
>> changes call statements according to call graph edges so that it
>> changes the fntype of the call statements also when
>> combined_args_to_skip is NULL.  This code path is taken for example
>> when a call is redirected to __builtin_unreachable and then the type
>> of the callee function is likely to mismatch with fntype of the
>> statement, which can confuse the compiler later on.
>>
>> If we agree it is a good idea, I'd like to also propose a patch
>> making the gimple verifier check whether fntypes of direct call
>> statements match the types of the callee (or at least that they have
>> the same number of same-typed arguments).
>>
>> The patch has been bootstrapped and tested on x86_64-linux, the
>> testcase is already checked in.  OK for trunk?
>>
>> Thanks,
>>
>> Martin
>>
>>
>> 2015-08-17  Martin Jambor  <mjambor@suse.cz>
>>
>>       PR middle-end/67133
>>       * cgraph.c (redirect_call_stmt_to_callee): Set gimple call fntype also
>>       when redirecting without removing any parameters.
>
> This makes sense in the case of __builtin_unreachable.  I wonder if we have
> some problems in cases where LTO or indirect call code places in incompatible
> declaration.
>
> Patch is fine with me, but I would like Richi to have final word on this one.

I don't like it too much - you'll scribble over users ABI choice here.
It's better
to guard inspectors of the call properly to _not_ expect actual arguments
according to the ABI.

Richard.

> Honza
>>
>> diff --git a/gcc/cgraph.c b/gcc/cgraph.c
>> index 22a9852..5e5b308 100644
>> --- a/gcc/cgraph.c
>> +++ b/gcc/cgraph.c
>> @@ -1461,6 +1461,7 @@ cgraph_edge::redirect_call_stmt_to_callee (void)
>>      {
>>        new_stmt = e->call_stmt;
>>        gimple_call_set_fndecl (new_stmt, e->callee->decl);
>> +      gimple_call_set_fntype (new_stmt, TREE_TYPE (e->callee->decl));
>>        update_stmt_fn (DECL_STRUCT_FUNCTION (e->caller->decl), new_stmt);
>>      }
>>