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

[serguei.spitsyn at oracle.com]

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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20141208/575f3b5e/attachment.html>