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

[Vladimir Ivanov]

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

Best regards,
Vladimir Ivanov

[1] https://bugs.openjdk.java.net/browse/JDK-8166974