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

[dean.long at oracle.com]

David, can I list you as a reviewer?

dl

On 12/16/18 8:47 PM, dean.long at oracle.com wrote:
> On 12/16/18 7:39 PM, dean.long at oracle.com wrote:
>> 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.
>>
>
> I forgot to address this in my last reply, but I'm not suggesting a 
> @DontInline annotation.  That was Claes.  My fixes uses a native method.
>
> dl
>
>> 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
>>>>
>>
>