RFR 8145964: NoClassDefFound error in transforming lambdas
Coleen Phillimore
coleen.phillimore at oracle.com
Tue Aug 16 23:16:57 UTC 2016
On 8/15/16 10:01 PM, David Holmes wrote:
> On 16/08/2016 9:34 AM, Coleen Phillimore wrote:
>> Rereading this and looking at the JVMTI spec, I think Dan's wording and
>> changes make the most sense, if we are to keep IsModifiableClass to
>> exclude retransforming classes, with the exception of specifying
>> anonymous classes. I think the wording should just say "some
>> implementation defined classes" as below.
>
> I'm not sure what you mean exactly. Dan's changes seemed excessive to
> me. The only changes I see as essential are expressing the
> can_redefine/restransform_* in terms of IsModifiableClass.
David, I did three things. I changed the description for the
capabilities can_redefine_any_class and can_retransform_any_class to:
http://cr.openjdk.java.net/~coleenp/jvmti.html#jvmtiCapabilities.can_redefine_any_class
http://cr.openjdk.java.net/~coleenp/jvmti.html#jvmtiCapabilities.can_retransform_any_class
And added "some implementation defined classes" to the description of
IsModifiableClass
http://cr.openjdk.java.net/~coleenp/jvmti.html#IsModifiableClass
I also rewrote the test. See webrev:
http://cr.openjdk.java.net/~coleenp/8145964.04/webrev/index.html
What do you think?
Thanks,
Coleen
>
> David
>
>> thanks,
>> Coleen
>>
>> 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.
>>>
>>> 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.
>>>
>>> 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().
>>>
>>> Sorry this is turning into a can of worms...
>>>
>>> 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