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

[Christian Thalinger]

On Mar 19, 2013, at 1:14 PM, Mandy Chung <mandy.chung at oracle.com> wrote:

> I do a partial review in particular to make sure the jdk and hotspot change are in sync.
> 
> javaClasses.hpp - MN_CALLER_SENSITIVE and MN_SEARCH_SUPERCLASSES have the same value.  Should they be different?
> 
> 1057     MN_CALLER_SENSITIVE     = 0x00100000, // @CallerSensitive annotation detected
> 1061     MN_SEARCH_SUPERCLASSES  = 0x00100000, // walk super classes

They can have the same value because they are used for different things in different places.  I talked to John about this and he said that the MN_SEARCH_* guys don't add much value and might be removed.

> 
> 
> method.hpp - Is caller_sensitive set to true if @CallerSensitive annotation is present and must be loaded by null class loader?  Does it only check annotations if the class of that method is defined by the null class loader?  Per our offline discussion early, classes loaded by the extension class loader may also be caller-sensitive.

Right.  I forgot to add that code.  Here is an incremental webrev:

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

And the full thing:

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

Let me know if that works for you.

> 
> If a method calls Reflection.getCallerClass but its class is defined by other class loader (non-null and not extension class loader), your fix will throw InternalError with the same error message even if that method is annotated with @CS.  Is there a way to improve the error message so that we can differentiate this case (i.e. @CS is present but not supported)?

Not easily.  We set a flag on the method when parse the class.  At that point we decide if the annotation is there or not.  If the annotation is not allowed in parsed class because it's not loaded by the right class loader then it does not "exist".

> 
> jvm.cpp: have you considered adding a new entry point instead of having JVM_GetCallerClass to behave differently depending on the existence of sun.reflect.CallerSensitive class? There are pros and cons of both options. Having a new entry point is cleaner and enables the opportunity to remove JVM_GetCallerClass(int) in the future.  I am fine with either approach but just to bring it up.

Yes.  I talked to Vladimir about that yesterday.  The better solution seems to be to leave the old entry point.  If we add a new one that kind of adds a new method to the "official" VM interface.  Other VM implementors would have to implement that one as well because the JDK then links to this new method.

Thanks for the review!

-- Chris

> 
> Mandy
> 
> 
> On 3/14/13 8:31 PM, Christian Thalinger 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:
>> 
>> More information in JEP 176:
>> 
>> http://openjdk.java.net/jeps/176
>> 
>> src/share/vm/ci/ciMethod.cpp
>> src/share/vm/ci/ciMethod.hpp
>> src/share/vm/classfile/classFileParser.cpp
>> src/share/vm/classfile/classFileParser.hpp
>> src/share/vm/classfile/javaClasses.hpp
>> src/share/vm/classfile/systemDictionary.hpp
>> src/share/vm/classfile/vmSymbols.hpp
>> src/share/vm/oops/method.cpp
>> src/share/vm/oops/method.hpp
>> src/share/vm/opto/library_call.cpp
>> src/share/vm/prims/jvm.cpp
>> src/share/vm/prims/methodHandles.cpp
>> src/share/vm/prims/unsafe.cpp
>> src/share/vm/runtime/vframe.cpp
>> src/share/vm/runtime/vframe.hpp
>> 
>