Re: RFR: 8161448: 4 JNI exception pending defect groups in DiagnosticCommandImpl.c

[Harsha Wardhana B]

Hi David,

Please find revised webrev.

http://cr.openjdk.java.net/~hb/8161448/webrev.03/

-Harsha

On Thursday 01 September 2016 03:31 PM, David Holmes wrote:

On 1/09/2016 6:05 PM, Harsha Wardhana B wrote:

Hi David,

On Thursday 01 September 2016 01:14 PM, David Holmes wrote:

Hi Harsha,

Sorry these style issues are proving to be so painful, normally there
would be more direct guidance from an existing team member.

On 30/08/2016 4:51 PM, Harsha Wardhana B wrote:

Hello,

Please find below webrev addressing David's and Dmitry's comments.

http://cr.openjdk.java.net/~hb/8161448/webrev.02/


The idiomatic way we write such macros is as follows:

#define EXCEPTION_CHECK_AND_FREE(x) \
  do { \
if ((*env)->ExceptionCheck(env)) { \
   free(x); \
   return NULL; \
} \
  } while (false)

I am aware of this style but I wasn't aware what style should be used
(BTW last line should be 'while(0)'). I didn't want to do anything fancy
and end up sending one more webrev and hence I followed the simplest way
to write a multi-line macro.
Now if there isn't anything wrong, I think we should leave it as is
until coding guidelines are put in place. I am facing same problem in
another issue where I have sent multiple webrev fixing only nits. It is
turning out to be very painful.


I understand and it is unfortunate that there has not been more 
definitive guidance here. But it is better to adopt established style 
and get used to using it. If you form is left in then you need spaces 
before trailing \ - another style nit, sorry.




---

 109 if (obj == NULL) {
 110 EXCEPTION_CHECK_AND_FREE(dcmd_arg_info_array);
 111 }

Checking both NULL and for exceptions is redundant as previously
pointed out. Either leave this code as it was:

Yes. it may be redundant but it is harmless. Does this really need to be
changed?


Yes it breeds confusion to see multiple checks that are unnecessary 
(and we're trying to address this with a lot of the JNI using code in 
the VM at the moment). In this case it is even worse because without 
knowing the exception check must be true and so "return NULL" will 
always execute, this code has the appearance of being able to continue 
if obj == NULL!


Sorry. And thanks.

David
-



91 if (obj == NULL) {
92   free(dcmd_arg_info_array);
93   return NULL;
94 }

or else just replace it with:

109EXCEPTION_CHECK_AND_FREE(dcmd_arg_info_array);

The same for the block here:

 201   if (obj == NULL){
 202 EXCEPTION_CHECK_AND_FREE(dcmd_info_array);
 203   }


Thanks,
David
-


-Harsha

On Tuesday 30 August 2016 09:46 AM, David Holmes wrote:

On 30/08/2016 2:06 PM, Harsha Wardhana B wrote:

Error checking might distract the main flow of code but it would be
far-fetched to call that it obfuscates the code. Ideally error
checking


You claimed macros obfuscate so the same word seemed appropriate. I
don't necessarily equate readability with obfuscation.


could have been made a function (inline if possible) instead of a
macro
but since it is context sensitive in this case (have to free 
variables

depending on context) , doing so would obfuscate the code even more.

I tried to separate it into a function and the code looks more 
uglier.


I was envisaging something like:

  jstring jname, jdesc,jtype,jdefStr;
  jname = (*env)->NewStringUTF(env,dcmd_arg_info_array[i].name);
  EXCEPTION_CHECK_AND_FREE(dcmd_arg_info_array);
  jdesc =
(*env)->NewStringUTF(env,dcmd_arg_info_array[i].description);
  EXCEPTION_CHECK_AND_FREE(dcmd_arg_info_array);
  jtype = (*env)->NewStringUTF(env,dcmd_arg_info_array[i].type);
  EXCEPTION_CHECK_AND_FREE(dcmd_arg_info_array);
  jdefStr = (*env)->NewStringUTF(env,
dcmd_arg_info_array[i].default_string);
  EXCEPTION_CHECK_AND_FREE(dcmd_arg_info_array);

Of course if this were C++ code instead of C there would be better
techniques for dealing with the free'ing.

Cheers,
David



