Re: RFR(S): URG! JDK-8049226 com/sun/jdi/OptionTest.java test times out again

[serguei.spit...@oracle.com]

Dmitry,

It makes sense to consider a special exit code for transport init error.
Other than that it looks good.
No need to re-review.

Thanks,
Serguei

On 8/26/14 10:46 AM, Dmitry Samersoff wrote:

Serguei,

exit_code set to 1 at ll. 1288

I thought of refactoring this code for better readability but finally
decide to keep changes minimal.

-Dmitry


On 2014-08-26 21:35, serguei.spit...@oracle.com wrote:

Dmitry,

This looks good in general.
But what error code will be returned in the case of
AGENT_ERROR_TRANSPORT_INIT ?
Is it Ok to return whatever exit_code we already have or we better
return some special error code?

Probably, it is Ok to keep the comment at the line 1315 as it is.


Thanks,
Serguei

On 8/26/14 7:47 AM, Dmitry Samersoff wrote:

Staffan,

http://cr.openjdk.java.net/~dsamersoff/JDK-8049226/webrev.03/

After couple of experiments I end up with simple fix - don't coredump on
AGENT_ERROR_TRANSPORT_INIT

Current code don't propagate transport error to upper level, so if we
need fine grained control I'll rewrite transport initialization.

-Dmitry


On 2014-08-25 15:53, Staffan Larsen wrote:

On 25 aug 2014, at 13:33, Dmitry Samersoff mailto:dmitry.samers...@oracle.com>> wrote:


Staffan,

On 2014-08-25 15:26, Staffan Larsen wrote:

Dmitry,

One problem with this fix is that debugInit_exit() is used from many
places in the JDWP code. For some of these I think it still does make
sense to receive a core dump for analysis. I agree that when parsing
options, we do not need a core dump.

jdwp has explicit option to request a coredump at debugInit_exit().

Is it enough or I should create a more sophisticated fix ?

I don’t think that is enough. Often a problem will happen and will not
be reproducible.

Maybe this code:

 if ((arg.error != JDWP_ERROR(NONE)) &&
 (arg.startCount == 0) &&
 initOnStartup) {
 EXIT_ERROR(map2jvmtiError(arg.error), "No transports initialized");
 }

can use some variant of EXIT_ERROR that does not call fatalError() ?

/Staffan


Actually, I don't think that coredump on every call to jni_FatalError()
(see jni.cpp:726) is necessary but this hotspot code is here for 6 years
and changing it probably out of scope of this fix.

-Dmitry



/Staffan


On 20 aug 2014, at 17:55, Dmitry Samersoff
mailto:dmitry.samers...@oracle.com>> wrote:


Serguei,

After some additional testing I changed the fix a bit. Please take
a look at new version.

http://cr.openjdk.java.net/~dsamersoff/JDK-8049226/webrev.02/

New version reports JVMTI error to stderr.

Also jniFatalError is not referenced any more so I remove it.

-Dmitry



On 2014-08-20 12:08, serguei.spit...@oracle.com wrote:

Ok.

Thank you for the explanation! Serguei

On 8/20/14 1:01 AM, Dmitry Samersoff wrote:

Serguei,

1. Historically JDI test-suite had no tests for failed
transport initialization behavior and invalid parameters
handling.

2. As a part of JDWP hardening work I added couple of such
tests to OptionTest.java - these tests pass invalid parameters
to dt_socket transport to make sure that transport doesn't
crash (one such crash was discovered and fixed) but just return
non-zero exit code to upper level.

3. After fix for JDK-6694099 any non-zero exit code from
transport cause VM to coredump. Dumping multiple cores on busy
machine takes a time so harness kills the test by timeout.

We can just increase timeout for this test but I don't think
it's a good idea to dump core when invalid parameters passed to
transport.

So there is the fix.

4. After the fix tests for negative parameters will return
non-zero exit code as expected but will not dump the core.

-Dmitry

On 2014-08-20 00:54, serguei.spit...@oracle.com wrote:

Hi Dmitry,

The fix seems to be Ok. Just want to make it clear... This
fix just changes the bug pattern. It a case of incorrect
transport parameters the test is still going to fail but
without crash, right?

Thanks, Serguei

On 8/19/14 12:09 PM, Dmitry Samersoff wrote:

Hi Everybody,

Please review the fix:

http://cr.openjdk.java.net/~dsamersoff/JDK-8049226/webrev.01/




JDWP call jniFatalError if transport can't be initialized (e.g. wrong

parameters) and jniFatalError call os::abort().  Therefor
all transport initialization errors cause vm to coredump.

I see no reason for debugInit_exit to call jniFatalError so
remove this code.

-Dmitry


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

Re: RFR(S): URG! JDK-8049226 com/sun/jdi/OptionTest.java test times out again

[Dmitry Samersoff]

Serguei,

exit_code set to 1 at ll. 1288

I thought of refactoring this code for better readability but finally
decide to keep changes minimal.

-Dmitry


On 2014-08-26 21:35, serguei.spit...@oracle.com wrote:
> Dmitry,
> 
> This looks good in general.
> But what error code will be returned in the case of
> AGENT_ERROR_TRANSPORT_INIT ?
> Is it Ok to return whatever exit_code we already have or we better
> return some special error code?
> 
> Probably, it is Ok to keep the comment at the line 1315 as it is.
> 
> 
> Thanks,
> Serguei
> 
> On 8/26/14 7:47 AM, Dmitry Samersoff wrote:
>> Staffan,
>>
>> http://cr.openjdk.java.net/~dsamersoff/JDK-8049226/webrev.03/
>>
>> After couple of experiments I end up with simple fix - don't coredump on
>> AGENT_ERROR_TRANSPORT_INIT
>>
>> Current code don't propagate transport error to upper level, so if we
>> need fine grained control I'll rewrite transport initialization.
>>
>> -Dmitry
>>
>>
>> On 2014-08-25 15:53, Staffan Larsen wrote:
>>> On 25 aug 2014, at 13:33, Dmitry Samersoff >> > wrote:
>>>
 Staffan,

 On 2014-08-25 15:26, Staffan Larsen wrote:
> Dmitry,
>
> One problem with this fix is that debugInit_exit() is used from many
> places in the JDWP code. For some of these I think it still does make
> sense to receive a core dump for analysis. I agree that when parsing
> options, we do not need a core dump.
 jdwp has explicit option to request a coredump at debugInit_exit().

 Is it enough or I should create a more sophisticated fix ?
