Re: RFR(L): 8008688: Make MethodHandleInfo public
Couldn't find any obvious problems. Looks good.
-- Chris
On Jul 2, 2013, at 6:26 PM, John Rose john.r.r...@oracle.com wrote:
Thanks for the helpful review, Vladimir.
I have incorporated all your comments and updated the webrev here:
http://cr.openjdk.java.net/~jrose/8008688/webrev.05
Detailed replies follow.
On Jul 1, 2013, at 3:36 PM, Vladimir Ivanov vladimir.x.iva...@oracle.com
wrote:
John,
I have some minor suggestions about code style and code readability.
Otherwise, the change looks good!
src/share/classes/java/lang/invoke/MemberName.java:
public MemberName(Method m, boolean wantSpecial) {
...
MethodHandleNatives.init(this, m);
+if (clazz == null) { // MHN.init failed
+if (m.getDeclaringClass() == MethodHandle.class
+isMethodHandleInvokeName(m.getName())) {
Please, add a comment with a short description why a custom init procedure
for MH.invoke* cases is needed.
Done:
// The JVM did not reify this signature-polymorphic instance.
// Need a special case here.
// See comments on MethodHandleNatives.linkMethod.
And I added several paragraphs in the javadoc for linkMethod.
They cover non-reification, linker methods, appendixes, synthetic, varargs,
and more.
+/** Create a name for a signature-polymorphic invoker. */
+static MemberName makeMethodHandleInvoke(String name, MethodType type) {
+return makeMethodHandleInvoke(name, type, MH_INVOKE_MODS |
SYNTHETIC);
Please, add a comment why SYNTHETIC flag is necessary.
/**
* Create a name for a signature-polymorphic invoker.
* This is a placeholder for a signature-polymorphic instance
* (of MH.invokeExact, etc.) that the JVM does not reify.
* See comments on {@link MethodHandleNatives#linkMethod}.
*/
src/share/classes/java/lang/invoke/MethodHandleInfo.java:
src/share/classes/java/lang/invoke/MethodHandles.java:
+import java.security.*;
This import isn't used.
Fixed.
src/share/classes/java/lang/invoke/MethodHandles.java:
+public MethodHandleInfo revealDirect(MethodHandle target) {
...
+byte refKind = member.getReferenceKind();
...
+// Check SM permissions and member access before cracking.
+try {
+//@@checkSecurityManager(defc, member, lookupClass());
+checkSecurityManager(defc, member);
+checkAccess(member.getReferenceKind(), defc, member);
+} catch (IllegalAccessException ex) {
+throw new IllegalArgumentException(ex);
+}
You prepare a separate refKind for MethodHandleInfo, but perform security
checks against original member.getReferenceKind(). Is it intentional?
No, it's bug. Thanks for catching that.
src/share/classes/java/lang/invoke/InfoFromMemberName.java:
81 public T extends Member T reflectAs(ClassT expected, Lookup
lookup) {
82 if (member.isMethodHandleInvoke() !member.isVarargs()) {
83 // this member is an instance of a signature-polymorphic
method, which cannot be reflected
84 throw new IllegalArgumentException(cannot reflect signature
polymorphic method);
Please, add a comment why (!member.isVarargs()) check is necessary.
// This member is an instance of a signature-polymorphic method,
which cannot be reflected
// A method handle invoker can come in either of two forms:
// A generic placeholder (present in the source code, and varargs)
// and a signature-polymorphic instance (synthetic and not
varargs).
// For more information see comments on {@link
MethodHandleNatives#linkMethod}.
src/share/classes/java/lang/invoke/InfoFromMemberName.java:
127 private void checkAccess(Member mem, Lookup lookup) throws
IllegalAccessException {
128 byte refKind = (byte) getReferenceKind();
129 if (mem instanceof Method) {
130 boolean wantSpecial = (refKind == REF_invokeSpecial);
131 lookup.checkAccess(refKind, getDeclaringClass(), new
MemberName((Method) mem, wantSpecial));
132 } else if (mem instanceof Constructor) {
133 lookup.checkAccess(refKind, getDeclaringClass(), new
MemberName((Constructor) mem));
134 } else if (mem instanceof Field) {
135 boolean isSetter = (refKind == REF_putField || refKind ==
REF_putStatic);
136 lookup.checkAccess(refKind, getDeclaringClass(), new
MemberName((Field) mem, isSetter));
137 }
138 }
Can you introduce a factory method to convert a Member instance into
MemberName and call lookup.checkAccess(refKind, getDeclaringClass(),
covertToMemberName(mem)) instead? It'll considerably simplify the code and
make the intention clearer.
Good idea. Done.
— John
Best regards,
Vladimir Ivanov
On 6/27/13
Re: RFR(L): 8008688: Make MethodHandleInfo public
John,
I have some minor suggestions about code style and code readability.
Otherwise, the change looks good!
src/share/classes/java/lang/invoke/MemberName.java:
public MemberName(Method m, boolean wantSpecial) {
...
MethodHandleNatives.init(this, m);
+if (clazz == null) { // MHN.init failed
+if (m.getDeclaringClass() == MethodHandle.class
+isMethodHandleInvokeName(m.getName())) {
Please, add a comment with a short description why a custom init
procedure for MH.invoke* cases is needed.
+/** Create a name for a signature-polymorphic invoker. */
+static MemberName makeMethodHandleInvoke(String name, MethodType
type) {
+return makeMethodHandleInvoke(name, type, MH_INVOKE_MODS |
SYNTHETIC);
Please, add a comment why SYNTHETIC flag is necessary.
src/share/classes/java/lang/invoke/MethodHandleInfo.java:
src/share/classes/java/lang/invoke/MethodHandles.java:
+import java.security.*;
This import isn't used.
src/share/classes/java/lang/invoke/MethodHandles.java:
+public MethodHandleInfo revealDirect(MethodHandle target) {
...
+byte refKind = member.getReferenceKind();
...
+// Check SM permissions and member access before cracking.
+try {
+//@@checkSecurityManager(defc, member, lookupClass());
+checkSecurityManager(defc, member);
+checkAccess(member.getReferenceKind(), defc, member);
+} catch (IllegalAccessException ex) {
+throw new IllegalArgumentException(ex);
+}
You prepare a separate refKind for MethodHandleInfo, but perform
security checks against original member.getReferenceKind(). Is it
intentional?
src/share/classes/java/lang/invoke/InfoFromMemberName.java:
81 public T extends Member T reflectAs(ClassT expected, Lookup
lookup) {
82 if (member.isMethodHandleInvoke() !member.isVarargs()) {
83 // this member is an instance of a signature-polymorphic
method, which cannot be reflected
84 throw new IllegalArgumentException(cannot reflect
signature polymorphic method);
Please, add a comment why (!member.isVarargs()) check is necessary.
src/share/classes/java/lang/invoke/InfoFromMemberName.java:
127 private void checkAccess(Member mem, Lookup lookup) throws
IllegalAccessException {
128 byte refKind = (byte) getReferenceKind();
129 if (mem instanceof Method) {
130 boolean wantSpecial = (refKind == REF_invokeSpecial);
131 lookup.checkAccess(refKind, getDeclaringClass(), new
MemberName((Method) mem, wantSpecial));
132 } else if (mem instanceof Constructor) {
133 lookup.checkAccess(refKind, getDeclaringClass(), new
MemberName((Constructor) mem));
134 } else if (mem instanceof Field) {
135 boolean isSetter = (refKind == REF_putField || refKind
== REF_putStatic);
136 lookup.checkAccess(refKind, getDeclaringClass(), new
MemberName((Field) mem, isSetter));
137 }
138 }
Can you introduce a factory method to convert a Member instance into
MemberName and call lookup.checkAccess(refKind, getDeclaringClass(),
covertToMemberName(mem)) instead? It'll considerably simplify the code
and make the intention clearer.
Best regards,
Vladimir Ivanov
On 6/27/13 10:00 AM, John Rose wrote:
This change implements the MethodHandleInfo API for cracking a direct method
handle back into its symbolic reference components. A DMH is any
CONSTANT_MethodHandle or any result of a Lookup.find* or Lookup.unreflect*
API call.
http://cr.openjdk.java.net/~jrose/8008688/webrev.04
Problem:
JDK 8 (esp. Project Lambda) needs a stable API for cracking
CONSTANT_MethodHandle constants that are involved with lambda capture sites
(which are implemented with invokedynamic).
Solution:
Create, specify, implement, and test such an API. Run the API design past
the 292 EG, the Project Lambda folks, and the Oracle internal review council
(CCC).
Testing:
Regular JSR 292 regression tests, plus a new jtreg test with positive and 3
kinds of negative tests, in hundreds of combinations. (See below.)
— John
P.S. Output from RevealDirectTest.java. (It runs with and without a security
manager.)
@Test testSimple executed 107 positive tests in 446 ms
@Test testPublicLookup/1 executed 56 positive tests in 99 ms
@Test testPublicLookup/2 executed 80 positive tests in 551 ms
@Test testPublicLookup/3 executed 56 positive tests in 47 ms
@Test testPublicLookupNegative/1 executed 23/0/0 negative tests in 2 ms
@Test testPublicLookupNegative/2 executed 0/27/0 negative tests in 4 ms
@Test testPublicLookupNegative/3 executed 0/0/27 negative tests in 10 ms
@Test testJavaLangClass executed 60 positive tests in 67 ms
@Test testCallerSensitive executed 30 positive tests in 425 ms
@Test testCallerSensitiveNegative executed 12/0/0 negative tests in
RFR(L): 8008688: Make MethodHandleInfo public
This change implements the MethodHandleInfo API for cracking a direct method handle back into its symbolic reference components. A DMH is any CONSTANT_MethodHandle or any result of a Lookup.find* or Lookup.unreflect* API call. http://cr.openjdk.java.net/~jrose/8008688/webrev.04 Problem: JDK 8 (esp. Project Lambda) needs a stable API for cracking CONSTANT_MethodHandle constants that are involved with lambda capture sites (which are implemented with invokedynamic). Solution: Create, specify, implement, and test such an API. Run the API design past the 292 EG, the Project Lambda folks, and the Oracle internal review council (CCC). Testing: Regular JSR 292 regression tests, plus a new jtreg test with positive and 3 kinds of negative tests, in hundreds of combinations. (See below.) — John P.S. Output from RevealDirectTest.java. (It runs with and without a security manager.) @Test testSimple executed 107 positive tests in 446 ms @Test testPublicLookup/1 executed 56 positive tests in 99 ms @Test testPublicLookup/2 executed 80 positive tests in 551 ms @Test testPublicLookup/3 executed 56 positive tests in 47 ms @Test testPublicLookupNegative/1 executed 23/0/0 negative tests in 2 ms @Test testPublicLookupNegative/2 executed 0/27/0 negative tests in 4 ms @Test testPublicLookupNegative/3 executed 0/0/27 negative tests in 10 ms @Test testJavaLangClass executed 60 positive tests in 67 ms @Test testCallerSensitive executed 30 positive tests in 425 ms @Test testCallerSensitiveNegative executed 12/0/0 negative tests in 1 ms @Test testMethodHandleNatives executed 4 positive tests in 5 ms @Test testMethodHandleInvokes/1 executed 640 positive tests in 828 ms @Test testMethodHandleInvokes/2 executed 640 positive tests in 177 ms ___ mlvm-dev mailing list mlvm-dev@openjdk.java.net http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
