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

[Neil Toda]

Got it backwards.  Will use

     #define RETURN(N) return (N)

On 7/24/2014 9:06 AM, Neil Toda wrote:
>
> Thanks Joe and Kumar
>
> It would be better in terms of usability.
>
> I'll have these three macros:
>
>     CHECK_JNI_RETURN_0
>     CHECK_JNI_RETURN_VOID
>     CHECK_JNI_RETURN_VALUE
>
> With this change, I can use
>
>     NULL_CHECK_RETURN_VALUE
>
> as you suggested Kumar.
>
> I'll also use use the cleaner define for RETURN(N)
>
>     #define RETURN(N) return N
>
> -neil
>
> On 7/23/2014 4:12 PM, Joe Darcy wrote:
>> 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.
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>