Hi David,
On 5/21/19 17:25, David Holmes wrote:
Hi Serguei,
Sorry but I think this "fix" is inappropriate. The capability may
be badly named but the intent was to have it apply to both
redefine and retransform.
Not sure, I fully understand you here.
This intent seemed to be wrong or, at least, it is really confusing
to everybody.
To understand it better, let's do some analysis first and start
from what we know.
1) The JVMTI RedefineClasses is implemented with using a
VM_RedefineClasses operation.
So, the VM_RedefineClasses is triggered on behave of a
RedefineClasses call from agent code.
The capability "can_redefine_classes" has to be possessed to
call RedefineClasses.
2) The JVMTI RetransformClasses is also implemented with using a
VM_RedefineClasses operation.
So, the VM_RedefineClasses is triggered on behave of a
RetransformClasses call from agent code.
The capability "can_retransform_classes" has to be possessed to
call RetransformClasses.
There is no check (and no intent to check) for the capability
"can_redefine_classes".
3) The VM_RedefineClasses starts a chain of CFLH's (per each
JvmtiEnv) in both cases above.
So that the retransformations (with CFLH's) both
RedefineClasses and RetransformClasses calls.
If the capability "can_retransform_classes" was not possessed
the retransformations
caused by RedefineClasses will be processed without problems.
It is because both redefine and retransform capabilities are
not required for CFLH's.
4) The capability "can_retransform_any_class" was added to allow
RetransformClasses calls
for any classes including non-modifiable (if the implementation
has such a notion).
It controls RetransformClasses calls only.
5) As I understand, the capability "can_redefine_any_class" was
added to allow
RedefineClasses calls for any classes including non-modifiable
(if the implementation has such a notion).
It should control RedefineClasses only.
I do not see why it has to apply to RetransformClasses as well.
Could you, explain, please, what is the advantages to apply it to
both retransform and redefine?
I see only problems/disadvantages with it:
D1: The terms "retransform" and "redefine" in this context are
confusing.
The RetransformClasses is using a VM_RedefineClasses operation.
Can we treat this VM_RedefineClasses operation as "redefine"?
My guess is that this was initial motivation to list both
"retransform" and "redefine"
for "can_redefine_any_class" capability.
D2: The "can_redefine_classes" and "can_retransform_classes" are
equal,
but "can_redefine_any_class" and "can_retransform_any_class"
are not.
There is no symmetry here which adds complexity and generates
confusion.
D3: The RedefineClasses is controlled with can_redefine_classes and
can_redefine_any_class.
But the RetransformClasses is controlled with:
can_retransform_classes, can_retransform_any_class and
can_redefine_any_class.
There is no symmetry here which adds complexity and generates
confusion.
Further this change should not have been pushed yet as the CSR has
not been approved.
Sorry, I confused this bug with another one.
This was not pushed yet.
Thank you for pointing it out!
Thanks,
Serguei
David
-----
On 22/05/2019 9:53 am, serguei.spit...@oracle.com wrote:
Hi Dan and David,
On 5/21/19 15:58, David Holmes wrote:
Hi Dan,
On 22/05/2019 2:34 am, Daniel D. Daugherty wrote:
On 5/21/19 2:19 AM, serguei.spit...@oracle.com wrote:
Hi guys,
I've found one more fragment in the IsModifiableClass spec
which has to be fixed.
Updated webrev v2:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2019/8046018-jvmti-cap-spec.2/
src/hotspot/share/prims/jvmti.xml
No comments.
Looks like there was a specific update to the spec to allow
can_redefine_any_class
to include retransform (in addition to redefine):
1.1.82
13 February 2006 Doc fixes: update can_redefine_any_class
to include retransform. Clarify that exception events cover all
Throwables. In GetStackTrace, no test is done for start_depth
too big if start_depth is zero, Clarify fields reported in
Primitive Field Callback -- static vs instance. Repair
confusing names of heap types, including callback names.
Require consistent usage of stack depth in the face of thread
launch methods. Note incompatibility of JVM TI memory
management with other systems.
I can't tell if you've chased down that change and why you no
longer
think that change is necessary.
I'm okay with the change, but I think you have more research to
do here.
I already chased that down and mentioned it in the CSR. It was
done under:
https://bugs.openjdk.java.net/browse/JDK-6328530
Unfortunately I misread the original bug description. In
relation to can_redefine_any_class it states:
A more precise definition would be:
Can retransform or redefine any non-primitive non-array
class.
It appears to me that they did consider can_redefine_any_class
to be a "super" capability that could be added on top of
can_redefine_classes to extend it to "any" class; or on top of
can_retransform_classes to extend it to "any" class. If so the
name choice was unfortunate.
Further it seems the implementation never checked this anyway.
Right.
The approach taken for JDK-6328530 is non-symmetrical and confusing.
But, I think, I understand why this decision was made. :)
If we ever decide to make some (non-primitive and non-array)
classes to be non-modifiable then
I do not see problems to implement it in a way that
can_redefine_any_class will be checked on
RedefineClasses only and can_retransform_any_class will be
checked on RetransformClasses only.
We will get a symmetry (and so, a simplification as well) in two
dimensions:
- between redefine and retransform capabilities
- between can_redefine_classes and can_redefine_any_class
capabilities
Thanks,
Serguei
David
Dan
Specdiff:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2019/8046018-jvmti-cap-spec.2/jvmti-specdiff/
Enhancement:
https://bugs.openjdk.java.net/browse/JDK-8046018
Related CSR:
https://bugs.openjdk.java.net/browse/JDK-8223915
Thanks,
Serguei
On 5/20/19 21:43, serguei.spit...@oracle.com wrote:
Hi David,
Thank you for looking at this!
On 5/20/19 20:53, David Holmes wrote:
Hi Serguei,
On 21/05/2019 4:07 am, serguei.spit...@oracle.com wrote:
Please, review a fix for enhancement:
https://bugs.openjdk.java.net/browse/JDK-8046018
Related CSR:
https://bugs.openjdk.java.net/browse/JDK-8223915
I have some comments on the CSR and about this change
overall as to me it is not a simple clarification at all,
but potentially a significant change in the meaning of the
capability.
I've answered your question in the CSR with my comment.
Webrev:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2019/8046018-jvmti-cap-spec.1/
You introduced a typo: modifialble
Assuming this proceeds a similar change is needed earlier:
7444 <capability id="can_redefine_any_class">
7445 If possessed then all classes (except
primitive, array, and some implementation defined
7446 classes) are modifiable (redefine or
retransform).
Good catch, thanks!
I've updated the webrev in place.
Thanks,
Serguei
Thanks,
David
-----
Summary:
The fix is to make the JVMTI can_redefine_any_class
capability spec more inconsistent.
It is just about a couple of lines.
Thanks,
Serguei
