RFR 8145964: NoClassDefFound error in transforming lambdas
Daniel D. Daugherty
daniel.daugherty at oracle.com
Thu Aug 18 01:39:28 UTC 2016
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.
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"/>.
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...
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.
L59: static boolean done = false;
This should be a volatile since it's used by two threads.
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