>>> I don’t think that is enough. Often a problem will happen and will not
>>> be reproducible. 
>>>
>>> Maybe this code:
>>>
>>> if ((arg.error != JDWP_ERROR(NONE)) &&
>>> (arg.startCount == 0) &&
>>> initOnStartup) {
>>> EXIT_ERROR(map2jvmtiError(arg.error), "No transports initialized");
>>> }
>>>
>>> can use some variant of EXIT_ERROR that does not call fatalError() ?
>>>
>>> /Staffan
>>>
 Actually, I don't think that coredump on every call to jni_FatalError()
 (see jni.cpp:726) is necessary but this hotspot code is here for 6 years
 and changing it probably out of scope of this fix.

 -Dmitry


> /Staffan
>
>
> On 20 aug 2014, at 17:55, Dmitry Samersoff
> mailto:dmitry.samers...@oracle.com>> wrote:
>
>> Serguei,
>>
>> After some additional testing I changed the fix a bit. Please take
>> a look at new version.
>>
>> http://cr.openjdk.java.net/~dsamersoff/JDK-8049226/webrev.02/
>>
>> New version reports JVMTI error to stderr.
>>
>> Also jniFatalError is not referenced any more so I remove it.
>>
>> -Dmitry
>>
>>
>>
>> On 2014-08-20 12:08, serguei.spit...@oracle.com wrote:
>>> Ok.
>>>
>>> Thank you for the explanation! Serguei
>>>
>>> On 8/20/14 1:01 AM, Dmitry Samersoff wrote:
 Serguei,

 1. Historically JDI test-suite had no tests for failed
 transport initialization behavior and invalid parameters
 handling.

 2. As a part of JDWP hardening work I added couple of such
 tests to OptionTest.java - these tests pass invalid parameters
 to dt_socket transport to make sure that transport doesn't
 crash (one such crash was discovered and fixed) but just return
 non-zero exit code to upper level.

 3. After fix for JDK-6694099 any non-zero exit code from
 transport cause VM to coredump. Dumping multiple cores on busy
 machine takes a time so harness kills the test by timeout.

 We can just increase timeout for this test but I don't think
 it's a good idea to dump core when invalid parameters passed to
 transport.

 So there is the fix.

 4. After the fix tests for negative parameters will return
 non-zero exit code as expected but will not dump the core.

 -Dmitry

 On 2014-08-20 00:54, serguei.spit...@oracle.com wrote:
> Hi Dmitry,
>
> The fix seems to be Ok. Just want to make it clear... This
> fix just changes the bug pattern. It a case of incorrect
> transport parameters the test is still going to fail but
> without crash, right?
>
> Thanks, Serguei
>
> On 8/19/14 12:09 PM, Dmitry Samersoff wrote:
>> Hi Everybody,
>>
>> Please review the fix:
>>
>> http://cr.openjdk.java.net/~dsamersoff/JDK-8049226/webrev.01/
>>
>>
>>
 JDWP call jniFatalError if transport can't be initialized (e.g. wrong
>> parameters) and jniFatalError call os::abort().  Therefor
>> all transport initialization errors cause vm to coredump.
>>
>> I see no reason for debugInit_exit to call jniFatalError so
>> remove thi

Re: RFR(S): URG! JDK-8049226 com/sun/jdi/OptionTest.java test times out again

[serguei.spit...@oracle.com]

Dmitry,

This looks good in general.
But what error code will be returned in the case of 
AGENT_ERROR_TRANSPORT_INIT ?
Is it Ok to return whatever exit_code we already have or we better 
return some special error code?


Probably, it is Ok to keep the comment at the line 1315 as it is.


Thanks,
Serguei

On 8/26/14 7:47 AM, Dmitry Samersoff wrote:

Staffan,

http://cr.openjdk.java.net/~dsamersoff/JDK-8049226/webrev.03/

After couple of experiments I end up with simple fix - don't coredump on
AGENT_ERROR_TRANSPORT_INIT

Current code don't propagate transport error to upper level, so if we
need fine grained control I'll rewrite transport initialization.

-Dmitry


On 2014-08-25 15:53, Staffan Larsen wrote:

On 25 aug 2014, at 13:33, Dmitry Samersoff mailto:dmitry.samers...@oracle.com>> wrote:


Staffan,

On 2014-08-25 15:26, Staffan Larsen wrote:

Dmitry,

One problem with this fix is that debugInit_exit() is used from many
places in the JDWP code. For some of these I think it still does make
sense to receive a core dump for analysis. I agree that when parsing
options, we do not need a core dump.

jdwp has explicit option to request a coredump at debugInit_exit().

Is it enough or I should create a more sophisticated fix ?

I don’t think that is enough. Often a problem will happen and will not
be reproducible.

Maybe this code:

 if ((arg.error != JDWP_ERROR(NONE)) &&
 (arg.startCount == 0) &&
 initOnStartup) {
 EXIT_ERROR(map2jvmtiError(arg.error), "No transports initialized");
 }

can use some variant of EXIT_ERROR that does not call fatalError() ?

/Staffan


Actually, I don't think that coredump on every call to jni_FatalError()
(see jni.cpp:726) is necessary but this hotspot code is here for 6 years
and changing it probably out of scope of this fix.

-Dmitry



/Staffan


On 20 aug 2014, at 17:55, Dmitry Samersoff
mailto:dmitry.samers...@oracle.com>> wrote:


Serguei,

After some additional testing I changed the fix a bit. Please take
a look at new version.

http://cr.openjdk.java.net/~dsamersoff/JDK-8049226/webrev.02/

New version reports JVMTI error to stderr.

Also jniFatalError is not referenced any more so I remove it.

-Dmitry



On 2014-08-20 12:08, serguei.spit...@oracle.com wrote:

Ok.

Thank you for the explanation! Serguei

On 8/20/14 1:01 AM, Dmitry Samersoff wrote:

Serguei,

1. Historically JDI test-suite had no tests for failed
transport initialization behavior and invalid parameters
handling.

2. As a part of JDWP hardening work I added couple of such
tests to OptionTest.java - these tests pass invalid parameters
to dt_socket transport to make sure that transport doesn't
crash (one such crash was discovered and fixed) but just return
non-zero exit code to upper level.

3. After fix for JDK-6694099 any non-zero exit code from
transport cause VM to coredump. Dumping multiple cores on busy
machine takes a time so harness kills the test by timeout.

We can just increase timeout for this test but I don't think
it's a good idea to dump core when invalid parameters passed to
transport.

So there is the fix.

4. After the fix tests for negative parameters will return
non-zero exit code as expected but will not dump the core.

-Dmitry

On 2014-08-20 00:54, serguei.spit...@oracle.com wrote:

Hi Dmitry,

