RFR 8145964: NoClassDefFound error in transforming lambdas
Coleen Phillimore
coleen.phillimore at oracle.com
Mon Aug 15 23:34:50 UTC 2016
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.
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