RFR 8145964: NoClassDefFound error in transforming lambdas

David Holmes david.holmes at oracle.com
Mon Aug 15 04:05:16 UTC 2016 

Hi Dan,

On 15/08/2016 12:20 PM, Daniel D. Daugherty wrote:
> On 8/12/16 6:33 PM, Coleen Phillimore wrote:
>>
>>
>> On 8/12/16 7:51 PM, David Holmes wrote:
>>> Hi Coleen,
>>>
>>> On 13/08/2016 4:19 AM, Coleen Phillimore wrote:
>>>>
>>>> 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, this is my preferred solution.
>>>
>>> Nit:
>>>
>>> !   // Cannot redefine an anonymous class.  Retransform filters them
>>> out.
>>>
>>> The second sentence  is not needed now.
>>
>> Thanks, David.   HIt reload, I had already changed that sentence but
>> neglected to create a new webrev.
>>>
>>> I didn't fully grok the test as I'm not familiar the various pieces.
>>>
>>>
>>> You need a second reviewer anyway so hopefully John and Tom will
>>> still chime in.
>>
>> Okay, I'll wait but it would be nice for someone else on the team to
>> review.
>
>> http://cr.openjdk.java.net/~coleenp/8145964.03/webrev/index.html
>
> src/share/vm/prims/jvmtiEnv.cpp
>     No comments.
>
> src/share/vm/prims/jvmtiRedefineClasses.cpp
>     L133:     // classes for primitives and arrays cannot be redefined
>         That comment needs updating.
>
> test/runtime/RedefineTests/RetransformAnonymous.java
>     L39: why is there a blank line here (in the middle of the imports)?
>
>     L79: System.out.println("retransform called for " + name);
>         Perhaps add "unexpected" to that message?
>
>     Shouldn't there also be a test for RedefineClasses()?
>
>
> The JVM/TI spec will need updates also:
>
> - RedefineClasses() API
> - RetransformClasses() API
> - IsModifiableClass() API
>
> Here's the line from IsModifiableClass():
>
>> Primitive classes (for example, java.lang.Integer.TYPE) and array
> classes are never modifiable.
>
> Perhaps change it like this:
>
>   Primitive classes (for example, java.lang.Integer.TYPE), array
> classes, and
>   some implementation defined classes (for example, anonymous classes) are
>   never modifiable.

Well ... I don't see that as a definitive list of all non-modifiable 
classes, simply a statement that such classes are never modifiable. You 
could argue that the spec could give additional examples of when classes 
may be non-modifiable, but I don't see that as strictly necessary and I 
certainly don't think we should mention VMAC's anywhere.

> Similar wording changes should be done for RedefineClasses() and
> RetransformClasses() APIs.

They already allow for encountering an unmodifiable class.

> There are various places that refer to can_redefine_any_class. Those places
> also need updating. Here's the one from RedefineClasses:
>
>> can_redefine_any_class    Can modify (retransform or redefine) any
> non-primitive
>>                           non-array class. See IsModifiableClass.
>
> Should probably be changed to something like:
>
> can_redefine_any_class    Can modify (retransform or redefine) any class
> except for
>                           a few specific classes. See IsModifiableClass
> for the classes
>                           that cannot be modified.

<sigh> This is so frustrating. Why even bother with the notion of 
IsModifiableClass (which suggests any class might be non-modifiable 
under some circumstances) when it is then completely undermined by the 
can_redefine_any_class capability? If the intent was that only 
primitives and arrays are ever non-modifiable then that should have 
simply been listed in the spec for RedefineClasses and 
restransformClasses, instead of introducing IsModifiableClass. Otherwise 
can_redefine_any_class should simply refer to IsModifiableClass.

> Similar wording change in the jvmtiCapabilities table that lists
> can_redefine_any_class. Of course, IsModifiableClass itself will
> have to be updated to spell out the classes that can't be modified.
>
> Looks like the spec wording for can_retransform_any_class is pretty
> much wrong w.r.t. IsModifiableClass() stuff and will all need work.
> Also looks like can_retransform_any_class wording is completely
> missing from IsModifiableClass().
>
> Sorry this is turning into a can of worms...

We may have to switch to option 2 and just silently ignore everything 
related to VMACs.

That said I don't think that we have to spell out every potential case 
where a class may be unmodifiable. It should be sufficient (as currently 
stated) that you can't redefine or retransform an unmodifiable class. 
The only thing that needs fixing in my opinion is the definition of 
can_redefine_any_class to not refer to primitives or arrays but simply 
to any class for which IsModifiableClass returns true!

can_redefine_any_class is really puzzling if you consider 
can_redefine_classes. It is far from clear to me what set of classes 
can_redefine_any_class gives access to that can_redefine_classes does 
not ??? What purpose do two capabilities serve here?

David
-----

> Dan
>
>
>>
>> thanks,
>> Coleen
>>>
>>> Thanks,
>>> David
>>>
>>>> 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
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>
>>>>>>
>>>>
>>
>

More information about the hotspot-dev mailing list