The fix seems to be Ok. Just want to make it clear... This
fix just changes the bug pattern. It a case of incorrect
transport parameters the test is still going to fail but
without crash, right?

Thanks, Serguei

On 8/19/14 12:09 PM, Dmitry Samersoff wrote:

Hi Everybody,

Please review the fix:

http://cr.openjdk.java.net/~dsamersoff/JDK-8049226/webrev.01/




JDWP call jniFatalError if transport can't be initialized (e.g. wrong

parameters) and jniFatalError call os::abort().  Therefor
all transport initialization errors cause vm to coredump.

I see no reason for debugInit_exit to call jniFatalError so
remove this code.

-Dmitry



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

Re: RFR(S): JDK-8054194 jstack crash: assert(handle != NULL) failed: JNI handle should not be null

[serguei.spit...@oracle.com]

On 8/26/14 8:25 AM, Dmitry Samersoff wrote:

Serguei,

On 2014-08-26 14:58, serguei.spit...@oracle.com wrote:

Dmitry,

I doubt, this webrev fixes the issue.
At least, I can't see it yet.

It looks like the assert is because the imagePath_ID or symbolPath_ID
were not initialized properly.

If imagePath_ID or symbolPath_ID is NULL we get a crash, not assert.

Assert happens if (and only if) path is NULL on call to
env->GetStringUTFChars(path, &isCopy); at ll. 316, 321

I verified it manually by setting these variables to different values.


Nice.
But how do we get path == null from one of the statements below ?
Is it because of the clazz value?

  path = (jstring) env->GetStaticObjectField(clazz, imagePath_ID);
  buf = env->GetStringUTFChars(path, &isCopy); < ???
  CHECK_EXCEPTION_(false);
  AutoJavaString imagePath(env, path, buf);

  path = (jstring) env->GetStaticObjectField(clazz, symbolPath_ID);
  buf = env->GetStringUTFChars(path, &isCopy); < ???
  CHECK_EXCEPTION_(false);




But there is no clear explanation yet how this could happen.
Maybe, the conclusion above was wrong, but some prove is needed and/or
another possible root cause.

Also, Staffan is right, about killing the remote process.
The remote process has no relation to the fix.

I can reproduce the problem (intermittently, once in couple of hours) by
two scripts running in two separate windows - first script runs and
kills Java2D demo second one runs jps and jstack -F against Java2D demo.

After the fix, problem is not reproducible anymore.

But I'll put more efforts to understand what is going wrong with jstack
process.


Good.

Thanks,
Serguei



-Dmitry


Thanks,
Serguei


On 8/25/14 3:58 AM, Staffan Larsen wrote:

Dmitry,

Your changes look good (except missing spaces after commas). But what I do not 
understand is how this relates to the bug. The code in setImageAndSymbolPath() 
is not looking at the remote process, it’s just setting up data in the jstack 
process. How does killing the remote process affect this code? What am I 
missing?

/Staffan

On 21 aug 2014, at 14:56, Dmitry Samersoff  wrote:


Hi Everyone,

Please review small agent changes:

http://cr.openjdk.java.net/~dsamersoff/JDK-8054194/webrev.01/

Under windows, If jstack attempts to attach to java process that is
being killed by someone else, GetStaticObjectField might return NULL.

-Dmitry

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

Re: Review request: 8055230: Rename attach provider implementation class

[Mandy Chung]

On 8/26/2014 3:59 AM, Staffan Larsen wrote:

There are some differences in the AttachProvideImpl files. Most notably the 
windows version is very different. Linux, Bsd and Aix are the same. The Solaris 
version has a minor difference in the  “public String type()” implementation. 
The Linux, Bsd, Aix and Solaris versions could probably be unified.


I would leave this as it is and further unification can be done
as future cleanup.  Thanks for all the review.

Mandy



/Staffan

On 26 aug 2014, at 12:40, Daniel Fuchs  wrote:


Hi Mandy,

