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

[David Holmes]

Hi Harold,

On 20/10/2017 11:37 PM, harold seigel wrote:
> 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:

Okay. It was totally not at all evident from the bug report and the test 
code that the underlying problem was actually related to indy due to the 
string concatenation.

Thanks,
David

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