On Mar 19, 2013, at 6:02 PM, Christian Thalinger <christian.thalin...@oracle.com> wrote:
> > On Mar 19, 2013, at 5:21 PM, John Rose <john.r.r...@oracle.com> wrote: > >> On Mar 14, 2013, at 8:31 PM, Christian Thalinger >> <christian.thalin...@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::is_ignored_by_security_stack_walk >> I would like to see reflect_invoke_cache go away some day. Would it be >> possible to strengthen the asserts to prove that it is an expensive alias >> for an intrinsic_id check? (I realize this is a question mainly for folks >> working on JVMTI.) > > That's what I tried to do: if the intrinsic_id == _invoke it also must be > the same method in reflect_invoke_cache. More than that would mean to > enhance ActiveMethodOopsCache because you can't ask for methods in the cache. > >> >> --- JVM_GetCallerClass >> 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::make_unloaded. 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. > >> >> --- JVM_GetClassContext >> What do these lines do: >> + // Collect method holders >> + GrowableArray<KlassHandle>* klass_array = new >> GrowableArray<KlassHandle>(); >> >> It looks like a paste-o from another source base. > > Left over. I filed an RFE for that improvement: > > JDK-8010124: JVM_GetClassContext: use GrowableArray instead of KlassLink > >> >> --- LibraryCallKit::inline_native_Reflection_getCallerClass >> >> 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 "caller_jvm=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 JVM_GetCallerClass). > if (!m->caller_sensitive()) { > #ifndef PRODUCT > if ((PrintIntrinsics || PrintInlining || PrintOptoInlining) && > Verbose) { > tty->print_cr(" Bailing out: CallerSensitive annotation expected at > frame %d", n); > } > #endif > return false; // bail-out; let JVM_GetCallerClass 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. >> >> --- JVM_RegisterUnsafeMethods >> 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::security_get_caller_frame >> 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