I'm seeing some small differences in the various VirtualMachineImpl.java
files, but if I'm not mistaken, all the new AttachProviderImpl.java look
all the same. I wonder if the patch could be further simplified by
moving AttachProviderImpl.java to jdk.attach/share/classes (unless
there's a reason to keep all the different copies)...

best regards,

-- daniel

On 8/26/14 6:29 AM, Mandy Chung wrote:

Webrev:
http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8055230/

This patch renames the class name of attach provider implementation class
to be the same for all platforms.  This simplifies the build logic and
removes the need for generating the service config file at build time.

The files renamed are
unix/classes/sun/tools/attach/${OS}VirtualMachine.java
  -> ${OS}/classes/sun/tools/attach/VirtualMachineImpl.java
unix/classes/sun/tools/attach/${OS}AttachProvider.java
  -> ${OS}/classes/sun/tools/attach/VirtualMachineImpl.java

${OS}/classes/sun/tools/attach/${OS}VirtualMachine.java
  -> ${OS}/classes/sun/tools/attach/VirtualMachineImpl.java
${OS}/classes/sun/tools/attach/${OS}AttachProvider.java
  -> ${OS}/classes/sun/tools/attach/VirtualMachineImpl.java

and also corresponding native files.

Mandy

Re: RFR(S): JDK-8054194 jstack crash: assert(handle != NULL) failed: JNI handle should not be null

[Dmitry Samersoff]

Serguei,

On 2014-08-26 14:58, serguei.spit...@oracle.com wrote:
> Dmitry,
> 
> I doubt, this webrev fixes the issue.
> At least, I can't see it yet.
> 
> It looks like the assert is because the imagePath_ID or symbolPath_ID
> were not initialized properly.

If imagePath_ID or symbolPath_ID is NULL we get a crash, not assert.

Assert happens if (and only if) path is NULL on call to
env->GetStringUTFChars(path, &isCopy); at ll. 316, 321

I verified it manually by setting these variables to different values.

> But there is no clear explanation yet how this could happen.
> Maybe, the conclusion above was wrong, but some prove is needed and/or
> another possible root cause.
> 
> Also, Staffan is right, about killing the remote process.
> The remote process has no relation to the fix.

I can reproduce the problem (intermittently, once in couple of hours) by
two scripts running in two separate windows - first script runs and
kills Java2D demo second one runs jps and jstack -F against Java2D demo.

After the fix, problem is not reproducible anymore.

But I'll put more efforts to understand what is going wrong with jstack
process.

-Dmitry

> 
> Thanks,
> Serguei
> 
> 
> On 8/25/14 3:58 AM, Staffan Larsen wrote:
>> Dmitry,
>>
>> Your changes look good (except missing spaces after commas). But what I do 
>> not understand is how this relates to the bug. The code in 
>> setImageAndSymbolPath() is not looking at the remote process, it’s just 
>> setting up data in the jstack process. How does killing the remote process 
>> affect this code? What am I missing?
>>
>> /Staffan
>>
>> On 21 aug 2014, at 14:56, Dmitry Samersoff  
>> wrote:
>>
>>> Hi Everyone,
>>>
>>> Please review small agent changes:
>>>
>>> http://cr.openjdk.java.net/~dsamersoff/JDK-8054194/webrev.01/
>>>
>>> Under windows, If jstack attempts to attach to java process that is
>>> being killed by someone else, GetStaticObjectField might return NULL.
>>>
>>> -Dmitry
>>>
>>> -- 
>>> 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.

Re: RFR(S): URG! JDK-8049226 com/sun/jdi/OptionTest.java test times out again

[Dmitry Samersoff]

Staffan,

http://cr.openjdk.java.net/~dsamersoff/JDK-8049226/webrev.03/

After couple of experiments I end up with simple fix - don't coredump on
AGENT_ERROR_TRANSPORT_INIT

Current code don't propagate transport error to upper level, so if we
need fine grained control I'll rewrite transport initialization.

-Dmitry


On 2014-08-25 15:53, Staffan Larsen wrote:
> 
> On 25 aug 2014, at 13:33, Dmitry Samersoff  > wrote:
> 
>> Staffan,
>>
>> On 2014-08-25 15:26, Staffan Larsen wrote:
>>> Dmitry,
>>>
>>> One problem with this fix is that debugInit_exit() is used from many
>>> places in the JDWP code. For some of these I think it still does make
>>> sense to receive a core dump for analysis. I agree that when parsing
>>> options, we do not need a core dump.
>>
>> jdwp has explicit option to request a coredump at debugInit_exit().
>>
>> Is it enough or I should create a more sophisticated fix ?
> 
> I don’t think that is enough. Often a problem will happen and will not
> be reproducible. 
> 
> Maybe this code:
> 
> if ((arg.error != JDWP_ERROR(NONE)) &&
> (arg.startCount == 0) &&
> initOnStartup) {
> EXIT_ERROR(map2jvmtiError(arg.error), "No transports initialized");
> }
> 
> can use some variant of EXIT_ERROR that does not call fatalError() ?
> 
> /Staffan
> 
>>
>> Actually, I don't think that coredump on every call to jni_FatalError()
>> (see jni.cpp:726) is necessary but this hotspot code is here for 6 years
>> and changing it probably out of scope of this fix.
>>
>> -Dmitry
>>
>>
>>>
>>> /Staffan
>>>
>>>
>>> On 20 aug 2014, at 17:55, Dmitry Samersoff
>>> mailto:dmitry.samers...@oracle.com>> wrote:
>>>
 Serguei,

 After some additional testing I changed the fix a bit. Please take
 a look at new version.

 http://cr.openjdk.java.net/~dsamersoff/JDK-8049226/webrev.02/

 New version reports JVMTI error to stderr.

 Also jniFatalError is not referenced any more so I remove it.

 -Dmitry



 On 2014-08-20 12:08, serguei.spit...@oracle.com wrote:
> Ok.
>
> Thank you for the explanation! Serguei
>
> On 8/20/14 1:01 AM, Dmitry Samersoff wrote:
>> Serguei,
>>
>> 1. Historically JDI test-suite had no tests for failed
>> transport initialization behavior and invalid parameters
>> handling.
>>
>> 2. As a part of JDWP hardening work I added couple of such
>> tests to OptionTest.java - these tests pass invalid parameters
>> to dt_socket transport to make sure that transport doesn't
>> crash (one such crash was discovered and fixed) but just return
>> non-zero exit code to upper level.
>>
>> 3. After fix for JDK-6694099 any non-zero exit code from
>> transport cause VM to coredump. Dumping multiple cores on busy
>> machine takes a time so harness kills the test by timeout.
>>
>> We can just increase timeout for this test but I don't think
>> it's a good idea to dump core when invalid parameters passed to
>> transport.
>>
>> So there is the fix.
>>
>> 4. After the fix tests for negative parameters will return
>> non-zero exit code as expected but will not dump the core.
>>
>> -Dmitry
>>
>> On 2014-08-20 00:54, serguei.spit...@oracle.com wrote:
>>> Hi Dmitry,
>>>
>>> The fix seems to be Ok. Just want to make it clear... This
>>> fix just changes the bug pattern. It a case of incorrect
>>> transport parameters the test is still going to fail but
>>> without crash, right?
>>>
>>> Thanks, Serguei
>>>
>>> On 8/19/14 12:09 PM, Dmitry Samersoff wrote:
 Hi Everybody,

 Please review the fix:

 http://cr.openjdk.java.net/~dsamersoff/JDK-8049226/webrev.01/



>> JDWP call jniFatalError if transport can't be initialized (e.g. wrong
 parameters) and jniFatalError call os::abort().  Therefor
 all transport initialization errors cause vm to coredump.

 I see no reason for debugInit_exit to call jniFatalError so
 remove this code.

 -Dmitry

>>
>


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


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

Re: RFR: JDK-8055845 - Add trace event for promoted objects

[Erik Gahlin]

Looks good

Bengt Rutisson skrev 2014-08-26 10:50:


Hi all,

Staffan sent me an updated webrev based on Erik's comments. I uploaded 
it here:

http://cr.openjdk.java.net/~brutisso/8055845/webrev.01/

Thanks,
Bengt


On 2014-08-25 19:32, Staffan Friberg wrote:

Hi Erik,

No issue with naming the field class, since the event is similar to 
the Allocation event I used that as a starting point and it also uses 
class as name together with a couple of other events.


Will go through the descriptions and remove periods.

Object age is basically the how many times an object has been kept in 
survivor region, it is incremented each time a YC happens and the 
object is aged.
Will see how I can update the description to make it clearer. Calling 
the field tenuringAge might help?


From the documentation, 
http://docs.oracle.com/javase/8/docs/technotes/tools/unix/java.html


-XX:MaxTenuringThreshold=/threshold/

Sets the maximum tenuring threshold for use in adaptive GC
sizing. The largest value is 15. The default value is 15 for the
parallel (throughput) collector, and 6 for the CMS collector.

The following example shows how to set the maximum tenuring
threshold to 10:

-XX:MaxTenuringThreshold=10


-XX:+PrintTenuringDistribution

Enables printing of tenuring age information. The following is
an example of the output:

Desired survivor size 48286924 bytes, new threshold 10 (max 10)
- age 1: 28992024 bytes, 28992024 total
- age 2: 1366864 bytes, 3035 total
- age 3: 1425912 bytes, 31784800 total
...

