On 2/21/18 16:59, David Holmes wrote:
On 22/02/2018 7:44 AM, serguei.spit...@oracle.com wrote:
Hi Karen,
Thank you for sorting this out!
On 2/21/18 09:55, Karen Kinnear wrote:
Dan,
Thank you for all the background digging. This is really helpful.
Serguei - do you know what tests exist for this behavior?
Dan already replied (thanks, Dan!)
There are two tests in the open/test/jdk/java/lang/instrument:
RedefineMethodAddInvoke*
RedefineMethodDelInvoke*
The way I read the source code - we currently allow ADD and DELETE for
PRIVATE OR STATIC OR FINAL methods. Did I read that correctly?
The above does not look correct to me.
We have the same check for both ADD and DELETE method:
if ((old_flags & JVM_ACC_PRIVATE) == 0
// hack: private should be treated as final, but alas
|| (old_flags & (JVM_ACC_FINAL|JVM_ACC_STATIC)) == 0
) {
// deleted methods must be private
return JVMTI_ERROR_UNSUPPORTED_REDEFINITION_METHOD_DELETED;
}
I read it as we allow ADD and DELETE for PRIVATE && (STATIC || FINAL)
methods.
(Rephrase: We allow PRIVATE FINAL or PRIVATE STATIC methods.)
As private should always be treated final then we can simplify the
above to:
We allow and and delete PRIVATE INSTANCE or PRIVATE STATIC methods
which is equal to just "PRIVATE methods".
I read it as "PRIVATE" or "FINAL STATIC".
There are multiple negations above, so this needs to be interpreted in a
right context:
Return error if deleted method is !PRIVATE or (!FINAL and !STATIC)
After inversion:
Allow to delete if method is PRIVATE and (FINAL or STATIC)
I feel myself stupid when reading this code. :)
Thanks,
Serguei
David
-----
With the current implementation, I am not sure if deletion works for
private methods - do we
have a test for that? Or could you add one as part of this exercise?
Yes, we have one j.l.instrument test: RedefineMethodDelInvoke.sh.
Today we create a vtable entry for private methods (my
misunderstanding ~ 2006ish). After discussions
with David I no longer believe we need those.
Today, klassVtable::adjust_method_entries has an assertion
assert(!old_method->is_deleted(), “vtable methods may not be
deleted”)
I may have read the code incorrectly - but I would expect to hit
this assertion if you had a private
method you were deleting that was not final and not static.
option 1) we could explicitly tighten the restrictions to match what
we have implemented
option 2) we could make this work by changing
klassVtable.cpp::update_inherited_vtable
handling of private fields to be done the way it handles final
fields.
option 3) I read the code incorrectly?
If we create a vtable entry for private methods then we should hit
the assert above.
If we no longer need this then we have no problem.
Thanks,
Serguei
thanks,
Karen
On Feb 21, 2018, at 10:40 AM, Daniel D. Daugherty
<daniel.daughe...@oracle.com <mailto:daniel.daughe...@oracle.com>>
wrote:
On 2/21/18 2:45 AM, serguei.spit...@oracle.com wrote:
On 2/20/18 23:01, David Holmes wrote:
On 21/02/2018 4:50 PM, serguei.spit...@oracle.com wrote:
Hi Karen and David,
On 2/20/18 19:52, David Holmes wrote:
Hi Karen,
On 21/02/2018 1:54 AM, Karen Kinnear wrote:
Folks,
As part of the Valhalla EG discussions for JVMTI changes for
nestmates (thank you Serguei and David),
IBM brought up a request that we update the JVMTI
documentation to reflect that we allow addition
of private methods.
Is there a reason we do not document this? I’m inviting those
who were involved at the time - please include
others if needed.
I support documenting this in the JVMTI spec and had a plan to
fix it in 11.
However, it is not clear to me yet if we have a consensus on it.
I would like to see a detailed analysis of the implications of
allowing this. I _think_ it is safe but ...
Valid concern.
Also, I'd love to collect more details on the initial motivation
to relax the JVMTI spec.
Most likely we had no CCC/CSR filed on this change.
This issue is tracked by:
https://bugs.openjdk.java.net/browse/JDK-8192936
"RI does not follow the JVMTI RedefineClasses spec that is too
strict in the definition"
Yes, this is the one.
Thank you, David, for posting the link.
As I wrote there ... It is not at all clear how JDK-6404550
morphed into "Permit the adding or deleting of private
final/static methods with redefine" - nor why those changes
failed to make any change to the spec itself. It is also
unclear whether the add/delete is restricted to final/static
methods or any private method? I can see that the intent was to
only allow something that would not perturb the vtable for
existing instances.
I agree, there is a confusion somewhere.
Is it possible, the JDK-6404550 in JIRA is a different bug than
the one in the Bugtraq system?
The JDK-6404550 in JIRA has a different synopsis:
https://bugs.openjdk.java.net/browse/JDK-6404550
Cannot implement late attach in NetBeans Profiler due to
missing functionality in JVMTI
Digging deeper ... to fix the problem described in that bug they
augmented JVM TI to allow private method redefinition as an
alternate to the "native rebinding" technique that had been used
previously. See the final comment in:
https://bugs.openjdk.java.net/browse/JDK-6341303
"JVMTI Spec: Need a way how to rebind Object.wait and
Thread.sleep with late attach"
which was closed as a duplicate.
Thank you for the point.
This explains it.
It seems, the bug synopsis was changed at some moment.
The synopsis for 6404550 has never changed. Here's the subject line
when
it was created on 2006.03.27:
> CR 6404550 *HOT* Created P1 hotspot/jvmti Cannot implement late
attach in NetBeans Profiler due to missing functionality in JVMTI
I think the confusion arises over comments like this in 6341303:
rfield Robert Field
<https://bugs.openjdk.java.net/secure/ViewProfile.jspa?name=rfield>
added a comment - 2006-05-04 11:54
BT2:EVALUATION
This can now be accomplished with Java programming language
instrumentation, via:
6404550: missing late attach (JVM TI redefine) functionality
Permit the adding or deleting of private final/static
methods with redefine
Closing this bug as a duplicate.
That's just Robert's style for an sccs delta comment:
D 1.65.2.3 06/04/25 23:36:35 rfield 140 139 00023/00013/03263
MRs:
COMMENTS:
6404550: missing late attach (JVM TI redefine) functionality
Add/delete private methods, continued: changes per review
Back in the ancient past we tried to include some brief
info about the change in the delta comment. This was one of many
deltas associated with 6404550.
Please see the attached email that I sent on 2012.12.17 about the
history behind this issue... (sent to Karen, Mikael V, and Serguei)
It seems I forwarded that same email to Coleen, Markus G and Serguei
back on 2014.05.20. Since Markus is on that thread, it must have had
something to do with research about JFR...
I need to do a detailed read thru my e-mail archive for 6404550 to
see if I can spot some clues about why we didn't do a spec update.
Dan
Thanks,
Serguei
David
-----
Thanks,
Serguei
--
David
thanks,
Karen
<Attached Message.eml>
