12 RFR(M) 8214583: AccessController.getContext may return wrong value after JDK-8212605

[dean.long at oracle.com]

On 12/16/18 7:03 PM, David Holmes wrote:
> On 17/12/2018 12:49 pm, dean.long at oracle.com wrote:
>> On 12/16/18 4:06 PM, David Holmes wrote:
>>> On 15/12/2018 10:59 am, dean.long at oracle.com wrote:
>>>> https://bugs.openjdk.java.net/browse/JDK-8214583
>>>> http://cr.openjdk.java.net/~dlong/8214583/webrev
>>>>
>>>> This change includes two new regression test that demonstrate the 
>>>> problem, and a fix that allows the tests
>>>> to pass.
>>>>
>>>> The problem happens when the JIT compiler's escape analysis 
>>>> eliminates the allocation of the AccessControlContext object passed 
>>>> to doPrivileged.  The compiler thinks this is safe because it does 
>>>> not see that the object "escapes".
>>>
>>> Then surely the compiler's notion of "escapes" needs to be updated!
>>>
>>
>> The compiler can inline the callee method and see that the value 
>> doesn't escape.  This is a valid optimization in cases where the 
>> callee method is known.
>
> But it's not a valid optimization in this case, so my comment stands.
>
> Is this stack walking something this is guaranteed by the spec to be 
> always valid (and hence the JIT is violating the rules), or is the 
> stack walking code making assumptions about whether it will find the 
> context object in the stack?
>

The stack walking is in the VM and is an internal implementation detail, 
not part of the AccessController API spec.  A different thread running 
normal Java code would never be able to see a non-escaping value.  The 
stack walking code does need to find the context object in the stack.  
Non-escaping objects won't show up in the stack.

> If we have to hack around this with an annotation I'd rather see a 
> specific annotation that addresses the problematic usecase than a 
> generic "don't inline" one. E.g. @StackVisible or something like that.
>

That sounds like a good idea for 13, but would require changes to both 
C2 and Graal, and it seems a little risky compared to using existing 
mechanisms.

dl

> Cheers,
> David
>
>>
>> dl
>>
>>> David
>>> -----
>>>
>>>   However, getContext needs to be able to find
>>>> the object using a stack walk, so we need a way to tell the 
>>>> compiler that it does indeed escape. To do this we pass the value 
>>>> to a native method that does nothing.
>>>>
>>>> Microbenchmark results:
>>>>
>>>> jdk12-b18:
>>>>
>>>> Benchmark                Mode  Cnt    Score   Error  Units
>>>> DoPrivileged.test        avgt   25  255.626 ± 6.446  ns/op
>>>> DoPrivileged.testInline  avgt   25  250.968 ± 4.975  ns/op
>>>>
>>>>
>>>> jdk12-b19:
>>>>
>>>> Benchmark                Mode  Cnt  Score    Error  Units
>>>> DoPrivileged.test        avgt   25  5.689 ±  0.001  ns/op
>>>> DoPrivileged.testInline  avgt   25  2.765 ±  0.001  ns/op
>>>>
>>>> this fix:
>>>>
>>>> Benchmark                Mode  Cnt  Score    Error  Units
>>>> DoPrivileged.test        avgt   25  5.020 ±  0.001  ns/op
>>>> DoPrivileged.testInline  avgt   25  2.774 ±  0.025  ns/op
>>>>
>>>>
>>>> dl
>>