diff mbox

[CHKP] Fix instrumented indirect calls with propagated pointers

Message ID CAFiYyc2b3GBhtGk4=ayRqZj1Y+KfhJHwDhd5GQvaVMz0_4vc=w@mail.gmail.com
State New
Headers show

Commit Message

Richard Biener March 25, 2015, 9:38 a.m. UTC 
On Wed, Mar 25, 2015 at 9:50 AM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote:
> 2015-03-24 17:40 GMT+03:00 Richard Biener <richard.guenther@gmail.com>:
>> On Tue, Mar 24, 2015 at 3:06 PM, Jakub Jelinek <jakub@redhat.com> wrote:
>>> On Tue, Mar 24, 2015 at 12:22:27PM +0300, Ilya Enkovich wrote:
>>>
>>> The question is what you want to do in the LTO case for the different cases,
>>> in particular a TU compiled with -fcheck-pointer-bounds and LTO link without
>>> that, or TU compiled without -fcheck-pointer-bounds and LTO link with it.
>>> It could be handled as LTO incompatible option, where lto1 would error out
>>> if you try to mix -fcheck-pointer-bounds with -fno-check-pointer-bounds
>>> code, or e.g. similar to var-tracking, you could consider adjusting the IL
>>> upon LTO reading if if some TU has been built with -fcheck-pointer-bounds
>>> and the LTO link is -fno-check-pointer-bounds.  Dunno what will happen
>>> with -fno-check-pointer-bounds TUs LTO linked with -fcheck-pointer-bounds.
>>> Or another possibility is to or in -fcheck-pointer-bounds from all TUs.
>>>
>>>> Maybe replace attribute usage with a new flag in tree_decl_with_vis structure?
>>>
>>> Depends, might be better to stick it into cgraph_node instead, depends on
>>> whether you are querying it already early in the FEs or just during GIMPLE
>>> when the cgraph node should be created too.
>>
>> I also wonder why it is necessary to execute pass_chkp_instrumentation_passes
>> when mpx is not active.
>>
>> That is, can we guard that properly in
>>
>> void
>> pass_manager::execute_early_local_passes ()
>> {
>>   execute_pass_list (cfun, pass_build_ssa_passes_1->sub);
>>   execute_pass_list (cfun, pass_chkp_instrumentation_passes_1->sub);
>>   execute_pass_list (cfun, pass_local_optimization_passes_1->sub);
>> }
>
> I'm worried about new functions generated in LTO. But with re-created
> flag_check_pointer_bounds it should be safe to guard it.
>
>>
>> (why's that so oddly wrapped?)
>>
>> class pass_chkp_instrumentation_passes
>>
>> also has no gate that guards with flag_mpx or so.
>>
>> That would save a IL walk over all functions (fixup_cfg) and a cgraph
>> edge rebuild.
>
> Right. Will fix it.

I am already testing



Richard.

> Thanks,
> Ilya
>
>>
>> Richard.
>>
>>>         Jakub

Comments

Jakub Jelinek March 25, 2015, 9:50 a.m. UTC | #1 
On Wed, Mar 25, 2015 at 10:38:56AM +0100, Richard Biener wrote:
> --- gcc/passes.c        (revision 221633)
> +++ gcc/passes.c        (working copy)
> @@ -156,7 +156,8 @@ void
>  pass_manager::execute_early_local_passes ()
>  {
>    execute_pass_list (cfun, pass_build_ssa_passes_1->sub);
> -  execute_pass_list (cfun, pass_chkp_instrumentation_passes_1->sub);
> +  if (flag_check_pointer_bounds)
> +    execute_pass_list (cfun, pass_chkp_instrumentation_passes_1->sub);
>    execute_pass_list (cfun, pass_local_optimization_passes_1->sub);
>  }
> 
> @@ -424,7 +425,8 @@ public:
>    virtual bool gate (function *)
>      {
>        /* Don't bother doing anything if the program has errors.  */
> -      return (!seen_error () && !in_lto_p);
> +      return (flag_check_pointer_bounds
> +             && !seen_error () && !in_lto_p);
>      }
> 
>  }; // class pass_chkp_instrumentation_passes

There is still the wasteful pass_fixup_cfg at the start of:
PUSH_INSERT_PASSES_WITHIN (pass_local_optimization_passes)
  NEXT_PASS (pass_fixup_cfg);
which wasn't there before chkp.  Perhaps this should be a different
pass with the same execute method, but gate containing
flag_check_pointer_bounds?

	Jakub
