PR78599

PR78599

[Prathamesh Kulkarni]

[-- Attachment #1: Type: text/plain, Size: 1574 bytes --]

Hi,
As mentioned in PR, the issue seems to be that in
propagate_bits_accross_jump_functions(),
ipa_get_type() returns record_type during WPA and hence we pass
invalid precision to
ipcp_bits_lattice::meet_with (value, mask, precision) which eventually
leads to runtime error.
The attached patch tries to fix that, by bailing out if type of param
is not integral or pointer type.
This happens for the edge from deque_test -> _Z4copyIPd1BEvT_S2_T0_.isra.0/9.

However I am not sure how ipcp_bits_lattice::meet_with (value, mask,
precision) gets called for this case. In
ipa_compute_jump_functions_for_edge(), we set jfunc->bits.known to
true only
if parm's type satisfies INTEGRAL_TYPE_P or POINTER_TYPE_P.
And ipcp_bits_lattice::meet_with (value, mask, precision) is called
only if jfunc->bits.known
is set to true. So I suppose it shouldn't really happen that
ipcp_bits_lattice::meet_with(value, mask, precision) gets called when
callee parameter's type is record_type, since the corresponding
argument's type would also need to be record_type and
jfunc->bits.known would be set to false.

Without -flto, parm_type is reference_type so that satisfies POINTER_TYPE_P,
but with -flto it's appearing to be record_type. Is this possibly the
same issue of TYPE_ARG_TYPES returning bogus types during WPA ?

I verified the attached patch fixes the runtime error with ubsan-built gcc.
Bootstrap+tested on x86_64-unknown-linux-gnu.
Cross-tested on arm*-*-*, aarch64*-*-*.
LTO bootstrap on x86_64-unknown-linux-gnu in progress.
Is it OK to commit if it succeeds ?

Thanks,
Prathamesh

[-- Attachment #2: pr78599-2.txt --]
[-- Type: text/plain, Size: 1123 bytes --]

2016-12-01  Prathamesh Kulkarni  <prathamesh.kulkarni@linaro.org>

	PR ipa/78599
	* ipa-cp.c (propagate_bits_accross_jump_function): Check if parm_type
	is integral or pointer type.
diff --git a/gcc/ipa-cp.c b/gcc/ipa-cp.c
index 2ec671f..28eb74c 100644
--- a/gcc/ipa-cp.c
+++ b/gcc/ipa-cp.c
@@ -1770,12 +1770,15 @@ propagate_bits_accross_jump_function (cgraph_edge *cs, int idx, ipa_jump_func *j
   tree parm_type = ipa_get_type (callee_info, idx);
 
   /* For K&R C programs, ipa_get_type() could return NULL_TREE.
-     Avoid the transform for these cases.  */
-  if (!parm_type)
+     Avoid the transform for these cases or if parm type is not
+     integral or pointer type.  */
+  if (!parm_type
+      || !(INTEGRAL_TYPE_P (parm_type) || POINTER_TYPE_P (parm_type)))
     {
       if (dump_file && (dump_flags & TDF_DETAILS))
 	fprintf (dump_file, "Setting dest_lattice to bottom, because"
-			    " param %i type is NULL for %s\n", idx,
+			    " param %i type is %s for %s\n", idx,
+			    (parm_type == NULL) ? "NULL" : "non-integral",
 			    cs->callee->name ());
 
       return dest_lattice->set_to_bottom ();

Re: PR78599

[Richard Biener]

On Thu, Dec 1, 2016 at 11:07 AM, Prathamesh Kulkarni
<prathamesh.kulkarni@linaro.org> wrote:
> Hi,
> As mentioned in PR, the issue seems to be that in
> propagate_bits_accross_jump_functions(),
> ipa_get_type() returns record_type during WPA and hence we pass
> invalid precision to
> ipcp_bits_lattice::meet_with (value, mask, precision) which eventually
> leads to runtime error.
> The attached patch tries to fix that, by bailing out if type of param
> is not integral or pointer type.
> This happens for the edge from deque_test -> _Z4copyIPd1BEvT_S2_T0_.isra.0/9.

Feels more like a DECL_BY_REFERENCE mishandling and should be fixed elsewhere.

> However I am not sure how ipcp_bits_lattice::meet_with (value, mask,
> precision) gets called for this case. In
> ipa_compute_jump_functions_for_edge(), we set jfunc->bits.known to
> true only
> if parm's type satisfies INTEGRAL_TYPE_P or POINTER_TYPE_P.
> And ipcp_bits_lattice::meet_with (value, mask, precision) is called
> only if jfunc->bits.known
> is set to true. So I suppose it shouldn't really happen that
> ipcp_bits_lattice::meet_with(value, mask, precision) gets called when
> callee parameter's type is record_type, since the corresponding
> argument's type would also need to be record_type and
> jfunc->bits.known would be set to false.
>
> Without -flto, parm_type is reference_type so that satisfies POINTER_TYPE_P,
> but with -flto it's appearing to be record_type. Is this possibly the
> same issue of TYPE_ARG_TYPES returning bogus types during WPA ?
>
> I verified the attached patch fixes the runtime error with ubsan-built gcc.
> Bootstrap+tested on x86_64-unknown-linux-gnu.
> Cross-tested on arm*-*-*, aarch64*-*-*.
> LTO bootstrap on x86_64-unknown-linux-gnu in progress.
> Is it OK to commit if it succeeds ?
>
> Thanks,
> Prathamesh

Re: PR78599

[Martin Jambor]

Hi,

On Thu, Dec 01, 2016 at 01:43:16PM +0100, Richard Biener wrote:
> On Thu, Dec 1, 2016 at 11:07 AM, Prathamesh Kulkarni
> <prathamesh.kulkarni@linaro.org> wrote:
> > Hi,
> > As mentioned in PR, the issue seems to be that in
> > propagate_bits_accross_jump_functions(),
> > ipa_get_type() returns record_type during WPA and hence we pass
> > invalid precision to
> > ipcp_bits_lattice::meet_with (value, mask, precision) which eventually
> > leads to runtime error.
> > The attached patch tries to fix that, by bailing out if type of param
> > is not integral or pointer type.
> > This happens for the edge from deque_test -> _Z4copyIPd1BEvT_S2_T0_.isra.0/9.
> 
> Feels more like a DECL_BY_REFERENCE mishandling and should be fixed elsewhere.

That is indeed what is happening. Prathamesh, if you are going to save
the type of arguments in the jump function, it should help you also
with this issue.  I know it was me who suggested using the function
type to get at them and am sorry, I did not realize there potential
issues with promotions and by_reference passing.

By the way, please be careful not to introduce code style violations,
especially lines exceeding 80 characters and adding trailing
whitespace (propagate_bits_accross_jump_function has a few instances
of both), I'd suggest setting your editor to highlight them.

Thanks,

Martin

Re: PR78599

[Prathamesh Kulkarni]

On 3 December 2016 at 00:25, Martin Jambor <mjambor@suse.cz> wrote:
> Hi,
>
> On Thu, Dec 01, 2016 at 01:43:16PM +0100, Richard Biener wrote:
>> On Thu, Dec 1, 2016 at 11:07 AM, Prathamesh Kulkarni
>> <prathamesh.kulkarni@linaro.org> wrote:
>> > Hi,
>> > As mentioned in PR, the issue seems to be that in
>> > propagate_bits_accross_jump_functions(),
>> > ipa_get_type() returns record_type during WPA and hence we pass
>> > invalid precision to
>> > ipcp_bits_lattice::meet_with (value, mask, precision) which eventually
>> > leads to runtime error.
>> > The attached patch tries to fix that, by bailing out if type of param
>> > is not integral or pointer type.
>> > This happens for the edge from deque_test -> _Z4copyIPd1BEvT_S2_T0_.isra.0/9.
>>
>> Feels more like a DECL_BY_REFERENCE mishandling and should be fixed elsewhere.
>
> That is indeed what is happening. Prathamesh, if you are going to save
> the type of arguments in the jump function, it should help you also
> with this issue.  I know it was me who suggested using the function
> type to get at them and am sorry, I did not realize there potential
> issues with promotions and by_reference passing.
>
> By the way, please be careful not to introduce code style violations,
> especially lines exceeding 80 characters and adding trailing
> whitespace (propagate_bits_accross_jump_function has a few instances
> of both), I'd suggest setting your editor to highlight them.
Oops sorry about that, I will pay more attention to formatting henceforth.
Using editor to highlight stray whitespace is indeed quite useful,
thanks for that suggestion.

Kugan has a patch for adding param type to jump function:
https://gcc.gnu.org/ml/gcc-patches/2016-11/msg02732.html
Once that gets committed, I will send a patch to use jfunc->param_type in
propagate_bits_accross_jump_function().

Thanks,
Prathamesh
>
> Thanks,
>
> Martin
>