Re: [PATCH, MPX, 2/X] Pointers Checker [14/25] Function splitting

[Ilya Enkovich <enkovich dot gnu at gmail dot com>]

2013/11/26 Richard Biener <richard.guenther@gmail.com>:
> On Tue, Nov 26, 2013 at 12:18 PM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote:
>> 2013/11/26 Ilya Enkovich <enkovich.gnu@gmail.com>:
>>> 2013/11/25 Jeff Law <law@redhat.com>:
>>>> On 11/25/13 04:12, Ilya Enkovich wrote:
>>>>>
>>>>>
>>>>> I'll prepare a patch to remove committed patches.  But the first part
>>>>> of series added new ISA extension support.  It is independent from the
>>>>> checker.  Should it be OK to keep ISA in trunk?
>>>>
>>>> I think this can/should reasonably be Uros's call.
>>>>
>>>> I'm sorry we didn't get this moved far enough to make it into GCC 4.9; if I
>>>> had thought we were going run into these issues I wouldn't have suggested
>>>> you check in the preparatory patches only to have to back them out later.
>>>
>>> I also though we could make it go into 4.9.  And taking into account
>>> encountered problem I think a short time-out to reconsider some design
>>> elements would be useful.
>>>
>>> My next steps in bounds checker improvement include better support for
>>> optimization passes and generic target support, which would not
>>> require Intel MPX on hardware to use it.  One of the way is to keep
>>> going with the current model.  It has some problems but proved to
>>> work. The main problems are:
>>>   - Hidden data flow to pass bounds into functions.  We have implicit
>>> data flow for calls from bind_bounds call in caller and arg_bounds
>>> call in callee.  Optimization passes do not know about this flow and
>>> thus may easily corrupt it (especially IPA passes).
>>>   - Wrapped values.  All call arguments are wrapped by bind_bounds
>>> calls.  It prevents from some optimizations.  E.g. we cannot propagate
>>> null pointer value into callee if call is instrumented.
>>>
>>> Positive points here:
>>>   - we do not affect basic infrastructure significantly
>>>   - initial checker implementation does not require enabling optimization passes
>>>   - it is possible to enable optimization passes for better work with
>>> instrumented code; it may be done incrementally
>>>   - it is implemented and works including some optimizations enabling
>>>
>>> The other way I see is to try to make all current optimizations work
>>> correctly with bounds by making changes in the way we instrument the
>>> code.  If the main problem is usage of bind_bounds and arg_bounds,
>>> then we may try to avoid their usage.  The obvious way to do it is to
>>> use regular arg passing mechanism using call args and params in
>>> function decls.  The basic idea is to not modify function during
>>> instrumentation, but create a new one.  We may either keep the
>>> original version of the function (e.g. for inline into
>>> non-instrumented callers) or just leave it's declaration to be used
>>> for non-instrumented calls.
>>>
>>> Thus, all functions may result in two nodes in cgraph.  One for
>>> non-instrumented version and one for instrumented version.  Function
>>> declaration of the instrumented function would have all bounds in it's
>>> param list and instrumented call would have all bounds in it's args
>>> list accordingly.
>>>
>>> Advantage of this approach:
>>>   - It would decrease number of cases when bounds flow is damaged
>>>   - It would allow to avoid (or at least decrease) modifications in
>>> IPA passes to handle bounds correctly
>>>   - It would allow to unbind bounds from pointers and perform IPA
>>> optimizations for bound values (e.g. if we do not know pointer value
>>> but know pointer is unchecked, we may propagate it's bounds value into
>>> callee using existing propagation optimization)
>>>
>>> Surely some doubts also exist:
>>>   - We have two declarations for actually the same function
>>>   - Most probably need some changes in PARAM_DECLs to bind bounds
>>> params with pointer params
>>>   - If we Most probably need some modifications in passes which
>>> generate new function decls (e.g. propagation or splitting) to
>>> generate decl with correctly declared bounds
>>
>> Accidentally sent the letter before finished it :/
>>
>> But seems I wrote the most of I wanted to.  I know this scheme
>> description with double function declarations is very basic but I
>> would like to get your opinion before diving deeper.  Probably current
>> scheme is not so bad and we should leave basic things as it is?
>
> The idea of making instrumented functions clones of the original
> function sounds interesting.  Can you give some numbers (out of
> the current implementation) on how much code ends up being
> instrumented?  Like when compiling GCC itself for example?

I did not try to get some statistics. But current implementation
instruments every function having any manipulation with a pointer
which is almost every function.

>
> I can see there may be both instrumented and not instrumented
> calls to a function and thus having two versions of the callee
> might improve optimization of the non-instrumented call via
> IPA optimizations that you otherwise have to disable?

Currently we just always use either instrumented or not instrumented
function for both instrumented and non instrumented calls.  E.g. we
may inline instrumented function into not instrumented and vice-versa.
 Agree having two versions of callee would would be preferable.  It
may require significant amount of memory but suppose in the most cases
not instrumented function would just have no callers and may be
deleted.

>
> Generally making dataflow explicit is always prefered.  How does
> making it explicit on the GIMPLE / SSA level affect RTL?  (I
> have not looked at all into how you deal with bounds on the RTL
> level currently)

In RTL bounds dataflow is explicit and follows used ABI.  So, GIMPLE
changes would not affect RTL.

>
> What model does ICC follow - the dataflow behind the backs or
> the callgraph + clones?  (well, more "closely" I should ask - it probably
> does a third thing ...)

ICC implementation details are closed for me.

Thanks,
Ilya

>
> Thanks,
> Richard.
>
>> Thanks,
>> Ilya
>>
>>>
>>>>
>>>> Jeff
>>>>