RFR 8174954: Parameter target type is allowed access after a module read edge or a package export has occurred after failed resolution

[harold seigel]

Hi David,

MethodAccessReadTwice.java is an indy test.  Lines 120 - 153 test that 
the same invokedynamic instruction gets the same result.  The 
invokedynamic instruction gets generated for the string concatenation at 
line 31 of file .../p5/c5.java:

       24 package p5;
       25
       26 import java.lang.Module;
       27 import p2.c2;
       28
       29 public class c5 {
       30     public void method5(p2.c2 param) {
       31         System.out.println("In c5's method5 with param = " + param);
       32     }
       33
       34     public void methodAddReadEdge(Module m) {
       35         // Add a read edge from p5/c5's module (first_mod) to second_mod
       36         c5.class.getModule().addReads(m);
       37     }
       38 }

The indy resolution throws an IAE because package p5's module cannot 
read package p2's module.

The second test case, at lines 156 - 161 of MethodAccessReadTwice.java, 
similarly tests invokedynamic instructions.  In this case, they are 
generated for the string concatenations at lines 32 and 43 of file 
.../p7/c7.java.

This problem does not exist for non-indy instructions because, when 
their resolutions fail, they update the constant pool.  Indy cannot do 
this because, unlike other instructions, different indy instructions 
that use the same constant pool index are allowed to get different 
results.  Hence, the fix is indy only.

Thanks, Harold

On 10/20/2017 1:15 AM, David Holmes wrote:
> Hi Harold,
>
> On 19/10/2017 12:20 AM, harold seigel wrote:
>> Hi David,
>>
>> Thanks for looking at this.
>>
>> Please see inline comments.
>>
>> Thanks, Harold
>>
>>
>> On 10/17/2017 10:22 PM, David Holmes wrote:
>>> Hi Harold,
>>>
>>> On 17/10/2017 12:20 AM, harold seigel wrote:
>>>> Hi,
>>>>
>>>> Please review this new JDK-10 fix for bug JDK-8174954.  If the 
>>>> initial resolution attempt fails with a LinkageError exception then 
>>>> the fix saves the exception in the resolution_errors table and sets 
>>>> a flag in the constant pool Cache, indicating the failure.  
>>>> Subsequent attempts to do the same resolution will see that the 
>>>> flag is set, retrieve the exception from the resolution_errors 
>>>> table, and throw it, instead of re-trying the resolution.
>>>
>>> I'm a little confused. The fix seems all about indy, which seems to 
>>> be the topic of:
>>>
>>>  JDK-8174942 Bootstrap Method Called Multiple Times Despite Initial 
>>> Resolution Failure
>>>
>>> whereas this bug seems to be about a more general problem. Is this 
>>> fix addressing both issues??
>> Thanks for noticing this.  This fix addresses both bugs.  The sample 
>> program for 8174942 is one of the test cases for this fix.  I'll 
>> close 8174942 as a duplicate.
>
> I'm still somewhat confused as it seems that all the substantive 
> changes here have the word "indy" in them. What part of the change 
> actually fixes the non-indy issue tested by MethodAccessReadTwice.java?
>
> Thanks,
> David
>>>
>>> BTW there's no reason for JDK-8174954 to remain confidential.
>> Agreed.  I opened the bug.
>>>
>>> Thanks,
>>> David
>>>
>>>> The fix also prevents a race condition if two or more threads try 
>>>> to do the same resolution concurrently. When a thread tries to 
>>>> record the result of its resolution attempt, it will check to see 
>>>> if another thread recorded its result first.  If so, then it will 
>>>> use the result recorded by the other thread.  Otherwise, it will 
>>>> record and use its result. Access to the resolution result is 
>>>> protected by the 'resolved_references' lock.
>>>>
>>>> Open webrev: http://cr.openjdk.java.net/~hseigel/bug_8174954.1/webrev/
>>>>
>>>> JBS Bug: https://bugs.openjdk.java.net/browse/JDK-8174954
>>>>
>>>> The fix was tested with the JCK Lang and VM tests, the JTreg 
>>>> hotspot, java/io, java/lang, java/util, and other tests, the 
>>>> co-located NSK tests, JPRT, with Mach5 tier2 - tier5 tests, and by 
>>>> hand to check race condition handling.
>>>>
>>>> Thanks, Harold
>>>>
>>