Ilya Enkovich March 25, 2015, 10:06 a.m. UTC | #2 
2015-03-25 12:50 GMT+03:00 Jakub Jelinek <jakub@redhat.com>:
> On Wed, Mar 25, 2015 at 10:38:56AM +0100, Richard Biener wrote:
>> --- gcc/passes.c        (revision 221633)
>> +++ gcc/passes.c        (working copy)
>> @@ -156,7 +156,8 @@ void
>>  pass_manager::execute_early_local_passes ()
>>  {
>>    execute_pass_list (cfun, pass_build_ssa_passes_1->sub);
>> -  execute_pass_list (cfun, pass_chkp_instrumentation_passes_1->sub);
>> +  if (flag_check_pointer_bounds)
>> +    execute_pass_list (cfun, pass_chkp_instrumentation_passes_1->sub);
>>    execute_pass_list (cfun, pass_local_optimization_passes_1->sub);
>>  }
>>
>> @@ -424,7 +425,8 @@ public:
>>    virtual bool gate (function *)
>>      {
>>        /* Don't bother doing anything if the program has errors.  */
>> -      return (!seen_error () && !in_lto_p);
>> +      return (flag_check_pointer_bounds
>> +             && !seen_error () && !in_lto_p);
>>      }
>>
>>  }; // class pass_chkp_instrumentation_passes
>
> There is still the wasteful pass_fixup_cfg at the start of:
> PUSH_INSERT_PASSES_WITHIN (pass_local_optimization_passes)
>   NEXT_PASS (pass_fixup_cfg);
> which wasn't there before chkp.  Perhaps this should be a different
> pass with the same execute method, but gate containing
> flag_check_pointer_bounds?

IIRC the reason for this pass was a different passes split, not
instrumentation itself. Previously function processing always started
with pass_fixup_cfg. Splitting processing into three stages we got
three pass_fixup_cfg passes.

Ilya

>
>         Jakub
Jakub Jelinek March 25, 2015, 10:11 a.m. UTC | #3 
On Wed, Mar 25, 2015 at 01:06:46PM +0300, Ilya Enkovich wrote:
> > There is still the wasteful pass_fixup_cfg at the start of:
> > PUSH_INSERT_PASSES_WITHIN (pass_local_optimization_passes)
> >   NEXT_PASS (pass_fixup_cfg);
> > which wasn't there before chkp.  Perhaps this should be a different
> > pass with the same execute method, but gate containing
> > flag_check_pointer_bounds?
> 
> IIRC the reason for this pass was a different passes split, not
> instrumentation itself. Previously function processing always started
> with pass_fixup_cfg. Splitting processing into three stages we got
> three pass_fixup_cfg passes.

Sure, but it would be really nice if for !flag_check_pointer_bounds
we really could have just one stage again, rather than 3.
When it is a global option, and for LTO ideally ored in from all the TUs,
that shouldn't be that hard...

	Jakub
Richard Biener March 25, 2015, 10:15 a.m. UTC | #4 
On Wed, Mar 25, 2015 at 10:50 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Wed, Mar 25, 2015 at 10:38:56AM +0100, Richard Biener wrote:
>> --- gcc/passes.c        (revision 221633)
>> +++ gcc/passes.c        (working copy)
>> @@ -156,7 +156,8 @@ void
>>  pass_manager::execute_early_local_passes ()
>>  {
>>    execute_pass_list (cfun, pass_build_ssa_passes_1->sub);
>> -  execute_pass_list (cfun, pass_chkp_instrumentation_passes_1->sub);
>> +  if (flag_check_pointer_bounds)
>> +    execute_pass_list (cfun, pass_chkp_instrumentation_passes_1->sub);
>>    execute_pass_list (cfun, pass_local_optimization_passes_1->sub);
>>  }
>>
>> @@ -424,7 +425,8 @@ public:
>>    virtual bool gate (function *)
>>      {
>>        /* Don't bother doing anything if the program has errors.  */
>> -      return (!seen_error () && !in_lto_p);
>> +      return (flag_check_pointer_bounds
>> +             && !seen_error () && !in_lto_p);
>>      }
>>
>>  }; // class pass_chkp_instrumentation_passes
>
> There is still the wasteful pass_fixup_cfg at the start of:
> PUSH_INSERT_PASSES_WITHIN (pass_local_optimization_passes)
>   NEXT_PASS (pass_fixup_cfg);
> which wasn't there before chkp.  Perhaps this should be a different
> pass with the same execute method, but gate containing
> flag_check_pointer_bounds?

That's not wasteful but required due to local_pure_const.  The remaining
wasteful fixup_cfg is the one in pass_build_ssa_passes.  ISTR
that pass_ipa_chkp_versioning/early_produce_thunks makes that one
required?  Or EH / CFG cleanup stuff makes it necessary to not
fail IL checking done by into-SSA.

Richard.

