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

[Dmitry Samersoff]

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


-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.