RFR 8145964: NoClassDefFound error in transforming lambdas

[David Holmes]

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