RFR 8145964: NoClassDefFound error in transforming lambdas
Tom Rodriguez
tom.rodriguez at oracle.com
Thu Aug 18 17:31:14 UTC 2016
Looks good to me.
tom
> On Aug 18, 2016, at 6:11 AM, Coleen Phillimore <coleen.phillimore at oracle.com> wrote:
>
>
> New webrev:
>
> http://cr.openjdk.java.net/~coleenp/8145964.05/webrev
>
> I changed the wording as Dan suggested, see:
>
> http://cr.openjdk.java.net/~coleenp/jvmti.html#IsModifiableClass
>
> I also added the wording in CFLH to except anonymous classes.
>
> http://cr.openjdk.java.net/~coleenp/jvmti.html#ClassFileLoadHook
>
> I think I have no typos... I checked this a bunch of times.
>
> Thanks,
> Coleen
>
>
> On 8/18/16 8:36 AM, Coleen Phillimore wrote:
>>
>>
>> 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