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

[Neil Toda]

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