[10] RFR (S): 8196022: java.lang.VerifyError is thrown instead of java.lang.IllegalAccessError

[David Holmes]

Missed one thing at end ...

On 30/01/2018 7:21 AM, Vladimir Ivanov wrote:
> Hi David,
> 
>> Okay. I'm quite concerned that 8188145 has been fixed so late in the 
>> JDK 10 release cycle. Aligning the implementation with the spec is 
>> generally the intention, but you have to evaluate the impact of the 
>> change in behaviour. How long have we been doing the wrong thing? How 
>> much code might depend on us doing the wrong thing? This could have 
>> unexpected consequences - as per the current issue!
> 
> I haven't explored initial implementation (which went into JDK7 GA), but 
> current code is mostly there since 7u40. There were some adjustments in 
> JDK8 (e.g., AbstractMethodError instead of IllegalAccessError).
> 
> But, most of the time, those errors were hidden by BootstrapMethodError, 
> because invokedynamic wrapped all errors. It was changed in 9 (see 
> JDK-8166974 [1]) and then JDK-8188145 was reported: there was a 
> discrepancy in behavior between different JVM implementations noticed.
> 
> So, considering significant behavioral change in 9, the risk some code 
> depends on previous behavior is low. Otherwise, most likely, it is 
> already broken since 9 due to by JDK-8166974.
> 
>>>>> The fix cleans JDK part to behave the same way.
>>>>
>>>> I'm unclear how this fix changes the JDK side. Is it because it 
>>>> tranforms LinkageErrors, and you no longer allow LinkageErrors to 
>>>> pass through? (Does that make the JDK code "dead"?).
>>>
>>> Almost, but there are some important checks on JDK side which can 
>>> fail (e.g., see MemberName.Factory.resolve() [3]). So, I decided to 
>>> leave them as is for now.
>>
>> This is unclear. Have we introduced new failure modes via 8188145, or 
>> are these existing failure modes?
> 
> No new failure modes were introduced. Some existing failure modes left 
> intact for some corner cases (when MethodHandles::resolve_MemberName() 
> returned empty handle w/o throwing any exception).
> 
>>>>
>>>> Also the CR talks about VerifyError being thrown, but I didn't spot 
>>>> anything that would be dealing with that case ?? Is VerifyError the 
>>>> correct/expected exception now?
>>>
>>> Are you asking about JDK part?
>>>
>>> The confusion comes from the fact that MemberName resolution can be 
>>> triggered both from bytecode (through an upcall) and reflectively 
>>> (through proper MethodHandles.Lookup call).
>>>
>>> Reflective case doesn't want to see Errors, so everything is 
>>> converted to ReflectiveOperationException. But if resolution is 
>>> initiated by the JVM, ReflectiveOperationException is unwrapped back 
>>> to proper Error [4].
>>>
>>> To sum up, JDK part doesn't want to see LinkageErrors, so all 
>>> "unknown" errors are wrapped into IllegalAccessExceptions, but 
>>> requests from JVM should behave according to "equivalent bytecode 
>>> behavior".
>>
>> Has this fix altered the behaviour of calls initiated from Java? Or is 
>> it only adjusting the exceptions the VM may see if the sequence is:
> 
> There's one case when behaviour of calls initiated from Java changes: 
> NoClassDefFoundError/ClassNotFoundException were converted to 
> IllegalAccessException, but after this change ClassNotFoundException 
> will be reported.
> 
> If it looks like too much for jdk10, I can change it back and commit 
> separately as a cleanup into the next release.
> 
>> VM -> Java -> VM
>>
>> ?
> 
> For VM->Java->VM it changes IllegalAccessError to proper subclass of 
> LinkageError for the following cases:
>    * NoClassDefFoundError / ClassNotFoundException
>    * LinkageError subclasses reported during method/field resolution in 
> JVM (except NoSuchMethodError, NoSuchFieldError, and AbstractMethodError);
> 
> 
>>> That's why I decided to eliminate ClassNotFoundException for now - it 
>>> complicates with Error <-> Exception mappings.
>>
>> Shouldn't you have just unwrapped the original NCDFE instead of 
>> wrapping the CNFE (which wraps the original NCDFE) with another NCDFE?
> 
> Good point. I decided to mimic the behavior for 
> NoSuchMethodE*/NoSuchFieldE* and not for IllegalAccessException. No 
> strong reasons for that on my side, so if you prefer unwrapping I'll go 
> with it.
> 
>           } catch (NoSuchMethodException ex) {
>               Error err = new NoSuchMethodError(ex.getMessage());
>               throw initCauseFrom(err, ex);
>           } catch (NoSuchFieldException ex) {
>               Error err = new NoSuchFieldError(ex.getMessage());
>               throw initCauseFrom(err, ex);
> +        } catch (ClassNotFoundException ex) {
> +            Error err = new NoClassDefFoundError(ex.getMessage());
> +            throw initCauseFrom(err, ex);

That not what I meant - that's still wrapping the original exception in 
a new one. I was thinking more:

} catch (ClassNotFoundException ex) {
   Throwable cause = ex.getcause();
   if (cause instanceof NoClassDefFoundError)
     throw (NoClassDefFoundError) cause;
   // else ...

Cheers,
David

> Best regards,
> Vladimir Ivanov
> 
> [1] https://bugs.openjdk.java.net/browse/JDK-8166974