JEP 270 concerns

[Frederic Parain]

Hi Andrew,

Thank you for the deep investigation and the detailed report.

On 01/11/2017 10:49 AM, Andrew Haley wrote:
> I've been porting "JEP 270: Reserved Stack Areas for Critical
> Sections" to AArch64.  I have no particular concerns about the port,
> but I found some serious flaws in testing.
>
> The main problem is that it doesn't work when inlining is enabled.  To
> demonstrate this fact, remove "-Inline" and add "-Xss512k" to the
> runtime arguments in ReservedStackTest.  (I tried x86_64 and AArch64.)
>
> The first problem is that the stack walking code in
> SharedRuntime::look_for_reserved_stack_annotated_method only looks at
> stack frames, not at the methods which have been inlined into compiled
> methods.  So, if a method marked with a ReservedStackAccess annotation
> is inlined, the runtime code will not see the annotation, and the
> reserved zone will not be used.

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.

> 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.

Please, could you sent me your code using Scopes? I'd like to
experiment with it.

> 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.

> 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 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?

> There is another problem: if a callee of a ReservedStackAccess method
> makes a runtime call, the yellow zone is disabled; when that runtime
> call returns, the yellow zone and the reserved zone are re-eanbled, so
> the reserved zone protection is re-enabled while that method is
> running.  We then return to the ReservedStackAccess method and trigger
> an assertion because the reserved zone protection is enabled, which is
> unexpected.

If the yellow zone has been disabled, it means that a StackOverflowError
has been thrown. If it happens, it means that the ReservedStackArea
was sizing too small. But even if a StackOverflowError is thrown in
an annotated section, it should not trigger an assertion failure,
so this is clearly a bug.

> Of course I may be very much mistaken about all of this, but I think
> not.

Your insights were very helpful on the in-lining issue.

Regards,

Fred