RFR 8145964: NoClassDefFound error in transforming lambdas
Coleen Phillimore
coleen.phillimore at oracle.com
Mon Aug 15 23:01:22 UTC 2016
Dan, Thank you for looking at this.
On 8/14/16 10: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.
Okay, I'll change the comment to
// classes for primitives and arrays and vm anonymous classes
cannot be redefined
>
> test/runtime/RedefineTests/RetransformAnonymous.java
> L39: why is there a blank line here (in the middle of the imports)?
I'll remove it.
>
> L79: System.out.println("retransform called for " + name);
> Perhaps add "unexpected" to that message?
But it's not unexpected! That's what I'm looking for.
>
> Shouldn't there also be a test for RedefineClasses()?
>
>
I just added one now, I'll post a rereview.
> 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.
>
> Similar wording changes should be done for RedefineClasses() and
> RetransformClasses() APIs.
>
> 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.
>
> 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().
>
Thank you for the wording but I agree with David's next email that we
shouldn't refer to anonymous classes here. Or maybe worded as "internal
java implementation classes". Anonymous classes are already confusing
because they're ambiguous.
Let me see what wording David suggests. I also agree that
can_retransform_any_class and can_redefine_any_class is an odd
capability since it doesn't really include any classes that can't be
transformed via can_retransform_classes.
> Sorry this is turning into a can of worms...
Yes it always does.
thanks,
Coleen
>
> 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