RFR 8145964: NoClassDefFound error in transforming lambdas
Coleen Phillimore
coleen.phillimore at oracle.com
Thu Aug 18 12:36:30 UTC 2016
On 8/17/16 9:39 PM, Daniel D. Daugherty wrote:
> On 8/16/16 5:16 PM, Coleen Phillimore wrote:
>>
>>
>> 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
>>
>
> Formatting looks good to me.
>
>
>> http://cr.openjdk.java.net/~coleenp/jvmti.html#jvmtiCapabilities.can_retransform_any_class
>>
>
> Formatting looks good to me
>
>
>> And added "some implementation defined classes" to the description of
>> IsModifiableClass
>>
>> http://cr.openjdk.java.net/~coleenp/jvmti.html#IsModifiableClass
>
> Formatting looks good to me.
>
>
>> I also rewrote the test. See webrev:
>>
>> http://cr.openjdk.java.net/~coleenp/8145964.04/webrev/index.html
>
> src/share/vm/prims/jvmti.xml
> L7161: <capability id="can_redefine_any_class">
> L7162: If possessed then all classes (except primitive,
> array, and some implementation defined
> L7163: classes) are modifiable.
>
> L7163 should be:
>
> classes) are modifiable (redefine or retransform).
>
> The above change matches the style used in can_redefine_any_class
> entry in the capability table.
Okay, I added this.
>
> L7165: <capability id="can_retransform_any_class">
> L7166: If possessed then all classes (except primitive,
> array, and some implementation defined
> L7167: classes) are modifiable.
>
> L7167: should be:
>
> classes) are modifiable with <functionlink
> id="RetransformClasses"/>.
>
Okay, I added this also.
> The above change matches the style used in
> can_retransform_any_class
> entry in the capability table.
>
> The two _any_ capability entries are written in different styles
> because they were added at different times possibly by different
> people...
Yes, I left them inconsistent in the capability table because that's the
way it was and it's not obvious whether that's correct or not.
>
> src/share/vm/prims/jvmtiEnv.cpp
> No comments.
>
> src/share/vm/prims/jvmtiRedefineClasses.cpp
> No comments.
>
> test/runtime/RedefineTests/RetransformAnonymous.java
>
> Since the test now does both retransform and redefine operations,
> you should rename it to ModifyAnonymous.java.
Sure.
>
> L59: static boolean done = false;
> This should be a volatile since it's used by two threads.
>
Okay, fixed.
Thank you for the careful review and thought and all the background
information about this issue.
Coleen
>
> Dan
>
>
>>
>> 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