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

[Joe Darcy]

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.
>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>