-Harsha
On Tuesday 30 August 2016 09:26 AM, David Holmes wrote:

On 30/08/2016 1:47 PM, Harsha Wardhana B wrote:

Hi Dmitry,

I am not sure how using macro will help in this context. As far 
as I

know, macros must be sparingly used as they are error-prone,
obfuscate
the code and hard to debug. Most of best coding practice for c/c++
(inluding Scott Myers Effective C++ ) advocate using macros only
if it
is absolutely needed.


Macros are used extensively throughout hotspot and the JDK native
code. That isn't to say that all such macro uses are good (we have
bad
code too). However macros make sense where you want to avoid code
duplication and improve readability - as is the case here. It is
quite
common to "hide" error checking logic behind a macro because it is
the
error-checking logic that obfuscates the code.

David
-


-Harsha


On Monday 29 August 2016 02:00 PM, Dmitry Samersoff wrote:

Harsha,

I'm for macro.

You can define a macro right before a place where you uses it and
undef
it when you 

Re: RFR: JDK-8164913: JVMTI.agent_load dcmd should show useful error message

[David Holmes]

Hi Yasumasa,

On 1/09/2016 1:47 PM, Yasumasa Suenaga wrote:

Hi all,

I think RDP1 has been started.
Cannot I fix this?


"P4-P5 bugs should, in general, be left to future releases unless they
only affect documentation, demos, or tests, in which case they should be
identified as such with the "noreg-doc", "noreg-demo", and "noreg-self"
labels, respectively." [1]

This is due to the need to stabilize changes as we head towards GA.

I think you would have to justify why this should be a P3 rather than P4 
issue. Is this something specific to code added in 9? If so that is 
motivation as we should not have a new feature with known bugs if we can 
avoid it. But if this already exists in JDK 8 and is a long standing 
"bug" ...


David

[1] http://mail.openjdk.java.net/pipermail/jdk9-dev/2016-August/004777.html



This problem is that jcmd shows incorrect status when JVMTI agent cannot
be attached.
I think this problem should be fixed in 9 GA.
The users who want.to attach JVMTI agent want to know
whether it succeed.

Yasumasa


2016/08/29 15:42 "Yasumasa Suenaga" >:

This comment no longer matches the code and should be deleted:

2412   // Agent_OnAttach executed so completion status is JNI_OK
2413   st->print_cr("return code: %d", result);


Thanks David!
I removed it in new webrev.

  http://cr.openjdk.java.net/~ysuenaga/JDK-8164913/webrev.01/



Yasumasa


On 2016/08/29 12:59, David Holmes wrote:

Hi Yasumasa,

On 28/08/2016 10:47 PM, Yasumasa Suenaga wrote:

Hi all,

If we try to attach invalid JVMTI agent via JVMTI.agent_load
dcmd, we
will get
"Command executed successfully". However, it implies error in
JVMTIAgentLoadDCmd.

This message is from JCmd.java when jcmd does not receive
output from
target VM.
So we should send error message from JVMTIAgentLoadDCmd.

I uploaded a webrev for it. Could you review it?


http://cr.openjdk.java.net/~ysuenaga/JDK-8164913/webrev.00/



This seems reasonable.

src/share/vm/prims/jvmtiExport.cpp

This comment no longer matches the code and should be deleted:

2412   // Agent_OnAttach executed so completion status is JNI_OK
2413   st->print_cr("return code: %d", result);

Thanks,
David

I cannot access JPRT.
So I need a sponsor.


Thanks,

Yasumasa

Re: PING: RFR: JDK-8164913: JVMTI.agent_load dcmd should show useful error message

[Dmitry Samersoff]

Yasumasa,

I'll look closely to the fix.

Please, notice:

1. We typically avoid printing attach error messages on
the target VM side.

2. We are in RDP1 for jdk9.

   http://mail.openjdk.java.net/pipermail/jdk9-dev/2016-August/004777.html

-Dmitry

