RFR 8145964: NoClassDefFound error in transforming lambdas

[Coleen Phillimore]

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.

This is John Rose's answer to a similar question:

http://mail.openjdk.java.net/pipermail/core-libs-dev/2016-January/038353.html

I am not willing to break existing applications.

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