>         Jakub
Richard Biener March 25, 2015, 10:20 a.m. UTC | #5 
On Wed, Mar 25, 2015 at 11:11 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Wed, Mar 25, 2015 at 01:06:46PM +0300, Ilya Enkovich wrote:
>> > There is still the wasteful pass_fixup_cfg at the start of:
>> > PUSH_INSERT_PASSES_WITHIN (pass_local_optimization_passes)
>> >   NEXT_PASS (pass_fixup_cfg);
>> > which wasn't there before chkp.  Perhaps this should be a different
>> > pass with the same execute method, but gate containing
>> > flag_check_pointer_bounds?
>>
>> IIRC the reason for this pass was a different passes split, not
>> instrumentation itself. Previously function processing always started
>> with pass_fixup_cfg. Splitting processing into three stages we got
>> three pass_fixup_cfg passes.
>
> Sure, but it would be really nice if for !flag_check_pointer_bounds
> we really could have just one stage again, rather than 3.
> When it is a global option, and for LTO ideally ored in from all the TUs,
> that shouldn't be that hard...

LTO doesn't even run all this stuff at it only runs before LTO streaming.

I don't think we want to go back to not going into SSA for all functions
before early-opts (esp. early inlining).  Which unfortunately won't
get the EH cleanup related benefits.

Btw, execute_fixup_cfg can be optimized as well - edge purging only
needs to be done for the last stmt of a BB.

Richard.

>         Jakub
Ilya Enkovich March 25, 2015, 10:24 a.m. UTC | #6 
2015-03-25 13:15 GMT+03:00 Richard Biener <richard.guenther@gmail.com>:
> On Wed, Mar 25, 2015 at 10:50 AM, Jakub Jelinek <jakub@redhat.com> wrote:
>> On Wed, Mar 25, 2015 at 10:38:56AM +0100, Richard Biener wrote:
>>> --- gcc/passes.c        (revision 221633)
>>> +++ gcc/passes.c        (working copy)
>>> @@ -156,7 +156,8 @@ void
>>>  pass_manager::execute_early_local_passes ()
>>>  {
>>>    execute_pass_list (cfun, pass_build_ssa_passes_1->sub);
>>> -  execute_pass_list (cfun, pass_chkp_instrumentation_passes_1->sub);
>>> +  if (flag_check_pointer_bounds)
>>> +    execute_pass_list (cfun, pass_chkp_instrumentation_passes_1->sub);
>>>    execute_pass_list (cfun, pass_local_optimization_passes_1->sub);
>>>  }
>>>
>>> @@ -424,7 +425,8 @@ public:
>>>    virtual bool gate (function *)
>>>      {
>>>        /* Don't bother doing anything if the program has errors.  */
>>> -      return (!seen_error () && !in_lto_p);
>>> +      return (flag_check_pointer_bounds
>>> +             && !seen_error () && !in_lto_p);
>>>      }
>>>
>>>  }; // class pass_chkp_instrumentation_passes
>>
>> There is still the wasteful pass_fixup_cfg at the start of:
>> PUSH_INSERT_PASSES_WITHIN (pass_local_optimization_passes)
>>   NEXT_PASS (pass_fixup_cfg);
>> which wasn't there before chkp.  Perhaps this should be a different
>> pass with the same execute method, but gate containing
>> flag_check_pointer_bounds?
>
> That's not wasteful but required due to local_pure_const.  The remaining
> wasteful fixup_cfg is the one in pass_build_ssa_passes.  ISTR
> that pass_ipa_chkp_versioning/early_produce_thunks makes that one
> required?  Or EH / CFG cleanup stuff makes it necessary to not
> fail IL checking done by into-SSA.

These two chkp passes don't modify function bodies (mat remove it
though). I don't expect them to require following fixup_cfg.

Ilya

>
> Richard.
>
>>         Jakub
diff mbox

Patch

Index: gcc/passes.c
===================================================================
--- gcc/passes.c        (revision 221633)
+++ gcc/passes.c        (working copy)
@@ -156,7 +156,8 @@  void
 pass_manager::execute_early_local_passes ()
 {
   execute_pass_list (cfun, pass_build_ssa_passes_1->sub);
-  execute_pass_list (cfun, pass_chkp_instrumentation_passes_1->sub);
+  if (flag_check_pointer_bounds)
+    execute_pass_list (cfun, pass_chkp_instrumentation_passes_1->sub);
   execute_pass_list (cfun, pass_local_optimization_passes_1->sub);
 }

@@ -424,7 +425,8 @@  public:
   virtual bool gate (function *)
     {
       /* Don't bother doing anything if the program has errors.  */
-      return (!seen_error () && !in_lto_p);
+      return (flag_check_pointer_bounds
+             && !seen_error () && !in_lto_p);
     }

 }; // class pass_chkp_instrumentation_passes