RFR 8145964: NoClassDefFound error in transforming lambdas
Coleen Phillimore
coleen.phillimore at oracle.com
Thu Aug 18 13:11:09 UTC 2016
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