This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


On Thu, Nov 21, 2013 at 10:31 AM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote:
> 2013/11/20 Richard Biener <richard.guenther@gmail.com>:
>> On Wed, Nov 20, 2013 at 10:57 AM, Richard Biener
>> <richard.guenther@gmail.com> wrote:
>>> On Tue, Nov 19, 2013 at 9:18 PM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote:
>>>> 2013/11/19 Jeff Law <law@redhat.com>:
>>>>> On 11/19/13 05:20, Ilya Enkovich wrote:
>>>>>>
>>>>>> 2013/11/19 Richard Biener <richard.guenther@gmail.com>:
>>>>>>>
>>>>>>> On Mon, Nov 18, 2013 at 8:12 PM, Ilya Enkovich <enkovich.gnu@gmail.com>
>>>>>>> wrote:
>>>>>>>>
>>>>>>>> 2013/11/18 Jeff Law <law@redhat.com>:
>>>>>>>>>
>>>>>>>>> On 11/18/13 11:27, Ilya Enkovich wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> How does pointer passed to regular function differ from pointer passed
>>>>>>>>>> to splitted function? How do I know then which pointer is to be passed
>>>>>>>>>> with bounds and wchich one is not? Moreover current ABI does not allow
>>>>>>>>>> to pass bounds with no pointer or pass bounds for some pointers in the
>>>>>>>>>> call only.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> But I don't see any case in function splitting where we're going to
>>>>>>>>> want to
>>>>>>>>> pass the pointer without the bounds.  If you want the former, you're
>>>>>>>>> going
>>>>>>>>> to want the latter.
>>>>>>>>
>>>>>>>>
>>>>>>>> There are at least cases when checks are eliminated or when lots of
>>>>>>>> pointer usages are accompanied with few checks performed earlier (e.g.
>>>>>>>> we are working with array). In such cases splitted part may easily get
>>>>>>>> no bounds.
>>>>>>>>
>>>>>>>>>
>>>>>>>>> I really don't see why you need to do anything special here.  At the
>>>>>>>>> most an
>>>>>>>>> assert in the splitting code to ensure that you don't have a situation
>>>>>>>>> where
>>>>>>>>> there's mixed pointers with bounds and pointers without bounds should
>>>>>>>>> be all
>>>>>>>>> you need or that you passed a bounds with no associated pointer :-)
>>>>>>>>
>>>>>>>>
>>>>>>>> It would also require generation of proper bind_bounds calls in the
>>>>>>>> original function and arg_bounds calls in a separated part. So,
>>>>>>>> special support is required.
>>>>>>>
>>>>>>>
>>>>>>> Well, only to keep proper instrumentation.  I hope code still works
>>>>>>> (doesn't trap) when optimizations "wreck" the bounds?  Thus all
>>>>>>> these patches are improving bounds propagation but are not required
>>>>>>> for correctness?  If so please postpone all of them until after the
>>>>>>> initial support is merged.  If not, please make sure BND instrumentation
>>>>>>> works conservatively when optimizations wreck it.
>>>>>>
>>>>>>
>>>>>> All patches I sent for optimization passes are required to avoid ICEs
>>>>>> when compiling instrumented code.
>>>>>
>>>>> Then I think we're going to need to understand them in more detail. That's
>>>>> going to mean testcases, probably dumps and some commentary about what went
>>>>> wrong.
>>>>>
>>>>> I can't speak for Richi, but when optimizations get disabled, I tend to want
>>>>> to really understand why and make sure we're not papering over a larger
>>>>> problem.
>>>>>
>>>>> The tail recursion elimination one we're discussing now is a great example.
>>>>> At this point I understand the problem you're running into, but I'm still
>>>>> trying to wrap my head around the implications of the funny semantics of
>>>>> __builtin_arg_bounds and how they may cause other problems.
>>>>
>>>> Root of all problems if implicit data flow hidden in arg_bounds and
>>>> bind_bounds.  Calls consume bounds and compiler does not know it. And
>>>> input bounds are always expressed via arg_bounds calls and never
>>>> expressed via formal args. Obviously optimizers have to be taught
>>>> about these data dependencies to work correctly.
>>>>
>>>> I agree semantics of arg_bounds call creates many issues for
>>>> optimizers but currently I do not see a better replacement for it.
>>>
>>> But it looks incredibly fragile if you ICE once something you don't like
>>> happens.  You should be able to easily detect the case and "punt",
>>> that is, drop to non-instrumented aka invalidating bounds.
>>>
>>> Thus, I really really don't like these patches.  They hint at some
>>> deeper problem with the overall design (or the HW feature or the
>>> accompaning ABI).
>>
>> Note that this, the intrusiveness of the feature and the questionable
>> gain makes me question whether GCC should have support for this
>> feature (and whether we really should rush this in this late).
>>
>> Thus, I hereby formally ask to push back this feature to 4.10.
>
> I think you overestimate the intrusiveness of the checker. Necessity
> of changes in optimization passes is artificial and is used to get
> maximum checking quality. It can be easily made immune for different
> code transformation by simple changes in the process of
> instrumentation expand (I have a fix for that already). With that
> change only pass itself, support for bound args during expand, support
> in i386 target and minor infrastructure changes are required (e.g.
> flag in varpool_node, bounds_constants). Changes in inline,
> propagation, SRA, tail recursion, strlen, function splitting, string
> function builtins expand would become optional and affect checking
> quality only.
>
> Also note that all changes do not affect compilation process when no
> instrumentation is used.
>
> Please reconsider your decision about pushing it to 4.10 taking that
> into account.

The point is that we are still pondering over the design and stage1
is basically over.

Richard.

> Thanks,
> Ilya
>
>>
>> Thanks,
>> Richard.
>>
>>> Richard.
>>>
>>>> Ilya
>>>>
>>>>>
>>>>>
>>>>> jeff
>>>>>


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]