Hi Dan,
Thanks for looking at it again.
See comment in-line.
Thanks, Harold
On 9/20/2016 11:25 AM, Daniel D. Daugherty wrote:
On 9/20/16 8:11 AM, harold seigel wrote:
Hi David,
Thanks for the review. Please review this updated webrev containing
the comment modifications that you suggested:
http://cr.openjdk.java.net/~hseigel/bug_8160987.3/
src/jdk.jdwp.agent/share/native/libjdwp/invoker.c
No comments.
test/com/sun/jdi/InterfaceMethodsTest.java
L200: // invoking static
nit typo: 'invoking' -> 'Invoking' (since you made it a sentence)
L208: // invoking static
nit typo: 'invoking' -> 'Invoking' (since you made it a sentence)
So for the test cases on L202 and L210 that changed from
testInvokePos() -> testInvokeNeg(), we had test cases in
place that verified the exact opposite of what the spec says?
And the same testInvokePos() -> testInvokeNeg() changes on
L255 and L263?
Note that the spec changed in JDK-9 (see JDK-8040167
<https://bugs.openjdk.java.net/browse/JDK-8040167>) from:
"Invokes a static method. The method must be member of the class
type or one of its superclasses, superinterfaces, or implemented
interfaces."
To:
"Invokes a static method. The method must be member of the class
type or one of its superclasses."
L253: // the instance
nit typo: 'the' -> 'The' and L254 needs a period at the end.
Re: use of "re-defined"
I doubt this means class redefinition but it had me going for a
second. :-)
Thumbs up! Feel free to ignore the nits above and I don't need
to see another webrev if you fix those.
Dan
Please also see comments in-line.
On 9/19/2016 10:50 PM, David Holmes wrote:
Hi Harold,
I'm having a lot of issues with the code and testing here. Please
bear with me.
On 19/09/2016 10:51 PM, harold seigel wrote:
Hi,
Please review this updated webrev for fixing JDK-8160987
<https://bugs.openjdk.java.net/browse/JDK-8160987>:
http://cr.openjdk.java.net/~hseigel/bug_8160987.2/
src/jdk.jdwp.agent/share/native/libjdwp/invoker.c
+ // If not the same class then check that containing_class is a
super type of
+ // clazz and not an interface (hence it's a super class).
Simpler to just say:
+ // If not the same class then check that containing_class is a
superclass of
+ // clazz (not a superinterface).
Took me a while to notice that interfaces don't inherit static
methods from superinterfaces either!
Done.
---
test/com/sun/jdi/InterfaceMethodsTest.java
The new comments are very verbose compared to other negative tests:
200 // try to invoke static method A on the instance. This
should fail because ref
201 // is of type InterfacemethodsTest$TargetClass which is
not a subtype of the
202 // class containing staticMethodA.
Could simply be:
200 // "staticMethodA" is not inherited by TargetClass
Done.
That aside the more I look at this test the more things I see that
seem to be wrong or at the very least confused, in the existing
code. First it seems that the test chooses to ignore the "class"
object when given a non-null ref object - so it talks about invoking
a static method on an instance, which is misleading at best as what
it will actually do is take a path that tries to invoke the static
method using the instances' class instead of the specified class
(which may be the interface class). This makes the test descriptions
and expected behaviours somewhat unintuitive in my opinion.
244 // "staticMethodA" must not be inherited by InterfaceB
245 testInvokeNeg(ifaceClass, null, "staticMethodA", "()I",
vm().mirrorOf(RESULT_A),
246 "Static interface methods are not inheritable");
I am wondering how this test passes without your fix? It suggests
there must already exist some code that compares the declaring class
with the passed in class. ??
This test passes without my fix because the ifaceClass passed to
testInvokeNeg() is InterfaceB, which does not contain a method called
staticMethodA. So, this code in the test's invoke() method throws an
exception because getMethod() returns null:
Method method = getMethod(targetClass, methodName, methodSig);
if (method == null) {
throw new Exception("Can't find method: " + methodName +
" for class = " + targetClass);
}
The code containing my fix does not get reached in this test case. In
contrast, the test cases that originally failed with my fix passed a
correct ifaceClass but an incorrect ObjectReference.
248 // however it is possible to call "staticMethodA" on the
actual instance
249 testInvokeNeg(ifaceClass, ref, "staticMethodA", "()I",
vm().mirrorOf(RESULT_A),
250 "Static interface methods are not inheritable");
The comment here suggests a successful execution when in fact it
expects it to fail. It should say "nor is it possible ...". But
again, how does this pass (by failing) without your fix ???
The comment here is obviously wrong. The test passes (by failing)
without my fix for the same reason as above. I fixed the comment.
252 // "staticMethodB" is overridden in InterfaceB
"overridden" is the wrong word here. Static interface methods are
not inherited so they can not be overridden. "re-defined" would be
an accurate description.
Done.
Similarly:
255 // the instance invokes the overriden form of
"staticMethodB" from InterfaceB
overridden -> re-defined
Will fix.
298 /* Static method calls */
This starts all the static method calls on the instance class - all
of which are expected to fail, and do so _without_ your fix present!
How?
These pass (by failing) for the same reasons as the above tests.
Method invoke()'s call to getMethod() returns null. So, invoke()
throws an exception.
Thanks, Harold
Thanks,
David
-----
It provides a more efficient implementation and fixes a test problem.
This fix was tested as described below and with the JTReg JDK
com/sun/jdi tests.
Thanks, Harold
On 9/16/2016 10:32 AM, harold seigel wrote:
Hi Serguei,
Thanks for the suggestion! That provides a much cleaner
implementation.
Harold
On 9/15/2016 11:28 PM, serguei.spit...@oracle.com wrote:
On 9/15/16 19:13, David Holmes wrote:
On 16/09/2016 8:52 AM, serguei.spit...@oracle.com wrote:
Hi Harold,
I did not got deep into the fix yet but wonder why the JVMTI
function is
My copy-paste failed.
I wanted to list the JVMTI function name: GetMethodDeclaringClass.
Thanks,
Serguei
not used.
I was wondering a similar thing. It seems very heavyweight to use
Java level reflection from inside native code to validate the
native
"handles" passed to that native code. I would have expected a
way to
go from a MethodId to the declaring class of the method, and a
simple way to test if there is an ancestor relation between the two
classes.
On 9/15/16 13:05, harold seigel wrote:
One could argue that a spec compliant JNI implementation wouldn't
need
this change in the first place...
Regardless, I'm withdrawing this change because I found that
it fails
a com/sun/jdi JTreg test involving static methods in interfaces.
I find it both intriguing and worrying that ClassType.InvokeMethod
refers to superinterfaces when prior to 8 (and this spec was not
updated in this area) static interface methods did not exist! The
main changes were in the definition of
InterfaceType.InvokeMethod. I
wonder whether invocation of static interface methods via
ClassType.InvokeMethod is actually tested directly?
I realize the specs are a bit of a minefield when it comes to what
is required by the different specs and what is implemented in
hotspot. Unfortunately it is a minefield I also have to wade
through
for private interface methods. In many cases it is not clear what
should happen and all we have to guide us is what hotspot does (eg
"virtual" invocations on non-virtual methods).
David
-----
Thanks, Harold
On 9/15/2016 3:37 PM, Daniel D. Daugherty wrote:
On 9/15/16 12:10 PM, harold seigel wrote:
(Adding hotspot-runtime)
Hi Dan,
Thanks for looking at this.
I could pass NULL instead of clazz to ToReflectMethod() to
ensure
that the method object isn't being obtained from clazz.
I don't think that would be a JNI spec compliant use of the
JNI ToReflectedMethod() function. That would be relying on
the fact that HotSpot doesn't use the clazz parameter to
convert {clazz,jmethodID} => method_object.
Sorry... again...
Dan
Harold
On 9/15/2016 1:09 PM, Daniel D. Daugherty wrote:
On 9/15/16 9:31 AM, harold seigel wrote:
Hi,
Please review this small fix for JDK-8160987. The JDWP
InvokeStatic() method was depending on the JNI function
that it
called to enforce the requirement that the specified
method must
be a member of the specified class or one of its super
classes.
But, JNI does not enforce this requirement. This fix adds
code to
JDWP to do its own check that the specified method is a
member of
the specified class or one of its super classes.
JBS Bug: https://bugs.openjdk.java.net/browse/JDK-8160987
Open webrev: http://cr.openjdk.java.net/~hseigel/bug_8160987/
src/jdk.jdwp.agent/share/native/libjdwp/invoker.c
Sorry I didn't think of this comment during the
pre-review...
The only "strange" part of this fix is:
L374: /* Get the method object from the method's
jmethodID. */
L375: method_object =
JNI_FUNC_PTR(env,ToReflectedMethod)(env,
L376: clazz,
L377: method,
L378: JNI_TRUE /* isStatic */);
L379: if (method_object == NULL) {
L380: return JVMTI_ERROR_NONE; /* Bad jmethodID
? This
will be handled elsewhere */
L381: }
Here we are using parameter 'clazz' to find the
method_object for
parameter 'method' so that we can validate that 'clazz'
refers to
method's class or superclass.
When a bogus 'clazz' value is passed in by a JCK test, the
only
reason that JNI ToReflectedMethod() can still find the
right
method_object is that our (HotSpot) implementation of JNI
ToReflectedMethod() doesn't really require the 'clazz'
parameter
to find the right method_object. So the 'method_object'
that we
return is the real one which has a 'clazz' field that
doesn't
match the 'clazz' parameter.
Wow does that twist your head around or what?
So we're trusting JNI ToReflectedMethod() to return the
right
method_object when we give it a potentially bad 'clazz'
value.
So should we use JNI FromReflectedMethod() to convert the
method_object back into a jmethodID and verify that
jmethodID
matches the one that we passed to check_methodClass()?
I might be too paranoid here so feel free to say that
enough is
enough with this fix.
Thumbs up!
Dan
The fix was tested with the two failing JCK vm/jdwp tests
listed
in the bug, the JCK Lang, VM, and API tests, the hotspot
JTReg
tests, the java/lang, java/util and other JTReg tests, the
co-located and non-colocated NSK tests, and with the RBT
Tier2
tests.
Thanks, Harold
