RFR 8145964: NoClassDefFound error in transforming lambdas

Fri Aug 12 18:19:33 UTC 2016

Okay, I've made retransforming a class an unmodifiable class error and 
will give whoever complains your phone number.   I've run the change 
through the redefinition tests in our colocated (our closed), 
noncolocated (also oracle-only), jdk/test/java/lang/instrument and 
jdk/test/com/sun/jdi testsets.

Please see new webrev.

http://cr.openjdk.java.net/~coleenp/8145964.03/webrev/index.html

Thanks,
Coleen

On 8/11/16 11:44 PM, David Holmes wrote:
> On 12/08/2016 12:23 AM, Coleen Phillimore wrote:
>> On 8/11/16 8:20 AM, David Holmes wrote:
>>> On 11/08/2016 10:09 PM, Coleen Phillimore wrote:
>>>>
>>>>
>>>> On 8/10/16 8:37 PM, David Holmes wrote:
>>>>> Hi Coleen,
>>>>>
>>>>> On 11/08/2016 12:45 AM, Coleen Phillimore wrote:
>>>>>>
>>>>>> New webrev:
>>>>>>
>>>>>>
>>>>>> http://cr.openjdk.java.net/~coleenp/8145964.02/webrev/index.html
>>>>>
>>>>> First I'm very surprised that the existing logic in
>>>>> JvmtiEnv::RetransformClasses doesn't utilize
>>>>> VM_RedefineClasses::is_modifiable_class to determine when to return
>>>>> JVMTI_ERROR_UNMODIFIABLE_CLASS. It would seem easy for the two 
>>>>> bits of
>>>>> code to get out of sync!
>>>>>
>>>>> Second, why do you silently ignore an attempt to redefine an 
>>>>> anonymous
>>>>> class instead of returning JVMTI_ERROR_UNMODIFIABLE_CLASS? The
>>>>> restriction on transforming anonymous classes seems no different 
>>>>> to me
>>>>> to the restriction on transforming primitive or array classes.
>>>>
>>>> I believe that there are existing applications, as in the test, get 
>>>> all
>>>> the loaded classes and try to transform them.  These will get the VM
>>>> anonymous class, so we didn't want to give them an error. Or set CLFH
>>>> and vm anonymous classes fall into the load hook. There's another bug
>>>> that Rachel has that ignores them for CFLH (rather than crashing).
>>>>
>>>> From the Java standpoint, the existence of vm anonymous classes are a
>>>> implementation detail and not real classes, and it's better to hide
>>>> these as much as possible.
>>>
>>> I think this is a mistake. While anonymous classes may have started as
>>> some obscure VM implementation detail related to JSR-292 and
>>> InvokeDynamic they are now entities that are prevalent in a running
>>> Java application due to the extensive use of Methodhandles and their
>>> use by lambda expressions. Unless we can hide such classes completely
>>> (we can't!) we need to define their semantics when treated like other
>>> "normal" Java classes. This should have been addressed for JVM TI when
>>> anonymous classes came into existence but it wasn't. But we have to
>>> deal with it, so simply define them as non-modifiable classes just
>>> like array classes and primitives. If code gets an error that is good
>>> it _needs_ to get an error because it needs to realize that it is
>>> dealing with anonymous classes not "real" ones!
>>>
>>
>> I disagree, and please read the bug where the consensus is that we
>> should ignore these for retransformation.  Redefinition is a different
>> matter because you have to give the class with new bytecodes, which
>> would be very bad, and that's why I chose to give an error message.
>
> I don't read such a consensus in the bug report.
>
>> This is John Rose's answer to a similar question:
>>
>> http://mail.openjdk.java.net/pipermail/core-libs-dev/2016-January/038353.html 
>>
>
> Yes I added that link to the bug report initially. It reinforces that 
> retransform/redefinition of VMAC is not intended to work  so don't do 
> that: "Please don't lead your users to rely on them."
>
> I read more in John's response that the code doing the transformations 
> should be skipping the VMAC's, than that the VM code should ignore the 
> VMAC's if passed in.
>
> The current situation is inconsistent and that is what I object to, 
> most strenuously. When I suggested this be handled through 
> VM_RedefineClasses::is_modifiable_class I was expecting the VMAC to be 
> treated as an unmodifiable class in all cases/sense. It is not 
> acceptable to me to have IsModifiableClass return false yet 
> retransform does not report JVMTI_ERROR_UNMODIFIABLE_CLASS. There 
> should be tests that check that the two are consistent!
>
> Please, either silently ignore it everywhere, or else deal with it 
> consistently as unmodifiable.
>
>> I am not willing to break existing applications.
>
> Any existing application that retransforms an anonymous class is 
> already in unchartered waters. To quote John "Even if you were able to 
> 'transform' one of these classfiles, it wouldn't necessary do what you 
> think it should do". So if it is lucky any existing code that attempts 
> to transform an anonymous class simply does no harm - but more likely 
> it gets an error (as per this bug report), or worse it "does something 
> you didn't think it would do"! If such code is now run against JDK 9 
> then I think they would be grateful to be told that they are running 
> broken code, and exactly where it is broken.
>
> Cheers,
> David
>
>> Coleen
>>
>>> Cheers,
>>> David
>>>
>>>> Coleen
>>>>
>>>>>
>>>>> Thanks,
>>>>> David
>>>>>
>>>>>> Reran jvmti tests.
>>>>>>
>>>>>> Thanks,
>>>>>> Coleen
>>>>>>
>>>>>> On 8/10/16 9:32 AM, Coleen Phillimore wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 8/9/16 7:40 PM, David Holmes wrote:
>>>>>>>> Hi Coleen,
>>>>>>>>
>>>>>>>> On 10/08/2016 4:52 AM, Coleen Phillimore wrote:
>>>>>>>>> Summary: Skip VM anonymous classes in retransformation and 
>>>>>>>>> give an
>>>>>>>>> error
>>>>>>>>> for redefinition.
>>>>>>>>>
>>>>>>>>> Contributed by Tom Rodriguez.
>>>>>>>>>
>>>>>>>>> Tested with redefinition colocated tests (tonga) and
>>>>>>>>> java/lang/instrument tests, and added test case.
>>>>>>>>>
>>>>>>>>> open webrev at
>>>>>>>>> http://cr.openjdk.java.net/~coleenp/8145964.01/webrev
>>>>>>>>> bug link https://bugs.openjdk.java.net/browse/JDK-8145964
>>>>>>>>
>>>>>>>> Shouldn't anonymous classes be added to the set of non-modifiable
>>>>>>>> classes - so IsModifiableClass returns false and we just add this
>>>>>>>> case to VM_RedefineClasses::is_modifiable_class.
>>>>>>>
>>>>>>> Yes, that might be a better way to do it.
>>>>>>> thanks,
>>>>>>> Coleen
>>>>>>>
>>>>>>>>
>>>>>>>> ??
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> David
>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> Coleen
>>>>>>>
>>>>>>
>>>>
>>