| Message ID | 20150817133857.GY2569@virgil.suse.cz |
|---|---|
| State | New |
| Headers | show |
> 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); > } >
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); >> } >>
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); }