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.
NCRV_return_value is an integer.
RETURNVALUE in CHECK_JNI_RETURN is a macro, which allows us to have
only the two macros:
CHECK_JNI_RETURN
and
CHECK_JNI_RETRUN_EXCEPTION
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 NULL_CHECK_RETURN_VALUE(NCRV_check_pointer, NCRV_return_value)
so thus your new macro would become....
+#define CHECK_JNI_RETURN(JNIRETURN, RETURNVALUE) \
+ CHECK_JNI_RETURN_EXCEPTION(RETURNVALUE); \
- do { \
- if ((JNIRETURN) == NULL) { \
- JLI_ReportErrorMessage(JNI_ERROR); \
- RETURNVALUE; \
- } \
- } while (JNI_FALSE)
+ NULL_CHECK_RETURN_VALUE(JNI_RETURN, RETURN_VALUE);
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.
