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

[dean.long at oracle.com]

Claes, can I list you as a reviewer?

dl

On 12/16/18 9:52 PM, Claes Redestad wrote:
> Fair enough, I wasn't aware EA was looking beyond the inlined code like
> this (which means it can't be "dead" or the JIT might see through the
> trick at some point). I've skimmed the existing usages in the JDK and
> can't find anything that seems to be dependent on DontInline defeating
> EA - maybe there was in the past, or I simply misremembered.
>
> Either way, it might be nice to have a more explicit facility for this
> in a future release, as David suggested. Say an @Escaping local
> variable/parameter annotation.
>
> /Claes
>
> On 2018-12-17 05:45, dean.long at oracle.com wrote:
>> Unfortunately, I don't think @DontInline on an empty method is 
>> sufficient
>> here.  If other code is relying on @DontInline for the same purpose then
>> we might need to reexamine that code.  My understanding from discussing
>> with other compiler engineers is that using a native method is the 
>> safest
>> technique that the compilers can't see through.  The problem with
>> @DontInline is that C2 looks at the bytecodes of the target method, even
>> if it isn't inlined (see BCEscapeAnalyzer and the EstimateArgEscape 
>> flag).
>> There may be a way to make it work, but that would require more
>> investigation, and I'm not sure the benefit outweighs the risk.
>>
>> dl
>>
>> On 12/15/18 6:48 AM, Claes Redestad wrote:
>>> Hi Dean,
>>>
>>> to avoid escape analysis-eliminated allocations in the past @DontInline
>>> has been sufficient. This means a simpler patch (no changes to native
>>> code needed - added assertions notwithstanding) and passes your tests
>>> with C2 (it'd concern me if Graal's EA sees through this trick, as it
>>> might break some existing places where DontInline is used to this
>>> effect):
>>>
>>>     /**
>>>      * The value needs to be physically located in the frame, so 
>>> that it
>>>      * can be found by a stack walk.
>>>      */
>>>     @Hidden
>>>     @DontInline
>>>     private static void ensureMaterializedForStackWalk(Object o) {}
>>>
>>> Thanks!
>>>
>>> /Claes
>>>
>>> On 2018-12-15 01:59, 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".  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
>>