|
Karen,
On 2/21/18 16:46, Karen Kinnear wrote:
Sergeui,
You were right - I read the sources incorrectly.
This code is easy to misunderstand - I read it incorrectly multiple
times. :)
This parts causes most of confusion:
(JVM_ACC_FINAL|JVM_ACC_STATIC)) == 0
Would still help to understand both
the motivation and the reason to not add to the
spec.
Robert - do you remember why we didn’t add this to
the specification? (6404550)
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”.
Sergeui - you are right - I misread this today.
And I played with the tests a bit - thanks Dan - and it
matches the way you read this.
So today, private methods that are not marked as final in
the source code can not be added -
I tried that variation by modifying the
RedefineMethodAddInvokeTarget_1.java and changing
private final void myMethod1() to private void myMethod1()
and got an UnsupportedOperationException.
So - private methods are not marked as ACC_FINAL today so I
think the simplification doesn’t
apply, so we are left with the ability to ADD or DELETE
PRIVATE && (STATIC || FINAL) methods - at least
that is what we support today.
Thank you for confirmation!
I suspected this because of the comment:
// hack: private should be treated as final, but alas
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.
I tried the test. And it works because of the requirement that
FINAL or STATIC are set,
which therefore means no vtable entry.
Good to know.
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.
We do create a vtable entry for private methods; however if
FINAL or STATIC is set, then
we do not create a vtable entry.
That is why we don’t ever get here.
Great.
I'll add this conclusion to the bug report to have it recorded.
Thanks,
Serguei
thanks,
Karen
Thanks,
Serguei
thanks,
Karen
On 2/21/18 2:45 AM, [email protected]
wrote:
On 2/20/18 23:01, David Holmes
wrote:
On
21/02/2018 4:50 PM, [email protected]
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:
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
|