Age 1 objects are the youngest survivors (they were created
after the previous scavenge, survived the latest scavenge, and
moved from eden to survivor space). Age 2 objects have survived
two scavenges (during the second scavenge they were copied from
one survivor space to the next). And so on.

In the preceding example, 28 992 024 bytes survived one scavenge
and were copied from eden to survivor space, 1 366 864 bytes are
occupied by age 2 objects, etc. The third value in each row is
the cumulative size of objects of age n or less.

By default, this option is disabled.



Thanks for the comments,
Staffan

On 08/25/2014 09:55 AM, Erik Gahlin wrote:
Did you manage to call the field "class"? It's a reserved word in 
C++, so we had to use "klass" in the past


Descriptions with only one sentence shouldn't end with "."

How is "Object Age" measured?

As a user, I would expect the number to be in seconds/minutes/hours, 
but that is not the case. Perhaps an explanation in the description 
and/or a field name that better reflects what we really mean with age.


Otherwise trace.xml looks good!

Erik

Staffan Friberg skrev 25/08/14 18:28:

Hi,

Could I please get reviews for this RFE that adds a trace event for 
aging and promotion for young collections in G1, CMS and Parallel GC.
It works similarly to allocation events, and generates the event on 
the slow path when either a direct copy occurs or a new PLAB is 
request.


Since I'm adding an event I cc:ed the serviceability list in case 
anyone has any comments on the event and changes to trace.xml.


RFE: https://bugs.openjdk.java.net/browse/JDK-8055845
Webrev: http://cr.openjdk.java.net/~brutisso/8055845/webrev.00

Bengt has been kind and agreed to both sponsor and host the the 
webrev.


Thanks,
Staffan

Re: Review request: 8049303: Transient network problems cause JMX thread to fail silenty

[Jaroslav Bachorik]

On 08/26/2014 01:20 PM, Poonam Bajaj wrote:

Hi Jaroslav,

On 8/26/2014 4:06 PM, Jaroslav Bachorik wrote:

Hi Poonam,

On 08/26/2014 12:27 PM, Poonam Bajaj wrote:

Sending the review request to serviceability-dev list as well...
---

Could I have reviews for this change:

Bug: https://bugs.openjdk.java.net/browse/JDK-8049303
Webrev: http://cr.openjdk.java.net/~poonam/8049303/webrev.00/


L1499-1504 can be completely removed. They serve no purpose now.


Removed this piece of code.


Please, adjust the indentation to fit the original one.


Corrected the indentation.

Updated webrev: http://cr.openjdk.java.net/~poonam/8049303/webrev.01/


Looks good!

-JB-



Thanks,
Poonam



Thanks,

-JB-



Problem and fix:
By default the JMX client side notification fetch timeout
(jmx.remote.x.notification.fetch.timeout) is 1 minute and the default
server connection timeout (jmx.remote.x.server.connection.timeout) is 2
minutes.

If the client side connector thread makes a notification fetch request
to the server, but a transient network problem prevents the server
response from reaching the client, the client side connector will wait
for a response until the timeout period (1 minute) has expired before
throwing an IOException.

The client side RMIConnector implementation handles the IOException, by
re-checking the connection status to understand whether or not it is
broken. If the connection is not available at that moment, the connector
fails by re-throwing the initial IOException. The problem is that this
re-check of the connection passes because the server side of the
connection doesn't time out until 2 minutes has passed (by default), so
the NotifFetcher thread
dies without posting a failed notification, and the client application
does not get a chance to recover.

The fix is to forward the exception on the JMX client side before
checking the connection status.

Testing:
All the jdk_jmx and jdk_management regression tests passed.

The fix applies cleanly to 8u and 7u repos.


Thanks,
Poonam

Re: RFR(S): JDK-8031583: warnings from b03 for hotspot/agent/src/os/solaris/proc: JNI exception pending

[serguei.spit...@oracle.com]

Looks good.

Thanks,
Serguei

On 8/11/14 7:57 AM, Dmitry Samersoff wrote:

Hi Everybody,

Please, review small fix that adds more exception handling code

http://cr.openjdk.java.net/~dsamersoff/JDK-8031583/webrev.01/

-Dmitry

Re: RFR(S): JDK-8054194 jstack crash: assert(handle != NULL) failed: JNI handle should not be null

[serguei.spit...@oracle.com]

On 8/26/14 3:58 AM, serguei.spit...@oracle.com wrote:

Dmitry,

I doubt, this webrev fixes the issue.
At least, I can't see it yet.

It looks like the assert is because the imagePath_ID or symbolPath_ID 
were not initialized properly.

But there is clear explanation yet how this could happen.

Sorry, a typo above. Wanted to say:
"But there is no clear explanation yet how this could happen."

Thanks,
Serguei

Maybe, the conclusion above was wrong, but some prove is needed and/or 
another possible root cause.


Also, Staffan is right, about killing the remote process.
The remote process has no relation to the fix.

Thanks,
Serguei


On 8/25/14 3:58 AM, Staffan Larsen wrote:

Dmitry,

Your changes look good (except missing spaces after commas). But what I do not 
understand is how this relates to the bug. The code in setImageAndSymbolPath() 
is not looking at the remote process, it’s just setting up data in the jstack 
process. How does killing the remote process affect this code? What am I 
missing?

/Staffan

On 21 aug 2014, at 14:56, Dmitry Samersoff  wrote:


Hi Everyone,

Please review small agent changes:

http://cr.openjdk.java.net/~dsamersoff/JDK-8054194/webrev.01/

Under windows, If jstack attempts to attach to java process that is
being killed by someone else, GetStaticObjectField might return NULL.

-Dmitry

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

Re: Review request: 8055230: Rename attach provider implementation class

[Daniel Fuchs]

On 8/26/14 12:59 PM, Staffan Larsen wrote:

There are some differences in the AttachProvideImpl files. Most notably the 
windows version is very different. Linux, Bsd and Aix are the same. The Solaris 
version has a minor difference in the  “public String type()” implementation. 
The Linux, Bsd, Aix and Solaris versions could probably be unified.


Ah - right - I missed those. Forget my comment then!

Thanks Staffan!

-- daniel



/Staffan

On 26 aug 2014, at 12:40, Daniel Fuchs  wrote:


Hi Mandy,