On 2016-09-05 16:25, Yasumasa Suenaga wrote:
> PING: Could you review and sponsor it?
> 
>>   http://cr.openjdk.java.net/~ysuenaga/JDK-8164913/webrev.01/
> 
> 
> Thanks,
> 
> Yasumasa
> 
> 
> On 2016/09/01 12:47, Yasumasa Suenaga wrote:
>> Hi all,
>>
>> I think RDP1 has been started.
>> Cannot I fix this?
>>
>> This problem is that jcmd shows incorrect status when JVMTI agent
>> cannot be attached.
>> I think this problem should be fixed in 9 GA.
>> The users who want.to  attach JVMTI agent want to know
>> whether it succeed.
>>
>> Yasumasa
>>
>>
>> 2016/08/29 15:42 "Yasumasa Suenaga" > >:
>>
>> This comment no longer matches the code and should be deleted:
>>
>> 2412   // Agent_OnAttach executed so completion status is
>> JNI_OK
>> 2413   st->print_cr("return code: %d", result);
>>
>>
>> Thanks David!
>> I removed it in new webrev.
>>
>>   http://cr.openjdk.java.net/~ysuenaga/JDK-8164913/webrev.01/
>> 
>>
>>
>> Yasumasa
>>
>>
>> On 2016/08/29 12:59, David Holmes wrote:
>>
>> Hi Yasumasa,
>>
>> On 28/08/2016 10:47 PM, Yasumasa Suenaga wrote:
>>
>> Hi all,
>>
>> If we try to attach invalid JVMTI agent via
>> JVMTI.agent_load dcmd, we
>> will get
>> "Command executed successfully". However, it implies error in
>> JVMTIAgentLoadDCmd.
>>
>> This message is from JCmd.java when jcmd does not receive
>> output from
>> target VM.
>> So we should send error message from JVMTIAgentLoadDCmd.
>>
>> I uploaded a webrev for it. Could you review it?
>>
>>  
>> http://cr.openjdk.java.net/~ysuenaga/JDK-8164913/webrev.00/
>> 
>>
>>
>> This seems reasonable.
>>
>> src/share/vm/prims/jvmtiExport.cpp
>>
>> This comment no longer matches the code and should be deleted:
>>
>> 2412   // Agent_OnAttach executed so completion status is
>> JNI_OK
>> 2413   st->print_cr("return code: %d", result);
>>
>> Thanks,
>> David
>>
>> I cannot access JPRT.
>> So I need a sponsor.
>>
>>
>> Thanks,
>>
>> Yasumasa
>>


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

PING: RFR: JDK-8164913: JVMTI.agent_load dcmd should show useful error message

[Yasumasa Suenaga]

PING: Could you review and sponsor it?


  http://cr.openjdk.java.net/~ysuenaga/JDK-8164913/webrev.01/



Thanks,

Yasumasa


On 2016/09/01 12:47, Yasumasa Suenaga wrote:

Hi all,

I think RDP1 has been started.
Cannot I fix this?

This problem is that jcmd shows incorrect status when JVMTI agent cannot be 
attached.
I think this problem should be fixed in 9 GA.
The users who want.to  attach JVMTI agent want to know whether 
it succeed.

Yasumasa


2016/08/29 15:42 "Yasumasa Suenaga" >:

This comment no longer matches the code and should be deleted:

2412   // Agent_OnAttach executed so completion status is JNI_OK
2413   st->print_cr("return code: %d", result);


Thanks David!
I removed it in new webrev.

  http://cr.openjdk.java.net/~ysuenaga/JDK-8164913/webrev.01/ 



Yasumasa


On 2016/08/29 12:59, David Holmes wrote:

Hi Yasumasa,

On 28/08/2016 10:47 PM, Yasumasa Suenaga wrote:

Hi all,

If we try to attach invalid JVMTI agent via JVMTI.agent_load dcmd, 
we
will get
"Command executed successfully". However, it implies error in
JVMTIAgentLoadDCmd.

This message is from JCmd.java when jcmd does not receive output 
from
target VM.
So we should send error message from JVMTIAgentLoadDCmd.

I uploaded a webrev for it. Could you review it?

  http://cr.openjdk.java.net/~ysuenaga/JDK-8164913/webrev.00/ 



This seems reasonable.

src/share/vm/prims/jvmtiExport.cpp

This comment no longer matches the code and should be deleted:

2412   // Agent_OnAttach executed so completion status is JNI_OK
2413   st->print_cr("return code: %d", result);

Thanks,
David

I cannot access JPRT.
So I need a sponsor.


Thanks,

Yasumasa