2d reviewer please . Re: RFR : JDK-8046545 launcher fix to check function return values (original) (raw)

Joe Darcy joe.darcy at oracle.com
Wed Jul 23 23:12:59 UTC 2014

Hi Neil,

On 07/23/2014 03:50 PM, Neil Toda wrote:

I'm hoping some one will volunteer to be a 2d reviewer so we can satisfy jcheck requirement for a 2d review for this 8u patch. It is a very simple set of macros and a couple of applications that we hope to use going forward in making sure that JNI exceptions are caught in the launcher. http://cr.openjdk.java.net/~ntoda/8046545/webrev-02/ Thanks

Hmm. From a usability perspective, I think I would prefer to see something like CHECK_JNI_RETURN0 as the macro that appears on the java.c sources so that the code could contain lines like

1185 CHECK_JNI_RETURN0(makePlatformStringMID = (*env)->GetStaticMethodID(env, 1187 cls, "makePlatformString", "(Z[B)Ljava/lang/String;"));

rather than

1185 CHECK_JNI_RETURN( 1186 makePlatformStringMID = (*env)->GetStaticMethodID(env, 1187 cls, "makePlatformString", "(Z[B)Ljava/lang/String;"), 1188 RETURN0 1189 );

My C macro skills are rusty, but isn't it recommended practice to define something like RETURN(N) as

278 #define RETURN(N) return (N)

rather than

278 #define RETURN(N) return N

to reduce the risk of bad things happening on the macro expansion of N?

-Joe

-neil On 7/21/2014 9:31 AM, Neil Toda wrote:

Hi Kumar Actually, the null check macros have different parameters. NCRVreturnvalue is an integer. RETURNVALUE in CHECKJNIRETURN is a macro, which allows us to have only the two macros: CHECKJNIRETURN and CHECKJNIRETRUNEXCEPTION I also think it is cleaner since there are only two, and they are for JNI, to keep them self contained. Would someone be willing to review webrev-02, which contain Kumar's suggested change in the comments included with the macros. Thanks -neil

On 7/19/2014 8:02 AM, Kumar Srinivasan wrote: [ Since I am sponsoring this patch, I think jcheck needs one more Reviewer besides myself] Neil, looking at your webrev: http://cr.openjdk.java.net/~ntoda/8046545/webrev-02/ Can we not re-use the existing macro for null check ? #define NULLCHECKRETURNVALUE(NCRVcheckpointer, NCRVreturnvalue) so thus your new macro would become.... _+#define CHECKJNIRETURN(JNIRETURN, RETURNVALUE) _ _+ CHECKJNIRETURNEXCEPTION(RETURNVALUE); _ _- do { _ _- if ((JNIRETURN) == NULL) { _ _- JLIReportErrorMessage(JNIERROR); _ _- RETURNVALUE; _ _- } _ - } while (JNIFALSE) + NULLCHECKRETURNVALUE(JNIRETURN, RETURNVALUE);

Kumar On 7/18/2014 10:40 AM, Neil Toda wrote: Thanks Kumar. Yes, misspoke here. Will correct the comment. On 7/18/2014 10:35 AM, Kumar Srinivasan wrote: Neil, The fix looks good. However there is an inaccuracy in the comment:

+ * Normally, JNI calls do not return if an exception is thrown. + * However, this behavior can change in the future, + * so check for thrown exceptions. This is not true, JNI calls will return if an exception is thrown, however best JNI practices dictate that a pending Exception(s) must be cleared or caught, before attempting another JNI call. Under such circumstances the return value will usually be an error or a null value. I suggest making this change to reflect this. Thanks Kumar

On 7/18/2014 9:53 AM, Neil Toda wrote: Please review this launcher change. Issue: https://bugs.openjdk.java.net/browse/JDK-8046545 webrev: http://cr.openjdk.java.net/~ntoda/8046545/webrev-01/ Summary: Introduce a set of macros for launcher to be used to check for certain conditions after return from select functions.

More information about the core-libs-dev mailing list