I'm seeing some small differences in the various VirtualMachineImpl.java
files, but if I'm not mistaken, all the new AttachProviderImpl.java look
all the same. I wonder if the patch could be further simplified by
moving AttachProviderImpl.java to jdk.attach/share/classes (unless
there's a reason to keep all the different copies)...

best regards,

-- daniel

On 8/26/14 6:29 AM, Mandy Chung wrote:

Webrev:
http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8055230/

This patch renames the class name of attach provider implementation class
to be the same for all platforms.  This simplifies the build logic and
removes the need for generating the service config file at build time.

The files renamed are
unix/classes/sun/tools/attach/${OS}VirtualMachine.java
  -> ${OS}/classes/sun/tools/attach/VirtualMachineImpl.java
unix/classes/sun/tools/attach/${OS}AttachProvider.java
  -> ${OS}/classes/sun/tools/attach/VirtualMachineImpl.java

${OS}/classes/sun/tools/attach/${OS}VirtualMachine.java
  -> ${OS}/classes/sun/tools/attach/VirtualMachineImpl.java
${OS}/classes/sun/tools/attach/${OS}AttachProvider.java
  -> ${OS}/classes/sun/tools/attach/VirtualMachineImpl.java

and also corresponding native files.

Mandy

Re: Review request: 8049303: Transient network problems cause JMX thread to fail silenty

[Poonam Bajaj]

Hi Jaroslav,

On 8/26/2014 4:06 PM, Jaroslav Bachorik wrote:

Hi Poonam,

On 08/26/2014 12:27 PM, Poonam Bajaj wrote:

Sending the review request to serviceability-dev list as well...
---

Could I have reviews for this change:

Bug: https://bugs.openjdk.java.net/browse/JDK-8049303
Webrev: http://cr.openjdk.java.net/~poonam/8049303/webrev.00/


L1499-1504 can be completely removed. They serve no purpose now.


Removed this piece of code.


Please, adjust the indentation to fit the original one.


Corrected the indentation.

Updated webrev: http://cr.openjdk.java.net/~poonam/8049303/webrev.01/

Thanks,
Poonam



Thanks,

-JB-



Problem and fix:
By default the JMX client side notification fetch timeout
(jmx.remote.x.notification.fetch.timeout) is 1 minute and the default
server connection timeout (jmx.remote.x.server.connection.timeout) is 2
minutes.

If the client side connector thread makes a notification fetch request
to the server, but a transient network problem prevents the server
response from reaching the client, the client side connector will wait
for a response until the timeout period (1 minute) has expired before
throwing an IOException.

The client side RMIConnector implementation handles the IOException, by
re-checking the connection status to understand whether or not it is
broken. If the connection is not available at that moment, the connector
fails by re-throwing the initial IOException. The problem is that this
re-check of the connection passes because the server side of the
connection doesn't time out until 2 minutes has passed (by default), so
the NotifFetcher thread
dies without posting a failed notification, and the client application
does not get a chance to recover.

The fix is to forward the exception on the JMX client side before
checking the connection status.

Testing:
All the jdk_jmx and jdk_management regression tests passed.

The fix applies cleanly to 8u and 7u repos.


Thanks,
Poonam

Re: Review request: 8055230: Rename attach provider implementation class

[Staffan Larsen]

There are some differences in the AttachProvideImpl files. Most notably the 
windows version is very different. Linux, Bsd and Aix are the same. The Solaris 
version has a minor difference in the  “public String type()” implementation. 
The Linux, Bsd, Aix and Solaris versions could probably be unified.

/Staffan

On 26 aug 2014, at 12:40, Daniel Fuchs  wrote:

> Hi Mandy,
> 
> I'm seeing some small differences in the various VirtualMachineImpl.java
> files, but if I'm not mistaken, all the new AttachProviderImpl.java look
> all the same. I wonder if the patch could be further simplified by
> moving AttachProviderImpl.java to jdk.attach/share/classes (unless
> there's a reason to keep all the different copies)...
> 
> best regards,
> 
> -- daniel
> 
> On 8/26/14 6:29 AM, Mandy Chung wrote:
>> Webrev:
>>http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8055230/
>> 
>> This patch renames the class name of attach provider implementation class
>> to be the same for all platforms.  This simplifies the build logic and
>> removes the need for generating the service config file at build time.
>> 
>> The files renamed are
>>unix/classes/sun/tools/attach/${OS}VirtualMachine.java
>>  -> ${OS}/classes/sun/tools/attach/VirtualMachineImpl.java
>>unix/classes/sun/tools/attach/${OS}AttachProvider.java
>>  -> ${OS}/classes/sun/tools/attach/VirtualMachineImpl.java
>> 
>>${OS}/classes/sun/tools/attach/${OS}VirtualMachine.java
>>  -> ${OS}/classes/sun/tools/attach/VirtualMachineImpl.java
>>${OS}/classes/sun/tools/attach/${OS}AttachProvider.java
>>  -> ${OS}/classes/sun/tools/attach/VirtualMachineImpl.java
>> 
>> and also corresponding native files.
>> 
>> Mandy
>> 
>> 
> 

Re: RFR(S): JDK-8054194 jstack crash: assert(handle != NULL) failed: JNI handle should not be null

[serguei.spit...@oracle.com]

Dmitry,

I doubt, this webrev fixes the issue.
At least, I can't see it yet.

It looks like the assert is because the imagePath_ID or symbolPath_ID 
were not initialized properly.

But there is clear explanation yet how this could happen.
Maybe, the conclusion above was wrong, but some prove is needed and/or 
another possible root cause.


Also, Staffan is right, about killing the remote process.
The remote process has no relation to the fix.

Thanks,
Serguei


On 8/25/14 3:58 AM, Staffan Larsen wrote:

Dmitry,

Your changes look good (except missing spaces after commas). But what I do not 
understand is how this relates to the bug. The code in setImageAndSymbolPath() 
is not looking at the remote process, it’s just setting up data in the jstack 
process. How does killing the remote process affect this code? What am I 
missing?

/Staffan

On 21 aug 2014, at 14:56, Dmitry Samersoff  wrote:


Hi Everyone,

Please review small agent changes:

http://cr.openjdk.java.net/~dsamersoff/JDK-8054194/webrev.01/

Under windows, If jstack attempts to attach to java process that is
being killed by someone else, GetStaticObjectField might return NULL.

-Dmitry

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

Re: Review request: 8055230: Rename attach provider implementation class

[Daniel Fuchs]

Hi Mandy,

I'm seeing some small differences in the various VirtualMachineImpl.java
files, but if I'm not mistaken, all the new AttachProviderImpl.java look
all the same. I wonder if the patch could be further simplified by
moving AttachProviderImpl.java to jdk.attach/share/classes (unless
there's a reason to keep all the different copies)...

best regards,

-- daniel

On 8/26/14 6:29 AM, Mandy Chung wrote:

Webrev:
http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8055230/

This patch renames the class name of attach provider implementation class
to be the same for all platforms.  This simplifies the build logic and
removes the need for generating the service config file at build time.

The files renamed are
unix/classes/sun/tools/attach/${OS}VirtualMachine.java
  -> ${OS}/classes/sun/tools/attach/VirtualMachineImpl.java
unix/classes/sun/tools/attach/${OS}AttachProvider.java
  -> ${OS}/classes/sun/tools/attach/VirtualMachineImpl.java

${OS}/classes/sun/tools/attach/${OS}VirtualMachine.java
  -> ${OS}/classes/sun/tools/attach/VirtualMachineImpl.java
${OS}/classes/sun/tools/attach/${OS}AttachProvider.java
  -> ${OS}/classes/sun/tools/attach/VirtualMachineImpl.java

and also corresponding native files.

Mandy

Re: Review request: 8049303: Transient network problems cause JMX thread to fail silenty

[Jaroslav Bachorik]

Hi Poonam,

On 08/26/2014 12:27 PM, Poonam Bajaj wrote:

Sending the review request to serviceability-dev list as well...
---

Could I have reviews for this change:

Bug: https://bugs.openjdk.java.net/browse/JDK-8049303
Webrev: http://cr.openjdk.java.net/~poonam/8049303/webrev.00/


L1499-1504 can be completely removed. They serve no purpose now.

Please, adjust the indentation to fit the original one.

Thanks,

-JB-



Problem and fix:
By default the JMX client side notification fetch timeout
(jmx.remote.x.notification.fetch.timeout) is 1 minute and the default
server connection timeout (jmx.remote.x.server.connection.timeout) is 2
minutes.

If the client side connector thread makes a notification fetch request
to the server, but a transient network problem prevents the server
response from reaching the client, the client side connector will wait
for a response until the timeout period (1 minute) has expired before
throwing an IOException.

The client side RMIConnector implementation handles the IOException, by
re-checking the connection status to understand whether or not it is
broken. If the connection is not available at that moment, the connector
fails by re-throwing the initial IOException. The problem is that this
re-check of the connection passes because the server side of the
connection doesn't time out until 2 minutes has passed (by default), so
the NotifFetcher thread
dies without posting a failed notification, and the client application
does not get a chance to recover.

The fix is to forward the exception on the JMX client side before
checking the connection status.

Testing:
All the jdk_jmx and jdk_management regression tests passed.

The fix applies cleanly to 8u and 7u repos.


Thanks,
Poonam

Review request: 8049303: Transient network problems cause JMX thread to fail silenty

[Poonam Bajaj]

Sending the review request to serviceability-dev list as well...
---

Could I have reviews for this change:

Bug: https://bugs.openjdk.java.net/browse/JDK-8049303
Webrev: http://cr.openjdk.java.net/~poonam/8049303/webrev.00/

Problem and fix:
By default the JMX client side notification fetch timeout 
(jmx.remote.x.notification.fetch.timeout) is 1 minute and the default 
server connection timeout (jmx.remote.x.server.connection.timeout) is 2 
minutes.


If the client side connector thread makes a notification fetch request 
to the server, but a transient network problem prevents the server 
response from reaching the client, the client side connector will wait 
for a response until the timeout period (1 minute) has expired before 
throwing an IOException.


The client side RMIConnector implementation handles the IOException, by 
re-checking the connection status to understand whether or not it is 
broken. If the connection is not available at that moment, the connector 
fails by re-throwing the initial IOException. The problem is that this 
re-check of the connection passes because the server side of the 
connection doesn't time out until 2 minutes has passed (by default), so 
the NotifFetcher thread
dies without posting a failed notification, and the client application 
does not get a chance to recover.


The fix is to forward the exception on the JMX client side before 
checking the connection status.


Testing:
All the jdk_jmx and jdk_management regression tests passed.

The fix applies cleanly to 8u and 7u repos.


Thanks,
Poonam

Re: RFR: JDK-8055845 - Add trace event for promoted objects

[Bengt Rutisson]

Hi all,

Staffan sent me an updated webrev based on Erik's comments. I uploaded 
it here:

http://cr.openjdk.java.net/~brutisso/8055845/webrev.01/

Thanks,
Bengt


On 2014-08-25 19:32, Staffan Friberg wrote:

Hi Erik,

No issue with naming the field class, since the event is similar to 
the Allocation event I used that as a starting point and it also uses 
class as name together with a couple of other events.


Will go through the descriptions and remove periods.

Object age is basically the how many times an object has been kept in 
survivor region, it is incremented each time a YC happens and the 
object is aged.
Will see how I can update the description to make it clearer. Calling 
the field tenuringAge might help?


From the documentation, 
http://docs.oracle.com/javase/8/docs/technotes/tools/unix/java.html


-XX:MaxTenuringThreshold=/threshold/

Sets the maximum tenuring threshold for use in adaptive GC
sizing. The largest value is 15. The default value is 15 for the
parallel (throughput) collector, and 6 for the CMS collector.

The following example shows how to set the maximum tenuring
threshold to 10:

-XX:MaxTenuringThreshold=10


-XX:+PrintTenuringDistribution

Enables printing of tenuring age information. The following is an
example of the output:

Desired survivor size 48286924 bytes, new threshold 10 (max 10)
- age 1: 28992024 bytes, 28992024 total
- age 2: 1366864 bytes, 3035 total
- age 3: 1425912 bytes, 31784800 total
...

Age 1 objects are the youngest survivors (they were created after
the previous scavenge, survived the latest scavenge, and moved
from eden to survivor space). Age 2 objects have survived two
scavenges (during the second scavenge they were copied from one
survivor space to the next). And so on.

In the preceding example, 28 992 024 bytes survived one scavenge
and were copied from eden to survivor space, 1 366 864 bytes are
occupied by age 2 objects, etc. The third value in each row is
the cumulative size of objects of age n or less.

By default, this option is disabled.



Thanks for the comments,
Staffan

On 08/25/2014 09:55 AM, Erik Gahlin wrote:
Did you manage to call the field "class"? It's a reserved word in 
C++, so we had to use "klass" in the past


Descriptions with only one sentence shouldn't end with "."

How is "Object Age" measured?

As a user, I would expect the number to be in seconds/minutes/hours, 
but that is not the case. Perhaps an explanation in the description 
and/or a field name that better reflects what we really mean with age.


Otherwise trace.xml looks good!

Erik

Staffan Friberg skrev 25/08/14 18:28:

Hi,

Could I please get reviews for this RFE that adds a trace event for 
aging and promotion for young collections in G1, CMS and Parallel GC.
It works similarly to allocation events, and generates the event on 
the slow path when either a direct copy occurs or a new PLAB is 
request.


Since I'm adding an event I cc:ed the serviceability list in case 
anyone has any comments on the event and changes to trace.xml.


RFE: https://bugs.openjdk.java.net/browse/JDK-8055845
Webrev: http://cr.openjdk.java.net/~brutisso/8055845/webrev.00

Bengt has been kind and agreed to both sponsor and host the the webrev.

Thanks,
Staffan

Re: RFR 8037082: java/lang/instrument/NativeMethodPrefixAgent.java failing

[Staffan Larsen]

Looks good!

Thanks,
/Staffan

On 26 aug 2014, at 10:15, Jaroslav Bachorik  
wrote:

> On 08/25/2014 08:37 PM, Staffan Larsen wrote:
>> Thanks for taking on this one!
>> 
>> The code looks good. Just two small things below.
>> 
>> Have you tested with -Xverify:all, just to see if there are any byte code 
>> problems?
> 
> I've re-run the tests with -Xverify:all and fixed one problem in the 
> generated bytecode. Thanks for reminding me.
> 
>> 
>> 
>> Could fix the auto-naming of the params in this code?
>>  131 @Override
>>  132 public void visit(int i, int i1, String className, String 
>> string1, String string2, String[] strings) {
>>  133 this.className = className;
>>  134 super.visit(i, i1, className, string1, string2, 
>> strings);
>>  135 }
> 
> Done ...
> 
>> 
>> nit: let’s ClassWriter to deal -> let ClassWriter deal
>> 163 mv.visitMaxs(1, 1); // dummy call; let's 
>> ClassWriter to deal with this
> 
> ... and done.
> 
> http://cr.openjdk.java.net/~jbachorik/8037082/webrev.01
> 
> Cheers,
> 
> -JB-
> 
>> 
>> 
>> Thanks,
>> /Staffan
>> 
>> 
>> On 25 aug 2014, at 19:16, Jaroslav Bachorik  
>> wrote:
>> 
>>> Please, review the following test fix.
>>> 
>>> Issue : https://bugs.openjdk.java.net/browse/JDK-8037082
>>> Webrev: http://cr.openjdk.java.net/~jbachorik/8037082/webrev.00
>>> 
>>> As Staffan mentions in the issue comments - "The two tests 
>>> NativeMethodPrefixAgent and RetransformAgent use their own byte code 
>>> instrumentation library in jdk/test/java/lang/instrument/ilib/. These tests 
>>> need to be rewritten to use ASM instead so that we don't have to maintain 
>>> the ilib library."
>>> 
>>> This patch is intended to remove the "ilib" library and replace the usages 
>>> with an ASM5 alternative. Only the currently used features of the "ilib" 
>>> library are being ported.
>>> 
>>> Thanks,
>>> 
>>> -JB-
>> 
> 

Re: Review request: 8055230: Rename attach provider implementation class

[Erik Joelsson]

Nice!

/Erik

On 2014-08-26 06:29, Mandy Chung wrote:

Webrev:
   http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8055230/

This patch renames the class name of attach provider implementation class
to be the same for all platforms.  This simplifies the build logic and
removes the need for generating the service config file at build time.

The files renamed are
   unix/classes/sun/tools/attach/${OS}VirtualMachine.java
 -> ${OS}/classes/sun/tools/attach/VirtualMachineImpl.java
   unix/classes/sun/tools/attach/${OS}AttachProvider.java
 -> ${OS}/classes/sun/tools/attach/VirtualMachineImpl.java

   ${OS}/classes/sun/tools/attach/${OS}VirtualMachine.java
 -> ${OS}/classes/sun/tools/attach/VirtualMachineImpl.java
   ${OS}/classes/sun/tools/attach/${OS}AttachProvider.java
 -> ${OS}/classes/sun/tools/attach/VirtualMachineImpl.java

and also corresponding native files.

Mandy

Re: RFR 8037082: java/lang/instrument/NativeMethodPrefixAgent.java failing

[Jaroslav Bachorik]

On 08/25/2014 08:37 PM, Staffan Larsen wrote:

Thanks for taking on this one!

The code looks good. Just two small things below.

Have you tested with -Xverify:all, just to see if there are any byte code 
problems?


I've re-run the tests with -Xverify:all and fixed one problem in the 
generated bytecode. Thanks for reminding me.





Could fix the auto-naming of the params in this code?
  131 @Override
  132 public void visit(int i, int i1, String className, String 
string1, String string2, String[] strings) {
  133 this.className = className;
  134 super.visit(i, i1, className, string1, string2, strings);
  135 }


Done ...



nit: let’s ClassWriter to deal -> let ClassWriter deal
163 mv.visitMaxs(1, 1); // dummy call; let's 
ClassWriter to deal with this


... and done.

http://cr.openjdk.java.net/~jbachorik/8037082/webrev.01

Cheers,

-JB-




Thanks,
/Staffan


On 25 aug 2014, at 19:16, Jaroslav Bachorik  
wrote:


Please, review the following test fix.

Issue : https://bugs.openjdk.java.net/browse/JDK-8037082
Webrev: http://cr.openjdk.java.net/~jbachorik/8037082/webrev.00

As Staffan mentions in the issue comments - "The two tests NativeMethodPrefixAgent 
and RetransformAgent use their own byte code instrumentation library in 
jdk/test/java/lang/instrument/ilib/. These tests need to be rewritten to use ASM instead 
so that we don't have to maintain the ilib library."

This patch is intended to remove the "ilib" library and replace the usages with an ASM5 
alternative. Only the currently used features of the "ilib" library are being ported.

Thanks,

-JB-

Re: Review request: 8055230: Rename attach provider implementation class

[Chris Hegarty]

On 26 Aug 2014, at 08:26, Alan Bateman  wrote:

> On 26/08/2014 05:29, Mandy Chung wrote:
>> Webrev:
>>   http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8055230/
>> 
>> This patch renames the class name of attach provider implementation class
>> to be the same for all platforms.  This simplifies the build logic and
>> removes the need for generating the service config file at build time.
> This looks good, nice to see the benefits of the new layout.

+1.  Wow this is nice, and the knock on simplification in the build logic is a 
bonus.

-Chris.

> -Alan.

Re: Review request: 8055230: Rename attach provider implementation class

[Alan Bateman]

On 26/08/2014 05:29, Mandy Chung wrote:

Webrev:
   http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8055230/

This patch renames the class name of attach provider implementation class
to be the same for all platforms.  This simplifies the build logic and
removes the need for generating the service config file at build time.

This looks good, nice to see the benefits of the new layout.

-Alan.