RFR 8145964: NoClassDefFound error in transforming lambdas
Coleen Phillimore
coleen.phillimore at oracle.com
Fri Aug 19 13:36:27 UTC 2016
Thanks, David.
Coleen
On 8/18/16 8:33 PM, David Holmes wrote:
> Looks good!
>
> Thanks Coleen!
>
> David
>
> On 18/08/2016 11:11 PM, Coleen Phillimore 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