RFR(S) 8028773: SA, warnings from b116 for hotspot.agent.src.share.native: JNI exception pending

[Staffan Larsen]

Looks good!

Thanks,
/Staffan


> On 9 dec 2014, at 15:10, serguei.spitsyn at oracle.com wrote:
> 
> Dmitry,
> 
> It looks good.
> 
> Than you for the update!
> Serguei
> 
> On 12/9/14 5:51 AM, Dmitry Samersoff wrote:
>> Serguei,
>> 
>> Fixed. Webrev updated in-place, please press shift-reload.
>> 
>> http://cr.openjdk.java.net/~dsamersoff/JDK-8028773/webrev.03/
>> 
>> -Dmitry
>> 
>> On 2014-12-08 14:02, serguei.spitsyn at oracle.com wrote:
>>> Looks good.
>>> 
>>> Some minor comments.
>>> 
>>> Better to reformat with one variable at a line:
>>> 
>>>  113   const char *error_message = NULL, *jrepath = NULL, *libname = NULL;
>>> 
>>>  283   jbyte *start, *end;
>>> 
>>> 
>>> Uninitialized locals:
>>> 
>>>  115 #ifdef _WINDOWS
>>>  116   HINSTANCE hsdis_handle;
>>>  117 #else
>>>  118   void* hsdis_handle;
>>>  119 #endif
>>>  ...
>>>  201   jlong result;
>>>  ...
>>> 
>>>  282   jboolean isCopy;
>>>  283   jbyte *start, *end;
>>>  284   jclass disclass;
>>>  285   const char *options;
>>> 
>>> 
>>> Unaligned parameters:
>>> 
>>>  209   result = (*env)->CallLongMethod(env, denv->dis, denv->handle_event, denv->visitor,
>>>  210                                         event_string, (jlong) (uintptr_t)arg);
>>> 
>>> 
>>> Thanks,
>>> Serguei
>>> 
>>> 
>>> On 12/7/14 6:17 AM, Dmitry Samersoff wrote:
>>>> Please, review a modified fix:
>>>> 
>>>> http://cr.openjdk.java.net/~dsamersoff/JDK-8028773/webrev.03/
>>>> 
>>>> Windows compiler doesn't allow declaration in the middle of the function
>>>> for c code.
>>>> 
>>>> Also I put my few cents to reduce build noise and add a pragma that
>>>> disable windows compiler warnings that have no value in this context.
>>>> 
>>>> -Dmitry
>>>> 
>>>> 
>>>> On 2014-12-03 16:01, Staffan Larsen wrote:
>>>>> Changes look good. What testing have you done?
>>>>> 
>>>>> /Staffan
>>>>> 
>>>>>> On 3 dec 2014, at 13:06, Dmitry Samersoff <dmitry.samersoff at oracle.com> wrote:
>>>>>> 
>>>>>> Serguei,
>>>>>> 
>>>>>> Updated webrev
>>>>>> 
>>>>>> http://cr.openjdk.java.net/~dsamersoff/JDK-8028773/webrev.02/
>>>>>> 
>>>>>> -Dmitry
>>>>>> 
>>>>>> On 2014-12-03 01:24, serguei.spitsyn at oracle.com wrote:
>>>>>>> Dmitry,
>>>>>>> 
>>>>>>> It is good in general modulo Staffan's comments.
>>>>>>> 
>>>>>>> There are some inconsistencies:
>>>>>>> -  the ExceptionOccurred(env) is compared to != NULL (which make the
>>>>>>> logic more complex)
>>>>>>>     in some places and no such comparison (implicit comparison instead)
>>>>>>> in others
>>>>>>> - two different forms of check/action are used:
>>>>>>>      -  (*env)->ExceptionOccurred(env) and
>>>>>>>      -  !(*env)->ExceptionOccurred(env)
>>>>>>> 
>>>>>>> I'd suggest to do it the same way and always return the "default" value
>>>>>>> if an exception occurred.
>>>>>>> It will make the logic more flat and clear.
>>>>>>> Otherwise, we have to think what exact value is returned in exception
>>>>>>> occurred cases like at lines 241, 255.
>>>>>>> 
>>>>>>> The comment // OOM is used inconsistently too.
>>>>>>> 
>>>>>>> 
>>>>>>> Thanks,
>>>>>>> Serguei
>>>>>>> 
>>>>>>> 
>>>>>>> On 12/2/14 11:14 AM, Dmitry Samersoff wrote:
>>>>>>>> Please review the small fix.
>>>>>>>> 
>>>>>>>> http://cr.openjdk.java.net/~dsamersoff/JDK-8028773/webrev.01/
>>>>>>>> 
>>>>>>>> Added more missed exception checks to sadis.c
>>>>>>>> 
>>>>>> -- 
>>>>>> Dmitry Samersoff
>>>>>> Oracle Java development team, Saint Petersburg, Russia
>>>>>> * I would love to change the world, but they won't give me the sources.
>> 
>