RFR 8145964: NoClassDefFound error in transforming lambdas
Coleen Phillimore
coleen.phillimore at oracle.com
Mon Aug 15 23:09:28 UTC 2016
David, thanks see below.
On 8/15/16 12:05 AM, David Holmes wrote:
> Hi Dan,
>
> On 15/08/2016 12: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.
>
> Well ... I don't see that as a definitive list of all non-modifiable
> classes, simply a statement that such classes are never modifiable.
> You could argue that the spec could give additional examples of when
> classes may be non-modifiable, but I don't see that as strictly
> necessary and I certainly don't think we should mention VMAC's anywhere.
>
Yes, this is the part I agree with. I don't want to mention these...
>> Similar wording changes should be done for RedefineClasses() and
>> RetransformClasses() APIs.
>
> They already allow for encountering an unmodifiable class.
>
>> 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.
>
> <sigh> This is so frustrating. Why even bother with the notion of
> IsModifiableClass (which suggests any class might be non-modifiable
> under some circumstances) when it is then completely undermined by the
> can_redefine_any_class capability? If the intent was that only
> primitives and arrays are ever non-modifiable then that should have
> simply been listed in the spec for RedefineClasses and
> restransformClasses, instead of introducing IsModifiableClass.
> Otherwise can_redefine_any_class should simply refer to
> IsModifiableClass.
>
>> 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...
>
> We may have to switch to option 2 and just silently ignore everything
> related to VMACs.
So now that you convinced me that IsModifiableClass is cleaner, I'm more
happy with that!
>
> That said I don't think that we have to spell out every potential case
> where a class may be unmodifiable. It should be sufficient (as
> currently stated) that you can't redefine or retransform an
> unmodifiable class. The only thing that needs fixing in my opinion is
> the definition of can_redefine_any_class to not refer to primitives or
> arrays but simply to any class for which IsModifiableClass returns true!
I think this would be a good change in light of these VMACs. There's
also can_retransform_any_class which is defined similarly but not with
IsModifiableClass.
>
> can_redefine_any_class is really puzzling if you consider
> can_redefine_classes. It is far from clear to me what set of classes
> can_redefine_any_class gives access to that can_redefine_classes does
> not ??? What purpose do two capabilities serve here?
I'd love to know this too.
Thanks,
Coleen
>
> David
> -----
>
>> 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