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

[serguei.spitsyn at oracle.com]

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