JEP 270 concerns

[Frederic Parain]

Hi Andrew,

I've re-read the code and my notes and here's
a more accurate description of the way JEP 270
intends to deal with in-lining:

When a method method m1 with the ReservedStackAccess
annotation is in-lined in a method m2, nothing special
is made in the way the method is in-lined and a check
on the reserved stack area is added to m2's return code.
The fact that m1 is a leaf or not doesn't matter (in
fact JEP 270 is important if m1 is not a leaf).

Does it mean that the reserved stack are is not re-enabled
immediately after the execution of an in-lined m1? Yes.

Does it mean m2's code can use the reserved stack area?
No, the stack banging performed at the beginning of m2
ensure that it has enough stack space available without
touching guard pages.

Does it mean that a method called by m2, after an execution
of m1 that trigger access to the reserved stack area, could
use the reserved stack area? Yes, this is unfortunate but
this is part of a trade-off explained below.

However, as you have spotted it, the method
look_for_reserved_stack_annotated_method() fails to detect
in-lined annotated methods. This is clearly a bug which makes
the annotation ineffective for in-lined methods.

The rational of the implementation.

The problem JEP 270 tries to address cannot be solved reliably
in all cases, simply because in Java when a method is invoked,
there's no way to anticipate what the execution will trigger
(class loading for instance).
The consequences of the original bug were considered serious enough
that we would like to mitigate the bug. But the bug occurrences were
also rare, so we didn't want to dedicate too much resources or to
impact performances for a rare bug.

It would be possible to modify the in-lining code to add checks
around an annotated in-lined method. The checks themselves might not
be that expensive (comparing values of 
JavaThread::_reserved_stack_activation
before and after the in-lined section is sufficient to detect that
the reserved stack area has been disabled). However, this would
put more constraints on the way the code is in-lined, requiring
to preserve well identified sequences of in-lined code. It was
considered that these constraints would reduce the number of
optimizations the JIT could performed when in-lining methods.

So, instead of modifying the in-lining code, the decision was
made to silently widen the scope of the annotation in case
of in-lining.

Now, if it appears that these hypothesis were wrong, and it
is possible to add the checks in in-lined code without impacting
performances and with a reasonable complexity in the JIT code,
we could revisit these choices. Having the semantic of the
annotation enforced strictly at the granularity of the annotated
method would a nice thing, but how much are we ready to pay for
the protection against this rare bug?

I hope it clarifies the implementation choices that have been
made.

Regards,

Fred

On 01/12/2017 07:19 AM, Andrew Haley wrote:
> Hi,
>
> On 11/01/17 23:47, Frederic Parain wrote:
>>
>> JEP 270 tried to resolve a very specific problem with stack overflows
>> occurring in critical sections. When an annotated method m1 is inlined,
>> in a method m2, m2 includes the stack requirements of m1 in the
>> computation of its stack requirements. So no stack overflow is
>> possible when m1 is "called (not really a call because of inlining).
>> However, it only works if m1 is a leaf. If m1 calls another method
>> and this method is not inlined, then I agree there is a case that
>> has been missed here.
>
> Right, but even in the jtreg case, m1 is not a leaf: it is
> nonfairTryAcquire.  This calls getExclusiveOwnerThread, which is the
> method that overflows the stack.  And
> look_for_reserved_stack_annotated_method does not see the method which is
> at the top anyway, so I think that a ReservedStackAccess method cannot
> itself use the reserved area: it has to be one of its callees.
>
>>> I thought that this was a simple omission, so I changed
>>> look_for_reserved_stack_annotated_method to walk through Scopes
>>> instead of frames, and this indeed detects that a ReservedStackAccess
>>> method has been inlined.  However, there is a deeper flaw: once
>>> control is returned to the method which inlines ReservedStackAccess,
>>> the reserved zone remains disabled, so the next time that a method is
>>> called the protection will not be in place.
>>
>> Regarding the very particular usage of the annotation, it could be
>> acceptable that a method which inlines an annotated method keeps
>> access to the ReservedStackArea. This is not ideal, but it won't
>> be worse than the situation before, and it would help anyway in
>> some cases.
>
> I guess so, but IMO the logic which handles a stack overflow when the
> reserved area is unprotected would have to be fixed so that it doesn't
> blindly re-protect the reserved area when it finishes.
>
>> Please, could you sent me your code using Scopes? I'd like to
>> experiment with it.
>
> I will.
>
>>> I think that the only reasonable way to fix this is to force methods
>>> annotated with ReservedStackAccess not to be inlined.
>>
>> JEP 270's goal is definitively not to prevent in-lining of the
>> critical methods using the annotation.
>
> OK.  It's the easy way to make this stuff all come out correctly,
> but I understand why not.
>
>>> It would be possible to fix this in a better way by changing the
>>> logic so that a runtime call to re-enable reserved zone is inserted
>>> at the return of every ReservedStackAccess-annotated method, but
>>> this would be more complex.
>>
>> This looks too expensive to me regarding the problem the
>> ReservedStackArea tries to mitigate. It is possible to test with
>> assembly code if the reserved area has been disabled or not (with
>> the JavaThread::_reserved_stack_activation).
>
> It wouldn't need to be any more expensive: a JIT is quite capable of
> inlining the exact same assembly code.  It's just more work.
>
>> It seems that there's a trade-off here between guaranteeing the
>> exact granularity of the annotation and preserving the performances
>> of in-lining. I think the later is the more important.
>>
>> Could widening the scope of the of the annotation in case of in-lining,
>> to include methods in which annotated methods are in-lined, be an
>> acceptable solution?
>
> That has a bad code smell, don't you think?  To do that you'll have to
> change the test so that it works when inlining is enabled: the test
> case works around the bug.  That feels wrong.  I don't think it's in
> any sense impossible to make JEP 270 work as designed, even when stuff
> is inlined.
>
> Andrew.
>