RFR (M): 7198429: need checked categorization of caller-sensitive methods in the JDK (original) (raw)

Christian Thalinger christian.thalinger at oracle.com
Fri Mar 22 23:10:03 UTC 2013

On Mar 19, 2013, at 6:02 PM, Christian Thalinger <christian.thalinger at oracle.com> wrote:

On Mar 19, 2013, at 5:21 PM, John Rose <john.r.rose at oracle.com> wrote:

On Mar 14, 2013, at 8:31 PM, Christian Thalinger <christian.thalinger at oracle.com> wrote:

[This is the HotSpot part of JEP 176]

http://cr.openjdk.java.net/~twisti/7198429 7198429: need checked categorization of caller-sensitive methods in the JDK Reviewed-by: Over all, great work on a tricky problem. I'd add a few asserts and tweak a couple of lines; see below. Reviewed as is or with suggested changes. — John --- Method::isignoredbysecuritystackwalk I would like to see reflectinvokecache go away some day. Would it be possible to strengthen the asserts to prove that it is an expensive alias for an intrinsicid check? (I realize this is a question mainly for folks working on JVMTI.) That's what I tried to do: if the intrinsicid == invoke it also must be the same method in reflectinvokecache. More than that would mean to enhance ActiveMethodOopsCache because you can't ask for methods in the cache. --- JVMGetCallerClass Suggest an assert for vfst.method() == NULL. Should not happen, and previous code would apparently have crashed already, but... (The corner case I'm thinking of is a compiled frame with nmethod::method returning null after nmethod::makeunloaded. Should not happen.) Sure, I can add that assert but there is other code in jvm.cpp that relies on the fact that vfst.method() is non-null. We should add asserts in all that places but that's for another RFE. --- JVMGetClassContext What do these lines do: + // Collect method holders + GrowableArray* klassarray = new GrowableArray(); It looks like a paste-o from another source base. Left over. I filed an RFE for that improvement: JDK-8010124: JVMGetClassContext: use GrowableArray instead of KlassLink --- LibraryCallKit::inlinenativeReflectiongetCallerClass I believe this assertion, but I would prefer to see it checked more forcibly: + // NOTE: Start the loop at depth 1 because the current JVM state does + // not include the Reflection.getCallerClass() frame. Not sure there is a good way to do this. But, perhaps put the comment here: case 0: // ...comment... ShouldNotReachHere(); How about: case 0: fatal("current JVM state does not include the Reflection.getCallerClass() frame"); break; Also, if something goes wrong with caller sensitivity, we just get a "return false". Perhaps do a "callerjvm=NULL;break" to branch to the pretty failure message? That makes it slightly easier to see what happened. It seems easier to add printing code to the case statement: case 1: // Frame 0 and 1 must be caller sensitive (see JVMGetCallerClass). if (!m->callersensitive()) { #ifndef PRODUCT if ((PrintIntrinsics || PrintInlining || PrintOptoInlining) && Verbose) { tty->printcr(" Bailing out: CallerSensitive annotation expected at frame %d", n); } #endif return false; // bail-out; let JVMGetCallerClass do the work } break; The LogCompilation switch should leave a "paper trail". Actually, I see that LogCompilation doesn't mention failed intrinsic inlines. Rats. At least PrintInlining or PrintIntrinsics (diagnostic flags) will give us some leverage if we need to debug. --- JVMRegisterUnsafeMethods That's an improvement. Thanks. (A nagging worry: How big are those static tables getting?) We could remove some very old ones like 1.4.0 and 1.4.1. This time, next time? --- vframeStreamCommon::securitygetcallerframe This now does something odd if depth < 0. Suggest an assert. The behavior with depth < 0 in the current code is even worse. An assert is a good idea. As discussed I want to remove that method in the future because its uses are dubious.

I forgot to update the webrev. Here you go:

http://cr.openjdk.java.net/~twisti/7198429/

-- Chris

More information about the core-libs-dev mailing list