Re: RFR: 8168479: Backout the changes for JDK-8168478

2016-10-21 Thread Dmitry Dmitriev 
Hi Alexander,

can you please change bug title to "Quarantine AllModulesCommandTest.java 
test". The change looks good.

Dmitry

- Original Message -
From: alexander.kulyakh...@oracle.com
To: serviceability-dev@openjdk.java.net
Sent: Friday, October 21, 2016 4:10:41 PM GMT +03:00 Iraq
Subject: RFR: 8168479: Backout the changes for  JDK-8168478

Hi,

Could you, please, review this trivial change (quarantining a test).

CR: https://bugs.openjdk.java.net/browse/JDK-8168479 "Backout the changes for 
JDK-8168478"
Webrev: http://cr.openjdk.java.net/~akulyakh/8168479/

The test is being quarantining due to 
https://bugs.openjdk.java.net/browse/JDK-8168478 "JDWP: canRead() reports false 
for reading from the same module 'java.base'"
(same as the previously unreproducible 
https://bugs.openjdk.java.net/browse/JDK-8166289)

On the SQE side we do not currently see any issues with the test itself.

Best regards,
Alexander

Re: RFR: 8158797: Test java/lang/management/MemoryMXBean/LowMemoryTest.java fails when GC is specified explicitly

2016-10-12 Thread Dmitry Dmitriev 

Hi Alexander,

Looks good. Please correct copyright year in the header(2015 -> 2016). 
Not need a new webrev for that.


Thanks,
Dmitry

On 10.10.2016 13:51, Alexander Kulyakhtin wrote:

Hi,

Could you, please, review this simple, test-only change:

CR: https://bugs.openjdk.java.net/browse/JDK-8158797 "Test 
java/lang/management/MemoryMXBean/LowMemoryTest.java fails when GC is specified 
explicitly"
Webrev: 
http://cr.openjdk.java.net/~akulyakh/8158797/test/java/lang/management/MemoryMXBean/LowMemoryTest.java.udiff.html

In order to verify some expected behavior, the test specifies its own GC 
options when running.
Currently, if the testing framework runs this test with some other explicitly specifed GC 
options, then the test fails with "Conflicting collector combinations in option 
list" message.
We are modifying the test so that it runs only when the framework does not 
specify any GC options.

Best regards,
Alexander

Re: RFR: 8150758: [TESTBUG] need jvmti tests for module aware agents

2016-09-20 Thread Dmitry Dmitriev 

Dmitry, Sergeui, thank you for the review!

For the record, webrev.02: 
http://cr.openjdk.java.net/~ddmitriev/8150758/webrev.02/ 
<http://cr.openjdk.java.net/%7Eddmitriev/8150758/webrev.02/>


Dmitry

On 20.09.2016 13:39, Dmitry Samersoff wrote:

Dmitry,

Looks good for me!

Minor formatting nits:

libMAAClassFileLoadHook.c

103: missed space after ,

libMAAClassFileLoadHook.c:
266: extra semicolon

-Dmitry

On 2016-09-20 13:17, Dmitry Dmitriev wrote:

Serguei, Dmitry,

Thank you for the feedback! Here is an updated webrev.01;
http://cr.openjdk.java.net/~ddmitriev/8150758/webrev.01/
<http://cr.openjdk.java.net/%7Eddmitriev/8150758/webrev.01/>

Dmitry

On 20.09.2016 12:48, Dmitry Samersoff wrote:

Dmitry,

Please, also change

!strcmp(...) to strcmp(...) == 0

because semantically strcmp result is not a boolean false.

-Dmitry

On 2016-09-20 06:38, serguei.spit...@oracle.com wrote:

Hi Dmitry,


Thanks a lot for this additional test coverage and discovering new bug:
https://bugs.openjdk.java.net/browse/JDK-8165681

The tests look pretty good to me.

A couple of minor comments.

Dots are missed in the .c files comments.


http://cr.openjdk.java.net/~ddmitriev/8150758/webrev.00/test/serviceability/jvmti




   265 printf("Expecting to find '%s' class in ClassLoad events
duringVM start phase.\n", EXPECTED_SIGNATURE);
   266 if (class_in_class_load_events_vm_start == JNI_FALSE) {
   267 throw_exc(env, "Unable to find expected class in
ClassLoad events duringstart phase!\n");
   268 return FAILED;
   269 }
   270
   271 if (class_prepare_events_vm_start_count == 0) {
   272 throw_exc(env, "Didn't get ClassPrepare events in
start
phase!\n");
   273 return FAILED;
   274 }
   275
   276 printf("Expecting to find '%s' class in ClassPrepare
events
duringVM start phase.\n", EXPECTED_SIGNATURE);
   277 if (class_in_class_prepare_events_vm_start == JNI_FALSE) {
   278 throw_exc(env, "Unable to find expected class in
ClassPrepare events duringstart phase!\n");
   279 return FAILED;
   280 }


 Could you, please, replace "start phase" with "early start phase"?
It will be more clear that the start phase mode is "early".

   288 if (class_in_class_prepare_events_vm_start == JNI_TRUE) {
   289 throw_exc(env, "Class is found in ClassLoad events
duringVM Start phase!\n");
   290 return FAILED;
   291 }
   292
   293 printf("Expecting that '%s' class is absent in
ClassPrepare
events.\n", EXPECTED_SIGNATURE);
   294 if (class_in_class_prepare_events_vm_start == JNI_TRUE) {
   295 throw_exc(env, "Class is found in ClassPrepare events
duringVM Start phase!\n");
   296 return FAILED;
   297 }

 Could you, please, replace "VM Start phase" with "normal VM start
phase"?It will be more clear that the start phase mode is "normal".
Thanks, Serguei On 9/19/16 05:47, Dmitry Dmitriev wrote:

Hello, Please review new tests for module aware agents. There are 3
tests: 1) MAAClassFileLoadHook.java - verifies ClassFileLoadHook event
with different combinations of can_generate_early_vmstart and
can_generate_early_class_hook_events JVMTI capabilities. Expects to
find(or not) class from java.base module in the right VM phase. 2)
MAAClassLoadPrepare.java - verifies ClassLoad and ClassPrepare events
with and without can_generate_early_vmstart JVMTI capability. Expects
to find(or not) class from java.base module in the VM start phase. 3)
MAAThreadStart.java - verifies ThreadStart event with
can_generate_early_vmstart JVMTI capability. Expect to find events in
the VM start phase. JBS:
https://bugs.openjdk.java.net/browse/JDK-8150758 webrev.00:
http://cr.openjdk.java.net/~ddmitriev/8150758/webrev.00/
<http://cr.openjdk.java.net/%7Eddmitriev/8150758/webrev.00/> Testing:
RBT on all platforms Thanks, Dmitry

Re: RFR: 8150758: [TESTBUG] need jvmti tests for module aware agents

2016-09-20 Thread Dmitry Dmitriev 

Serguei, Dmitry,

Thank you for the feedback! Here is an updated webrev.01; 
http://cr.openjdk.java.net/~ddmitriev/8150758/webrev.01/ 
<http://cr.openjdk.java.net/%7Eddmitriev/8150758/webrev.01/>


Dmitry

On 20.09.2016 12:48, Dmitry Samersoff wrote:

Dmitry,

Please, also change

!strcmp(...) to strcmp(...) == 0

because semantically strcmp result is not a boolean false.

-Dmitry

On 2016-09-20 06:38, serguei.spit...@oracle.com wrote:

Hi Dmitry,


Thanks a lot for this additional test coverage and discovering new bug:
   https://bugs.openjdk.java.net/browse/JDK-8165681

The tests look pretty good to me.

A couple of minor comments.

Dots are missed in the .c files comments.


http://cr.openjdk.java.net/~ddmitriev/8150758/webrev.00/test/serviceability/jvmti



  265 printf("Expecting to find '%s' class in ClassLoad events
duringVM start phase.\n", EXPECTED_SIGNATURE);
  266 if (class_in_class_load_events_vm_start == JNI_FALSE) {
  267 throw_exc(env, "Unable to find expected class in
ClassLoad events duringstart phase!\n");
  268 return FAILED;
  269 }
  270
  271 if (class_prepare_events_vm_start_count == 0) {
  272 throw_exc(env, "Didn't get ClassPrepare events in start
phase!\n");
  273 return FAILED;
  274 }
  275
  276 printf("Expecting to find '%s' class in ClassPrepare events
duringVM start phase.\n", EXPECTED_SIGNATURE);
  277 if (class_in_class_prepare_events_vm_start == JNI_FALSE) {
  278 throw_exc(env, "Unable to find expected class in
ClassPrepare events duringstart phase!\n");
  279 return FAILED;
  280 }


Could you, please, replace "start phase" with "early start phase"?
It will be more clear that the start phase mode is "early".

  288 if (class_in_class_prepare_events_vm_start == JNI_TRUE) {
  289 throw_exc(env, "Class is found in ClassLoad events
duringVM Start phase!\n");
  290 return FAILED;
  291 }
  292
  293 printf("Expecting that '%s' class is absent in ClassPrepare
events.\n", EXPECTED_SIGNATURE);
  294 if (class_in_class_prepare_events_vm_start == JNI_TRUE) {
  295 throw_exc(env, "Class is found in ClassPrepare events
duringVM Start phase!\n");
  296 return FAILED;
  297 }

Could you, please, replace "VM Start phase" with "normal VM start
phase"?It will be more clear that the start phase mode is "normal".
Thanks, Serguei On 9/19/16 05:47, Dmitry Dmitriev wrote:

Hello, Please review new tests for module aware agents. There are 3
tests: 1) MAAClassFileLoadHook.java - verifies ClassFileLoadHook event
with different combinations of can_generate_early_vmstart and
can_generate_early_class_hook_events JVMTI capabilities. Expects to
find(or not) class from java.base module in the right VM phase. 2)
MAAClassLoadPrepare.java - verifies ClassLoad and ClassPrepare events
with and without can_generate_early_vmstart JVMTI capability. Expects
to find(or not) class from java.base module in the VM start phase. 3)
MAAThreadStart.java - verifies ThreadStart event with
can_generate_early_vmstart JVMTI capability. Expect to find events in
the VM start phase. JBS:
https://bugs.openjdk.java.net/browse/JDK-8150758 webrev.00:
http://cr.openjdk.java.net/~ddmitriev/8150758/webrev.00/
<http://cr.openjdk.java.net/%7Eddmitriev/8150758/webrev.00/> Testing:
RBT on all platforms Thanks, Dmitry

RFR: 8150758: [TESTBUG] need jvmti tests for module aware agents

2016-09-19 Thread Dmitry Dmitriev 

Hello,

Please review new tests for module aware agents. There are 3 tests:
1) MAAClassFileLoadHook.java - verifies ClassFileLoadHook event with 
different combinations of can_generate_early_vmstart and 
can_generate_early_class_hook_events JVMTI capabilities. Expects to 
find(or not) class from java.base module in the right VM phase.
2) MAAClassLoadPrepare.java - verifies ClassLoad and ClassPrepare events 
with and without can_generate_early_vmstart JVMTI capability. Expects to 
find(or not) class from java.base module in the VM start phase.
3) MAAThreadStart.java - verifies ThreadStart event with 
can_generate_early_vmstart JVMTI capability. Expect to find events in 
the VM start phase.


JBS: https://bugs.openjdk.java.net/browse/JDK-8150758
webrev.00: http://cr.openjdk.java.net/~ddmitriev/8150758/webrev.00/ 


Testing: RBT on all platforms

Thanks,
Dmitry

Re: RFR(S): JDK-8165500: TestJpsJar shouldn't jar all test.classpath directories

2016-09-16 Thread Dmitry Dmitriev 
Dmitry, here is how I see this situation: When we ran 
sun/tools/jps/TestJpsJar.java alone in the clean folder, it builds 
testlibrary classes implicitly and put it under the same folder with 
test class, i.e. to JTwork/classes/sun/tools/jps folder and test works fine.
But sun/tools/jinfo/BasicJInfoTest.java builds testlibrary classes by 
"@build jdk.testlibrary.*" command and these classes are places in 
JTwork/classes/lib/testlibrary folder. After that 
sun/tools/jps/TestJpsJar.java skip building testlibrary classes, because 
these classes are already built and classpath contains path to these 
classes. Due to JDK-8165944 JpsHelper unable to add classes from second 
folder within classpath and test failed. And with your patch JpsHelper 
will add classes only from folder with test class.


Thanks,
Dmitry

On 15.09.2016 19:14, Dmitry Samersoff wrote:

Dmitry,

I'd reproduced it. I'll check what is happening.

-Dmitry


On 2016-09-15 15:34, Dmitry Dmitriev wrote:

Hi Dmitry,

I don't think that this solves the problem. If some test build
testlibrary before TestJpsJar.java, then testlibrary classes will be
outside the folder with JpsHelper class and thus missed in the jar file.

I can reproduce this problem with your patch applied:
1) Run sun/tools/jinfo/BasicJInfoTest.java in clean folder
2) Then run sun/tools/jps/TestJpsJar.java. TestJpsJar.java fails with
following error:
stderr: [Exception in thread "main" java.lang.NoClassDefFoundError:
jdk/testlibrary/ProcessTools
 at JpsBase.main(JpsBase.java:73)
Caused by: java.lang.ClassNotFoundException: jdk.testlibrary.ProcessTools
 at
jdk.internal.loader.BuiltinClassLoader.loadClass(java.base@9-internal/BuiltinClassLoader.java:366)

 at
jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(java.base@9-internal/ClassLoaders.java:185)

 at
java.lang.ClassLoader.loadClass(java.base@9-internal/ClassLoader.java:424)
 ... 1 more
]
  exitValue = 1

java.lang.RuntimeException: Expected to get exit value of [0]

Thanks,
Dmitry

On 15.09.2016 15:18, Dmitry Samersoff wrote:

Everybody,

Please, review the small fix.

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

The problem:

TestJpsJar attempts to copy all directories found in test.class.path
into a single jar file.

It's not necessary and could lead to intermittent ClassNotFound
exceptions.

Solution:

Jar only a directory with required files.

-Dmitry

Re: RFR(S): JDK-8165500: TestJpsJar shouldn't jar all test.classpath directories

2016-09-15 Thread Dmitry Dmitriev 

Hi Dmitry,

I don't think that this solves the problem. If some test build 
testlibrary before TestJpsJar.java, then testlibrary classes will be 
outside the folder with JpsHelper class and thus missed in the jar file.


I can reproduce this problem with your patch applied:
1) Run sun/tools/jinfo/BasicJInfoTest.java in clean folder
2) Then run sun/tools/jps/TestJpsJar.java. TestJpsJar.java fails with 
following error:
stderr: [Exception in thread "main" java.lang.NoClassDefFoundError: 
jdk/testlibrary/ProcessTools

at JpsBase.main(JpsBase.java:73)
Caused by: java.lang.ClassNotFoundException: jdk.testlibrary.ProcessTools
at 
jdk.internal.loader.BuiltinClassLoader.loadClass(java.base@9-internal/BuiltinClassLoader.java:366)
at 
jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(java.base@9-internal/ClassLoaders.java:185)
at 
java.lang.ClassLoader.loadClass(java.base@9-internal/ClassLoader.java:424)

... 1 more
]
 exitValue = 1

java.lang.RuntimeException: Expected to get exit value of [0]

Thanks,
Dmitry

On 15.09.2016 15:18, Dmitry Samersoff wrote:

Everybody,

Please, review the small fix.

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

The problem:

TestJpsJar attempts to copy all directories found in test.class.path
into a single jar file.

It's not necessary and could lead to intermittent ClassNotFound exceptions.

Solution:

Jar only a directory with required files.

-Dmitry

Re: RFR(XXS): 8165513: Quarantine sun/tools/jps/TestJpsJar.java

2016-09-06 Thread Dmitry Dmitriev 

Dmitry, thank you for the review!

Dmitry

On 06.09.2016 20:32, Dmitry Samersoff wrote:

Dmitry,

Looks good for me!

-Dmitry

On 2016-09-06 19:42, Dmitry Dmitriev wrote:

Hello,

Please review small fix. JDK-8160923
<https://bugs.openjdk.java.net/browse/JDK-8160923> removes this test
from problem list, but it still fails due to JDK-8165500
<https://bugs.openjdk.java.net/browse/JDK-8165500>. Thus put
sun/tools/jps/TestJpsJar.java back to the problem list with new bug id.

JBS: https://bugs.openjdk.java.net/browse/JDK-8165513
webrev.00: http://cr.openjdk.java.net/~ddmitriev/8165513/webrev.00/
<http://cr.openjdk.java.net/%7Eddmitriev/8165513/webrev.00/>
Testing: locally

Thanks,
Dmitry

RFR(XXS): 8165513: Quarantine sun/tools/jps/TestJpsJar.java

2016-09-06 Thread Dmitry Dmitriev 

Hello,

Please review small fix. JDK-8160923 
 removes this test 
from problem list, but it still fails due to JDK-8165500 
. Thus put 
sun/tools/jps/TestJpsJar.java back to the problem list with new bug id.


JBS: https://bugs.openjdk.java.net/browse/JDK-8165513
webrev.00: http://cr.openjdk.java.net/~ddmitriev/8165513/webrev.00/ 


Testing: locally

Thanks,
Dmitry

Re: RFR(S): 8165490: [TESTBUG] sun/tools/jps/TestJpsJar.java still fails with ClassNotFoundException: jdk.testlibrary.ProcessTools

2016-09-06 Thread Dmitry Dmitriev 
FYI: the problem is with new JDK 9 multi-release jar tool which ignores 
second(and others) '-C  .' arguments. Bug: JDK-8165500 "jar not 
proceed second(and others) -C  . arguments" 
<https://bugs.openjdk.java.net/browse/JDK-8165500>


Dmitry

On 06.09.2016 15:35, Dmitry Dmitriev wrote:

Hello, I withdraw this patch because it's wrong. Thanks!

On 06.09.2016 14:43, Dmitry Dmitriev wrote:

Hello,

Please review fix for jdk/test/sun/tools/jps tests. 
sun/tools/jps/TestJpsJar.java still fails with 
ClassNotFoundException: jdk.testlibrary.ProcessTools. The problem is 
that TestJpsJar not add class path to the process builder and fails 
to find test library classes if these classes were compiled before 
with @build directive.


In this fix I also removed unnecessary build directives from 
sun/tools/jps/ tests.


JBS: https://bugs.openjdk.java.net/browse/JDK-8165490
webrev.00: http://cr.openjdk.java.net/~ddmitriev/8165490/webrev.00/ 
<http://cr.openjdk.java.net/%7Eddmitriev/8165490/webrev.00/>

Testing: locally, RBT(in progress)

Thanks,
Dmitry

Re: RFR(S): 8165490: [TESTBUG] sun/tools/jps/TestJpsJar.java still fails with ClassNotFoundException: jdk.testlibrary.ProcessTools

2016-09-06 Thread Dmitry Dmitriev 

Hello, I withdraw this patch because it's wrong. Thanks!

On 06.09.2016 14:43, Dmitry Dmitriev wrote:

Hello,

Please review fix for jdk/test/sun/tools/jps tests. 
sun/tools/jps/TestJpsJar.java still fails with ClassNotFoundException: 
jdk.testlibrary.ProcessTools. The problem is that TestJpsJar not add 
class path to the process builder and fails to find test library 
classes if these classes were compiled before with @build directive.


In this fix I also removed unnecessary build directives from 
sun/tools/jps/ tests.


JBS: https://bugs.openjdk.java.net/browse/JDK-8165490
webrev.00: http://cr.openjdk.java.net/~ddmitriev/8165490/webrev.00/ 
<http://cr.openjdk.java.net/%7Eddmitriev/8165490/webrev.00/>

Testing: locally, RBT(in progress)

Thanks,
Dmitry

RFR(S): 8165490: [TESTBUG] sun/tools/jps/TestJpsJar.java still fails with ClassNotFoundException: jdk.testlibrary.ProcessTools

2016-09-06 Thread Dmitry Dmitriev 

Hello,

Please review fix for jdk/test/sun/tools/jps tests. 
sun/tools/jps/TestJpsJar.java still fails with ClassNotFoundException: 
jdk.testlibrary.ProcessTools. The problem is that TestJpsJar not add 
class path to the process builder and fails to find test library classes 
if these classes were compiled before with @build directive.


In this fix I also removed unnecessary build directives from 
sun/tools/jps/ tests.


JBS: https://bugs.openjdk.java.net/browse/JDK-8165490
webrev.00: http://cr.openjdk.java.net/~ddmitriev/8165490/webrev.00/ 


Testing: locally, RBT(in progress)

Thanks,
Dmitry

Re: RFR(XXS): 8160102: Typo in message for NULL memory size arguments in diagnosticArgument.cpp

2016-06-23 Thread Dmitry Dmitriev 

Robbin, thank you for the review!

Dmitry

On 23.06.2016 12:54, Robbin Ehn wrote:

Hi Dmitry,

Looks good, thanks!

/Robbin

On 06/22/2016 10:57 PM, Dmitry Dmitriev wrote:

Hello,

Please review small patch which fixes error message for NULL memory 
size arguments. Currently message for nanotime arguments is printed. 
I change it to make it consistent

with other error messages for memory size arguments.

JBS: https://bugs.openjdk.java.net/browse/JDK-8160102
webrev.00: http://cr.openjdk.java.net/~ddmitriev/8160102/webrev.00/ 
<http://cr.openjdk.java.net/%7Eddmitriev/8160102/webrev.00/>

Testing: jprt

Thanks,
Dmitry

Re: RFR(XXS): 8160102: Typo in message for NULL memory size arguments in diagnosticArgument.cpp

2016-06-23 Thread Dmitry Dmitriev 

David, thank you for the review!

Dmitry

On 23.06.2016 3:08, David Holmes wrote:

Looks good.

Thanks,
David

On 23/06/2016 6:57 AM, Dmitry Dmitriev wrote:

Hello,

Please review small patch which fixes error message for NULL memory size
arguments. Currently message for nanotime arguments is printed. I change
it to make it consistent with other error messages for memory size
arguments.

JBS: https://bugs.openjdk.java.net/browse/JDK-8160102
webrev.00: http://cr.openjdk.java.net/~ddmitriev/8160102/webrev.00/
<http://cr.openjdk.java.net/%7Eddmitriev/8160102/webrev.00/>
Testing: jprt

Thanks,
Dmitry

RFR(XXS): 8160102: Typo in message for NULL memory size arguments in diagnosticArgument.cpp

2016-06-22 Thread Dmitry Dmitriev 

Hello,

Please review small patch which fixes error message for NULL memory size 
arguments. Currently message for nanotime arguments is printed. I change 
it to make it consistent with other error messages for memory size 
arguments.


JBS: https://bugs.openjdk.java.net/browse/JDK-8160102
webrev.00: http://cr.openjdk.java.net/~ddmitriev/8160102/webrev.00/ 


Testing: jprt

Thanks,
Dmitry

Re: RFR[9u-dev]: 8151442: jstack doesn't close quotation marks properly with threads' name greater than 1996 characters

2016-03-19 Thread Dmitry Dmitriev 

Hello Cheleswer,

It is possible to create a regression test for that?

Dmitry

On 18.03.2016 10:54, Cheleswer Sahu wrote:


Hi,

Please review the code changes for 
https://bugs.openjdk.java.net/browse/JDK-8151442.


Webrev Link: http://cr.openjdk.java.net/~csahu/8151442/

Bug Brief:

In jstack thread dumps , thread name greater than 1996 characters 
doesn’t close quotation marks properly.


Problem Identified:

Jstack is using below code to print thread name

src/share/vm/runtime/thread.cpp

void JavaThread::print_on(outputStream *st) const {

  st->print("\"%s\" ", get_thread_name());

Here “st->print()”  internally uses max buffer length as O_BUFLEN (2000).

void outputStream::do_vsnprintf_and_write_with_automatic_buffer(const 
char* format, va_list ap, bool add_cr) {

  char buffer[O_BUFLEN];

do_vsnprintf_and_write_with_automatic_buffer() finally calls 
 “vsnprintf()”  which truncates the anything greater than the max 
size(2000). In this case thread’s name(> 1996) along with quotation 
marks (2)


plus one terminating character exceeds the max buffer size (2000), 
therefore the closing quotation  marks gets truncated.


Solution:

Split the  “st->print("\"%s\" ", get_thread_name())” in two statements

1.st->print("\"%s", get_thread_name());

2.st->print("\" “);

This will ensure presence of closing quotation mark always.

Regards,

Cheleswer

Re: RFR[9u-dev]: 8151509: In check_addr0() function pointer is not updated correctly

2016-03-11 Thread Dmitry Dmitriev 
Cheleswer, thank you! But I think you need a "R"eviewer for that change 
before push.


Dmitry

On 11.03.2016 14:38, Cheleswer Sahu wrote:


Thanks Dmitry and Thomas for reviews. After adding space I will 
 request for the push.


Regards,

Cheleswer

*From:*Dmitry Dmitriev
*Sent:* Friday, March 11, 2016 5:00 PM
*To:* Cheleswer Sahu; Thomas Stüfe
*Cc:* serviceability-dev@openjdk.java.net; 
hotspot-runtime-...@openjdk.java.net
*Subject:* Re: RFR[9u-dev]: 8151509: In check_addr0() function pointer 
is not updated correctly


Hi Cheleswer,

Please, add space between SIZE_FORMAT and " because C++11 requires a 
space between literal and identifier. Not need a new webrev for that.


Thanks,
Dmitry

On 11.03.2016 12:31, Cheleswer Sahu wrote:

Hi Thomas, Dmitry,

Thanks for your review comments.  My answers are below for your
review comments

1873   if( 0 != ret % sizeof(prmap_t)){
1874 break;
1875   }


>> For this it has been thought that mostly read() will return the
desired number of bytes, but only in case if something goes wrong
and read() will  not able to read the data, it will return lesser
number of bytes, which contains the partial data of  “prmap_t”
structure. The reason could be like file is corrupted, in such
cases we don’t want to read anymore and feel it’s safe to skip the
rest of file.

2) Just interesting, do you really need to set memory to 0 by memset?

>>  I thought this it is good to have a clean buffer every time we
read something into it, but it’s really not that much required as
we are reading a binary data. So I am removing this line from the
code.

For rest of the comments I have made correction in the code. The
new webrev is available in the below location

http://cr.openjdk.java.net/~csahu/8151509/webrev.01/
<http://cr.openjdk.java.net/%7Ecsahu/8151509/webrev.01/>

Regards,

Cheleswer

*From:*Thomas Stüfe [mailto:thomas.stu...@gmail.com]
*Sent:* Thursday, March 10, 2016 7:39 PM
*To:* Dmitry Dmitriev
*Cc:* Cheleswer Sahu; serviceability-dev@openjdk.java.net
<mailto:serviceability-dev@openjdk.java.net>;
hotspot-runtime-...@openjdk.java.net
<mailto:hotspot-runtime-...@openjdk.java.net>
*Subject:* Re: RFR[9u-dev]: 8151509: In check_addr0() function
pointer is not updated correctly

(Sorry, pressed Send button too early)

Just wanted to add that

1873   if( 0 != ret % sizeof(prmap_t)){
1874 break;
1875   }

may be a bit harsh, as it skips the entire mapping in case read()
stopped reading in a middle of a record. You could just continue
to read until you read the rest of the record.

Kind Regards, Thomas

On Thu, Mar 10, 2016 at 3:07 PM, Thomas Stüfe
<thomas.stu...@gmail.com <mailto:thomas.stu...@gmail.com>> wrote:

Hi Cheleswer,

thanks for fixing this.

Some more issues:

- 1866 char *mbuff = (char *) calloc(read_chunk,
sizeof(prmap_t) + 1);

Why the "+1"? It is unnecessary and causes the allocation to
be 200 bytes larger than necessary.

- 1880 st->print("Warning: Address: " PTR_FORMAT ", Size: %dK,
",p->pr_vaddr, p->pr_size/1024);

Format specifier for Size is wrong.%d is int, but p->pr_size
is size_t. Theoretical truncation for mappings larger than
4g*1024.

(But I know this coding was there before)

Beside those points, I think both points of Dmitry are valid.

    Also, I find

Kind Regards, Thomas

On Thu, Mar 10, 2016 at 1:45 PM, Dmitry Dmitriev
<dmitry.dmitr...@oracle.com
<mailto:dmitry.dmitr...@oracle.com>> wrote:

Hi Cheleswer,

Looks good, but I have questions/comments about other code
in this function:
1) I think that "::close(fd);" should be inside "if (fd >=
0) {".
2) Just interesting, do you really need to set memory to 0
by memset?

Thanks,
Dmitry


On 10.03.2016 13:43, Cheleswer Sahu wrote:


Hi,

Please review the code changes for
https://bugs.openjdk.java.net/browse/JDK-8151509.

Webrev link:
http://cr.openjdk.java.net/~csahu/8151509/
<http://cr.openjdk.java.net/%7Ecsahu/8151509/>
<http://cr.openjdk.java.net/%7Ecsahu/8151509/>

Bug Brief:

In check_addr0(),  pointer ”p” is not updated
correctly, because of this it was reading only first
two entries from buffer.

Regards,

Cheleswer

Re: RFR[9u-dev]: 8151509: In check_addr0() function pointer is not updated correctly

2016-03-11 Thread Dmitry Dmitriev 

Hi Cheleswer,

Please, add space between SIZE_FORMAT and " because C++11 requires a 
space between literal and identifier. Not need a new webrev for that.


Thanks,
Dmitry

On 11.03.2016 12:31, Cheleswer Sahu wrote:


Hi Thomas, Dmitry,

Thanks for your review comments.  My answers are below for your review 
comments


1873   if( 0 != ret % sizeof(prmap_t)){
1874 break;
1875   }

>> For this it has been thought that mostly read() will return the 
desired number of bytes, but only in case if something goes wrong and 
read() will  not able to read the data, it will return lesser number 
of bytes, which contains the partial data of  “prmap_t” structure. The 
reason could be like file is corrupted, in such cases we don’t want to 
read anymore and feel it’s safe to skip the rest of file.


2) Just interesting, do you really need to set memory to 0 by memset?

>>  I thought this it is good to have a clean buffer every time we 
read something into it, but it’s really not that much required as we 
are reading a binary data. So I am removing this line from the code.


For rest of the comments I have made correction in the code. The new 
webrev is available in the below location


http://cr.openjdk.java.net/~csahu/8151509/webrev.01/ 
<http://cr.openjdk.java.net/%7Ecsahu/8151509/webrev.01/>


Regards,

Cheleswer

*From:*Thomas Stüfe [mailto:thomas.stu...@gmail.com]
*Sent:* Thursday, March 10, 2016 7:39 PM
*To:* Dmitry Dmitriev
*Cc:* Cheleswer Sahu; serviceability-dev@openjdk.java.net; 
hotspot-runtime-...@openjdk.java.net
*Subject:* Re: RFR[9u-dev]: 8151509: In check_addr0() function pointer 
is not updated correctly


(Sorry, pressed Send button too early)

Just wanted to add that

1873   if( 0 != ret % sizeof(prmap_t)){
1874 break;
1875   }

may be a bit harsh, as it skips the entire mapping in case read() 
stopped reading in a middle of a record. You could just continue to 
read until you read the rest of the record.


Kind Regards, Thomas

On Thu, Mar 10, 2016 at 3:07 PM, Thomas Stüfe <thomas.stu...@gmail.com 
<mailto:thomas.stu...@gmail.com>> wrote:


Hi Cheleswer,

thanks for fixing this.

Some more issues:

- 1866 char *mbuff = (char *) calloc(read_chunk, sizeof(prmap_t) + 1);

Why the "+1"? It is unnecessary and causes the allocation to be
200 bytes larger than necessary.

- 1880 st->print("Warning: Address: " PTR_FORMAT ", Size: %dK,
",p->pr_vaddr, p->pr_size/1024);

Format specifier for Size is wrong.%d is int, but p->pr_size is
size_t. Theoretical truncation for mappings larger than 4g*1024.

(But I know this coding was there before)

Beside those points, I think both points of Dmitry are valid.

Also, I find

    Kind Regards, Thomas

On Thu, Mar 10, 2016 at 1:45 PM, Dmitry Dmitriev
<dmitry.dmitr...@oracle.com <mailto:dmitry.dmitr...@oracle.com>>
wrote:

Hi Cheleswer,

Looks good, but I have questions/comments about other code in
this function:
1) I think that "::close(fd);" should be inside "if (fd >= 0) {".
2) Just interesting, do you really need to set memory to 0 by
memset?

Thanks,
Dmitry


On 10.03.2016 13:43, Cheleswer Sahu wrote:


Hi,

Please review the code changes for
https://bugs.openjdk.java.net/browse/JDK-8151509.

Webrev link: http://cr.openjdk.java.net/~csahu/8151509/
<http://cr.openjdk.java.net/%7Ecsahu/8151509/>
<http://cr.openjdk.java.net/%7Ecsahu/8151509/>

Bug Brief:

In check_addr0(),  pointer ”p” is not updated correctly,
because of this it was reading only first two entries from
buffer.

Regards,

Cheleswer

Re: RFR[9u-dev]: 8151509: In check_addr0() function pointer is not updated correctly

2016-03-10 Thread Dmitry Dmitriev 

Hi Cheleswer,

Looks good, but I have questions/comments about other code in this function:
1) I think that "::close(fd);" should be inside "if (fd >= 0) {".
2) Just interesting, do you really need to set memory to 0 by memset?

Thanks,
Dmitry


On 10.03.2016 13:43, Cheleswer Sahu wrote:


Hi,

Please review the code changes for 
https://bugs.openjdk.java.net/browse/JDK-8151509.


Webrev link: http://cr.openjdk.java.net/~csahu/8151509/ 



Bug Brief:

In check_addr0(),  pointer ”p” is not updated correctly, because of 
this it was reading only first two entries from buffer.


Regards,

Cheleswer

Re: RFR(XXS): 8151100: Test java/lang/instrument/NativeMethodPrefixAgent.java can't attempt to do CheckIntrinsics

2016-03-03 Thread Dmitry Dmitriev 

Hello Markus,

CheckIntrinsics is a diagnostic flag. I think you need to add 
-XX:+UnlockDiagnosticVMOptions before it because otherwise test will 
fail on product build.


Thanks,
Dmitry

On 03.03.2016 3:15, Markus Gronlund wrote:


Greetings,

Could a please ask for reviews for the following simple fix to resolve 
a test issue associated with 
test/java/lang/instrument/NativeMethodPrefixAgent.java


Bug: https://bugs.openjdk.java.net/browse/JDK-8151100

Webrev/diff:

diff --git a/test/java/lang/instrument/NativeMethodPrefixAgent.java 
b/test/java/lang/instrument/NativeMethodPrefixAgent.java


--- a/test/java/lang/instrument/NativeMethodPrefixAgent.java

+++ b/test/java/lang/instrument/NativeMethodPrefixAgent.java

@@ -31,7 +31,7 @@

  * java.management

  * java.instrument

  * @run shell/timeout=240 MakeJAR2.sh NativeMethodPrefixAgent 
NativeMethodPrefixApp 'Can-Retransform-Classes: true' 
'Can-Set-Native-Method-Prefix: true'


- * @run main/othervm -javaagent:NativeMethodPrefixAgent.jar 
NativeMethodPrefixApp


+ * @run main/othervm -XX:-CheckIntrinsics 
-javaagent:NativeMethodPrefixAgent.jar NativeMethodPrefixApp


*/

 import java.lang.instrument.*;

Description:

The java/lang/instrument/NativeMethodPrefixAgent.java modifies (wraps) 
the names of native methods.


If a class is loaded that contain the @HotSpotIntrinsicCandidate 
annotation on a native method, one step of the class file parser is an 
attempt to validate the method name against a registered intrinsic in 
the VM.


By default, the flag CheckIntrinsics is true, which will yield:

“Method 
[.wrapped_tr0_wrapped_tr1_wrapped_tr2_()] 
is annotated with @HotSpotIntrinsicCandidate, but no compiler 
intrinsic is defined for the method. Exiting.”


This "wrapped" prepending to the method name invalidates the intrinsic 
check.


In order for the test to pass even when loading a class with a native 
method containing the @HotSpotIntrinsicCandidate annotation, the test 
should explicitly disable the CheckIntrinsics flag:


-XX:-CheckIntrinsics

Thanks in advance

Markus