from:"Cheleswer Sahu"

RFR[ 9u-dev] JDK-8138745: Implement ExitOnOutOfMemory and CrashOnOutOfMemory in HotSpot

2015-11-25 Thread cheleswer sahu


Hi,

Please review the code changes for 
"https://bugs.openjdk.java.net/browse/JDK-8138745";.
Web review link: http://cr.openjdk.java.net/~kevinw/8138745/webrev.00/ 



Enhancement Brief:
ExitOnOutOfMemoryError: When user enable this option, the JVM exits on 
the first occurrence of an out-of-memory error. It can be used if user 
prefer restarting an instance of the JVM rather than handling out of 
memory errors.


CrashOnOutOfMemoryError: If this option is enabled, when an 
out-of-memory error occurs, the JVM crashes and produces text and binary 
crash files.


For more details please refer http://ccc.us.oracle.com/8138745


Regards,
Cheleswer

Re: RFR[ 9u-dev] JDK-8138745: Implement ExitOnOutOfMemory and CrashOnOutOfMemory in HotSpot

2015-12-02 Thread cheleswer sahu

Hi,
Thanks David and Staffan for your comments. Please review the code
changes in the updated webrev below

http://cr.openjdk.java.net/~kevinw/8138745/webrev.01/

Regards,
Cheleswer

On 11/26/2015 11:58 AM, David Holmes wrote:

Sorry forgot the tests

test/runtime/ErrorHandling/TestExitOnOutOfMemoryError.java

This test is checking that new Object[Integer.MAX_VALUE]; caused the
"Requested array size exceeds VM limit" failure _but_ it doesn't
actually do anything to verify that the VM terminated because of the
ExitOnOutOfMemory flag. I suggest:

a) augment the termination message in the VM as I suggested earlier so
that you can be sure you hit the termination code

b) check for a zero/non-zero return code as appropriate
c) Add: throw new Error("OOME was not triggered"); after line 41.
d) Put a try/catch(OOME) around the allocation and throw an Error if
you get to the catch block

That way we will get a test failure when Arrays 2.0 allows such
massive arrays to be created :)

Similar considerations for TestCrashOnOutOfMemoryError.java, but you
also need to disable core dump generation.

David
-

On 26/11/2015 3:43 PM, David Holmes wrote:

Hi,

On 25/11/2015 10:40 PM, cheleswer sahu wrote:

Hi,

Please review the code changes for
"https://bugs.openjdk.java.net/browse/JDK-8138745";.
Web review link:
<http://cr.openjdk.java.net/%7Ekevinw/8138745/webrev.00/>http://cr.openjdk.java.net/~kevinw/8138745/webrev.00/

Enhancement Brief:
ExitOnOutOfMemoryError: When user enable this option, the JVM exits on
the first occurrence of an out-of-memory error. It can be used if user
prefer restarting an instance of the JVM rather than handling out of
memory errors.

CrashOnOutOfMemoryError: If this option is enabled, when an
out-of-memory error occurs, the JVM crashes and produces text and
binary

crash files.

The term "crash" is not very appropriate - crashes are bad things. Abort
may have been a better choice.

For more details please refer http://ccc.us.oracle.com/8138745

This is not accessible outside of Oracle.

A few minor comments:

src/share/vm/runtime/globals.hpp

+ "JVM crashes and produces text and binary crash files")

Terminology should be consistent with other options that control core
dump. Should also say "on first occurrence of an out-of-memory error".

src/share/vm/utilities/debug.cpp:

308 if (CrashOnOutOfMemoryError) {
309 tty->print_cr("java.lang.OutOfMemoryError: %s", message);
310 fatal("OutOfMemory encountered: %s", message);
311 }

don't really need the tty->print when using the fatal. Though you may
want to use the j.l.OOME form of the message for consistency. Might also
be useful for both Crash and Exit to include in the logged messages the
fact that these flags were involved - something like:

"Terminating due to java.lang.OutOfMemoryError: %s"
"Aborting due to java.lang.OutOfMemoryError: %s"

Thanks,
David
-

Regards,
Cheleswer

Re: RFR[ 9u-dev] JDK-8138745: Implement ExitOnOutOfMemory and CrashOnOutOfMemory in HotSpot

2015-12-08 Thread cheleswer sahu


Hi David,
Please review the code changes in updated webrev
http://cr.openjdk.java.net/~kevinw/8138745/webrev.02/

Regards,
Cheleswer

On 12/8/2015 4:36 AM, David Holmes wrote:

Hi Cheleswer,

Thanks. Just a few minor updates ...

On 3/12/2015 4:58 PM, cheleswer sahu wrote:

Hi,
Thanks David and Staffan for your comments. Please review the code
changes in the updated webrev below
http://cr.openjdk.java.net/~kevinw/8138745/webrev.01/


src/share/vm/runtime/globals.hpp

As I said previously this terminology:

+   "JVM aborts and produces text and binary crash files on 
the " \


should be consistent with what we already use. So I suggest:

+   "JVM aborts, producing an error log and core/mini dump, on 
the " \



In the tests either "throw new Error(...)" or "throw new 
RuntimeException(...)" but don't switch between them.


test/runtime/ErrorHandling/TestExitOnOutOfMemoryError.java

  45 } catch (OutOfMemoryError err) {
  46 throw err;
  47 }

should be:

  45 } catch (OutOfMemoryError err) {
  46 throw new Error("OOME didn't terminate JVM!");
  47 }

---

test/runtime/ErrorHandling/TestCrashOnOutOfMemoryError.java

  50 } catch (OutOfMemoryError err) {
  51 throw err;
  52 }

should be:

  50  } catch (OutOfMemoryError err) {
  51  throw new Error("OOME didn't abort JVM!");
  52  }


Here:

 55 ProcessBuilder pb = 
ProcessTools.createJavaProcessBuilder("-XX:+CrashOnOutOfMemoryError",
  56 "-Xmx64m", 
TestCrashOnOutOfMemoryError.class.getName(),"throwOOME");


you also need to disable core dump generation: 
-XX:-CreateCoredumpOnCrash  otherwise you may run into issues with 
core dumps taking a long time to generate and the test times out.



 64 /* Actual output should look like this:

"should look _something_ like this". The actual text will depend on 
the OS and its core dump processing.


Thanks,
David



Regards,
Cheleswer

On 11/26/2015 11:58 AM, David Holmes wrote:

Sorry forgot the tests 

test/runtime/ErrorHandling/TestExitOnOutOfMemoryError.java

This test is checking that new Object[Integer.MAX_VALUE]; caused the
"Requested array size exceeds VM limit" failure _but_ it doesn't
actually do anything to verify that the VM terminated because of the
ExitOnOutOfMemory flag. I suggest:

a) augment the termination message in the VM as I suggested earlier so
that you can be sure you hit the termination code
b) check for a zero/non-zero return code as appropriate
c) Add: throw new Error("OOME was not triggered"); after line 41.
d) Put a try/catch(OOME) around the allocation and throw an Error if
you get to the catch block

That way we will get a test failure when Arrays 2.0 allows such
massive arrays to be created :)

Similar considerations for TestCrashOnOutOfMemoryError.java, but you
also need to disable core dump generation.

David
-

On 26/11/2015 3:43 PM, David Holmes wrote:

Hi,

On 25/11/2015 10:40 PM, cheleswer sahu wrote:

Hi,

Please review the code changes for
"https://bugs.openjdk.java.net/browse/JDK-8138745";.
Web review link:
<http://cr.openjdk.java.net/%7Ekevinw/8138745/webrev.00/>http://cr.openjdk.java.net/~kevinw/8138745/webrev.00/ 





Enhancement Brief:
ExitOnOutOfMemoryError: When user enable this option, the JVM 
exits on
the first occurrence of an out-of-memory error. It can be used if 
user

prefer restarting an instance of the JVM rather than handling out of
memory errors.

CrashOnOutOfMemoryError: If this option is enabled, when an
out-of-memory error occurs, the JVM crashes and produces text and
binary
crash files.


The term "crash" is not very appropriate - crashes are bad things. 
Abort

may have been a better choice.


For more details please refer http://ccc.us.oracle.com/8138745


This is not accessible outside of Oracle.

A few minor comments:

src/share/vm/runtime/globals.hpp

+   "JVM crashes and produces text and binary crash files")

Terminology should be consistent with other options that control core
dump. Should also say "on first occurrence of an out-of-memory error".

src/share/vm/utilities/debug.cpp:

  308 if (CrashOnOutOfMemoryError) {
  309   tty->print_cr("java.lang.OutOfMemoryError: %s", message);
  310   fatal("OutOfMemory encountered: %s", message);
  311 }

don't really need the tty->print when using the fatal. Though you may
want to use the j.l.OOME form of the message for consistency. Might 
also
be useful for both Crash and Exit to include in the logged messages 
the

fact that these flags were involved - something like:

"Terminating due to java.lang.OutOfMemoryError: %s"
"Aborting due to java.lang.OutOfMemoryError: %s"

Thanks,
David
-



Regards,
Cheleswer

RFR[9u-dev] 8130425: libjvm crash due to stack overflow in executables with 32k tbss/tdata

2016-01-13 Thread cheleswer sahu


Hi,

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


Problem summary: When a large TLS (Thread Local Storage) size is set for 
threads,  JVM is throwing StackOverflow exception.


Problem Identified:
As per investigation and a discussion we came to the conclusion that 
issue is not with the JVM but it lies in the way glibc has been 
implemented. When a TLS is declared , it steals the space from threads 
stack size.
So if a thread is created with small stack size, and TLS is set to a 
large value, then it will result in StackOverflow. This is the exact 
case in this bug where reaper thread is allocated a very low stack size 
32768.


Discussion thread: 
http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-December/037558.html 



Solution proposed:
Its expected to get fix in glibc sometime , but for now I propose a 
workaround, a boolean system property "processReaperUseDefaultStackSize" 
using which we can set the stack size for reaper thread to default
instead of fix 32768. This property can be set by the user using "-D" or 
"System.setProperty()". I have tested this fix, it works well with TLS 
size between 32k to 128k.


Fix:
diff -r 5c4530bb9ae6 
src/java.base/share/classes/java/lang/ProcessHandleImpl.jav
--- a/src/java.base/share/classes/java/lang/ProcessHandleImpl.java Fri 
Jan 08 13:06:29 2016 +0800
+++ b/src/java.base/share/classes/java/lang/ProcessHandleImpl.java Tue 
Jan 12 15:55:50 2016 +0530

@@ -83,9 +83,13 @@
 ThreadGroup systemThreadGroup = tg;

 ThreadFactory threadFactory = grimReaper -> {
-// Our thread stack requirement is quite modest.
-Thread t = new Thread(systemThreadGroup, grimReaper,
-"process reaper", 32768);
+   Thread t = null;
+   if 
(Boolean.getBoolean("processReaperUseDefaultStackSize")) {
+   t = new Thread(systemThreadGroup, grimReaper, 
"process reaper");

+} else {
+   // Our thread stack requirement is quite modest.
+   t = new Thread(systemThreadGroup, grimReaper, 
"process reaper", 32768);

+}
 t.setDaemon(true);
 // A small attempt (probably futile) to avoid 
priority inversion

 t.setPriority(Thread.MAX_PRIORITY);


Regards,
Cheleswer

Re: RFR[9u-dev] 8130425: libjvm crash due to stack overflow in executables with 32k tbss/tdata

2016-01-13 Thread cheleswer sahu


Adding core-libs-dev and hotspot-runtime-dev team .

On 1/14/2016 12:24 AM, Martin Buchholz wrote:

You should include core-libs-dev (and perhaps hotspot-runtime-dev) in
this thread.  You're changing core library code.

On Wed, Jan 13, 2016 at 2:25 AM, cheleswer sahu
 wrote:

Hi,

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

Problem summary: When a large TLS (Thread Local Storage) size is set for
threads,  JVM is throwing StackOverflow exception.

Problem Identified:
As per investigation and a discussion we came to the conclusion that issue
is not with the JVM but it lies in the way glibc has been implemented. When
a TLS is declared , it steals the space from threads stack size.
So if a thread is created with small stack size, and TLS is set to a large
value, then it will result in StackOverflow. This is the exact case in this
bug where reaper thread is allocated a very low stack size 32768.

Discussion thread:
http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-December/037558.html

Solution proposed:
Its expected to get fix in glibc sometime , but for now I propose a
workaround, a boolean system property "processReaperUseDefaultStackSize"
using which we can set the stack size for reaper thread to default
instead of fix 32768. This property can be set by the user using "-D" or
"System.setProperty()". I have tested this fix, it works well with TLS size
between 32k to 128k.

Fix:
diff -r 5c4530bb9ae6
src/java.base/share/classes/java/lang/ProcessHandleImpl.jav
--- a/src/java.base/share/classes/java/lang/ProcessHandleImpl.java Fri Jan
08 13:06:29 2016 +0800
+++ b/src/java.base/share/classes/java/lang/ProcessHandleImpl.java Tue Jan
12 15:55:50 2016 +0530
@@ -83,9 +83,13 @@
  ThreadGroup systemThreadGroup = tg;

  ThreadFactory threadFactory = grimReaper -> {
-// Our thread stack requirement is quite modest.
-Thread t = new Thread(systemThreadGroup, grimReaper,
-"process reaper", 32768);
+   Thread t = null;
+   if
(Boolean.getBoolean("processReaperUseDefaultStackSize")) {
+   t = new Thread(systemThreadGroup, grimReaper,
"process reaper");
+} else {
+   // Our thread stack requirement is quite modest.
+   t = new Thread(systemThreadGroup, grimReaper,
"process reaper", 32768);
+}
  t.setDaemon(true);
  // A small attempt (probably futile) to avoid priority
inversion
  t.setPriority(Thread.MAX_PRIORITY);


Regards,
Cheleswer

Re: RFR[9u-dev] 8130425: libjvm crash due to stack overflow in executables with 32k tbss/tdata

2016-02-17 Thread cheleswer sahu


Hi,
I have made changes in the property name 
(jdk.lang.processReaperUseDefaultStackSize) and code as suggested in the 
earlier reviews.


 --- old/src/java.base/share/classes/java/lang/ProcessHandleImpl.java 
2016-02-17 18:48:10.54563 +0530
+++ new/src/java.base/share/classes/java/lang/ProcessHandleImpl.java 
2016-02-17 18:48:10.08163 +0530

@@ -81,9 +81,8 @@
 ThreadGroup systemThreadGroup = tg;

 ThreadFactory threadFactory = grimReaper -> {
-// Our thread stack requirement is quite modest.
-Thread t = new Thread(systemThreadGroup, grimReaper,
-"process reaper", 32768);
+long stackSize = 
Boolean.getBoolean("jdk.lang.processReaperUseDefaultStackSize") ? 0 : 32768;
+Thread t = new Thread(systemThreadGroup, 
grimReaper, "process reaper", stackSize);

 t.setDaemon(true);
 // A small attempt (probably futile) to avoid 
priority inversion

 t.setPriority(Thread.MAX_PRIORITY);

I would really like to thanks "Martin" for the patch 
(http://cr.openjdk.java.net/~martin/webrevs/openjdk9/tls-size-guarantee/ 
<http://cr.openjdk.java.net/%7Emartin/webrevs/openjdk9/tls-size-guarantee/>) 
which he has provided. Since we support a wider range of glibc versions 
and platform using patch will
require more testing and work. I think the use of system property for 
this issue will be safer at this point of time.


Regards,
Cheleswer


On 1/19/2016 5:40 PM, David Holmes wrote:

On 19/01/2016 9:53 PM, Kevin Walls wrote:

|
Hi Cheleswer, I think Martin is suggesting something like:
|

// Use a modest stack size, unless requested otherwise:
long stackSize = 
Boolean.getBoolean("processReaperUseDefaultStackSize") ? 0 : 32768;


Someone from core-libs needs to chime on what the appropriate 
namespace for such a property would be.


David
-

Thread t = new Thread(systemThreadGroup, grimReaper, "process 
reaper", stackSize);


|||
If that tests OK for you, it may be the way we can go ahead with the
point fix for this.

Regards
Kevin
|
On 18/01/2016 16:52, Martin Buchholz wrote:

Historical note - I never liked having a reaper thread for each
subprocess, but no other reliable implementation is known. Given the
potential for many reaper threads, I at least wanted to keep the
memory waste low.

On Wed, Jan 13, 2016 at 2:25 AM, cheleswer sahu
 wrote:


+   Thread t = null;
+   if
(Boolean.getBoolean("processReaperUseDefaultStackSize")) {
+   t = new Thread(systemThreadGroup, grimReaper,
"process reaper");
+} else {
+   // Our thread stack requirement is quite 
modest.

+   t = new Thread(systemThreadGroup, grimReaper,
"process reaper", 32768);
+}

If we do end up using this strategy, cleaner would be to use
https://docs.oracle.com/javase/8/docs/api/java/lang/Thread.html#Thread-java.lang.ThreadGroup-java.lang.Runnable-java.lang.String-long- 


stackSize - the desired stack size for the new thread, or zero to
indicate that this parameter is to be ignored.




--- old/src/java.base/share/classes/java/lang/ProcessHandleImpl.java
2016-02-17 18:48:10.54563 +0530
+++ new/src/java.base/share/classes/java/lang/ProcessHandleImpl.java
2016-02-17 18:48:10.08163 +0530
@@ -81,9 +81,8 @@
 ThreadGroup systemThreadGroup = tg;
 
 ThreadFactory threadFactory = grimReaper -> {
-// Our thread stack requirement is quite modest.
-Thread t = new Thread(systemThreadGroup, grimReaper,
-"process reaper", 32768);
+long stackSize = 
Boolean.getBoolean("jdk.lang.processReaperUseDefaultStackSize") ? 0 : 32768;
+Thread t = new Thread(systemThreadGroup, grimReaper, 
"process reaper", stackSize); 
 t.setDaemon(true);
 // A small attempt (probably futile) to avoid priority 
inversion
 t.setPriority(Thread.MAX_PRIORITY);

RFR[9u-dev]: 8146683: check_addr0 should be more efficient

2016-02-23 Thread Cheleswer Sahu

Hi,

 

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

 

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

JBS Link: https://bugs.openjdk.java.net/browse/JDK-8146683

 

Bug Brief: 

While investigating  bug https://bugs.openjdk.java.net/browse/JDK-8144483  it 
has been observed that "check_addr0() " function  is not written efficiently.

This function is trying to read each "prmap_t" entry in "/proc/self/map" file 
one by one which is time taking and can cause in long pauses.

 

Solution proposed:

Instead of reading each "prmap_t" entry in "/proc/self/map" file one by one, 
read the entries in chunks. There can be many entries in "map" file, so I have 
decided to read these

in chunks of 100  "prmap_t" entries. Reading entries in chunks saves a lot of 
read calls and results in smaller pause times. 

 



 

Regards,

Cheleswer

RE: RFR[9u-dev]: 8146683: check_addr0 should be more efficient

2016-03-03 Thread Cheleswer Sahu

Hi,

Please review the new code changes for this bug. I have removed  " fstat()"  
call which seems not safe to read the "/proc/map/self" file and have made some 
other changes too. Please find the latest code in below link
Webrev link: http://cr.openjdk.java.net/~csahu/8146683/webrev.01/


Regards,
Cheleswer

-Original Message-
From: Dean Long 
Sent: Wednesday, February 24, 2016 7:44 AM
To: Daniel Daugherty; Thomas Stüfe; Cheleswer Sahu
Cc: serviceability-dev@openjdk.java.net; hotspot-runtime-...@openjdk.java.net
Subject: Re: RFR[9u-dev]: 8146683: check_addr0 should be more efficient

On 2/23/2016 10:07 AM, Daniel D. Daugherty wrote:
> On 2/23/16 5:56 AM, Thomas Stüfe wrote:
>> Hi Cheleswer,
>>
>>
>> On Tue, Feb 23, 2016 at 9:38 AM, Cheleswer Sahu 
>> 
>>   wrote:
>>
>>> Hi,
>>>
>>>
>>>
>>> Please review the code changes for
>>> https://bugs.openjdk.java.net/browse/JDK-8146683 .
>>>
>>>
>>>
>>> Webrev link: http://cr.openjdk.java.net/~csahu/8146683/
>>>
>>> JBS Link: https://bugs.openjdk.java.net/browse/JDK-8146683
>>>
>>>
>>>
>>> Bug Brief:
>>>
>>> While investigating  bug
>>> https://bugs.openjdk.java.net/browse/JDK-8144483
>>> it has been observed that "check_addr0() " function  is not written 
>>> efficiently.
>>>
>>> This function is trying to read each "prmap_t" entry in 
>>> "/proc/self/map"
>>> file one by one which is time taking and can cause in long pauses.
>>>
>>>
>>>
>>> Solution proposed:
>>>
>>> Instead of reading each "prmap_t" entry in "/proc/self/map" file one 
>>> by one, read the entries in chunks. There can be many entries in "map"
>>> file,
>>> so I have decided to read these
>>>
>>> in chunks of 100  "prmap_t" entries. Reading entries in chunks saves 
>>> a lot of read calls and results in smaller pause times.
>>>
>>>
>>>
>>>
>>>
>>> Regards,
>>>
>>> Cheleswer
>>>
>>
>> We saw cases, especially on Solaris, where the content of a file in the
>> proc file system changed under us while we were reading it. So this 
>> should
>> be kept in mind. The original code seems also broken in that regard,
>> because it assumes the ::read() call to read the full size of a prmap_t
>> entry, but it may theoretically read an incomplete entry.
>>
>> As for your coding, you first estimate the size of the mapping file and
>> then use this to determine how many entries to read; but when 
>> reading, this
>> information may already be stale. I think it would be more robust and 
>> also
>> simpler to just try to read as many entries as possible - in chunks, to
>> make reading faster - until you get EOF.
>>
>> Kind Regards, Thomas
>
> I'm really sure that we've been down this road before. In particular,
> Dmitry Samersoff has worked on this issue (or one very much like it)
> before, but he is on vacation until the end of the month.
>

There was a similar issue with Linux /proc/self/maps, and whether it was 
safe to read the file with buffered stdio or not.  See 8009062 and 7017193.

dl

> Dan
>
>
>>
>> On Tue, Feb 23, 2016 at 9:38 AM, Cheleswer Sahu 
>> 
>> wrote:
>>
>>> Hi,
>>>
>>>
>>>
>>> Please review the code changes for
>>> https://bugs.openjdk.java.net/browse/JDK-8146683 .
>>>
>>>
>>>
>>> Webrev link: http://cr.openjdk.java.net/~csahu/8146683/
>>>
>>> JBS Link: https://bugs.openjdk.java.net/browse/JDK-8146683
>>>
>>>
>>>
>>> Bug Brief:
>>>
>>> While investigating  bug 
>>> https://bugs.openjdk.java.net/browse/JDK-8144483
>>> it has been observed that "check_addr0() " function  is not written
>>> efficiently.
>>>
>>> This function is trying to read each "prmap_t" entry in 
>>> "/proc/self/map"
>>> file one by one which is time taking and can cause in long pauses.
>>>
>>>
>>>
>>> Solution proposed:
>>>
>>> Instead of reading each "prmap_t" entry in "/proc/self/map" file one by
>>> one, read the entries in chunks. There can be many entries in "map" 
>>> file,
>>> so I have decided to read these
>>>
>>> in chunks of 100  "prmap_t" entries. Reading entries in chunks saves 
>>> a lot
>>> of read calls and results in smaller pause times.
>>>
>>>
>>>
>>>
>>>
>>> Regards,
>>>
>>> Cheleswer
>>>
>

RE: RFR[9u-dev]: 8146683: check_addr0 should be more efficient

2016-03-09 Thread Cheleswer Sahu

Hi Thomas,

 

Thanks for identifying the issue. I will file a new bug for this and correct 
the issue.

 

 

Regards,

Cheleswer

 

 

From: Thomas Stüfe [mailto:thomas.stu...@gmail.com] 
Sent: Wednesday, March 09, 2016 4:44 PM
To: David Holmes
Cc: Kevin Walls; Cheleswer Sahu; Daniel Daugherty; 
serviceability-dev@openjdk.java.net; hotspot-runtime-...@openjdk.java.net
Subject: Re: RFR[9u-dev]: 8146683: check_addr0 should be more efficient

 

Hi all,

 

I see the change is already pushed, but I am not sure this works as intended:

 

We iterate now over all fully read items:


+      p = (prmap_t *)mbuff;
+      for(int i = 0; i < nmap; i++){
+        if (p->pr_vaddr == 0x0) {
+   
+        }
+        p = (prmap_t *)(mbuff + sizeof(prmap_t));
+      }

 

Where do we move the pointer forward? The way I see it, we only ever examine 
the first and the second item, then repeat examining the second item over and 
over again.

 

Instead of 

 

+        p = (prmap_t *)(mbuff + sizeof(prmap_t));

 

Should this not be

 

p = ((prmap_t *)mbuff) + i;

 

or just

 

p++;

 

?

 

Kind Regards, Thomas

 

 

 

 

 

 

On Mon, Mar 7, 2016 at 12:23 PM, David Holmes mailto:david.hol...@oracle.com"david.hol...@oracle.com> wrote:

On 7/03/2016 8:36 PM, Kevin Walls wrote:


Hi Cheleswer,

Looks good.  (While we've discussed it offline I haven't actually
offered a public review).

Just one more thing, and this concerns the original code not the change:

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


Also %x is the wrong format specifier:

typedef struct prmap {
        uintptr_t pr_vaddr;     /* virtual address of mapping */

David
-

 

We pass 3 params when we only print two items?  The "p->pr_mapname", we
pass again on the next line where we do have the the "%s" in the format
string, so this one is unnecessary. 8-)

Thanks
Kevin



On 07/03/2016 10:07, Dmitry Samersoff wrote:

Cheleswer,

Looks good for me.

-Dmitry


On 2016-03-03 19:28, Cheleswer Sahu wrote:

Hi,

Please review the new code changes for this bug. I have removed  "
fstat()"  call which seems not safe to read the "/proc/map/self" file
and have made some other changes too. Please find the latest code in
below link
Webrev link: http://cr.openjdk.java.net/~csahu/8146683/webrev.01/


Regards,
Cheleswer

-Original Message-
From: Dean Long
Sent: Wednesday, February 24, 2016 7:44 AM
To: Daniel Daugherty; Thomas Stüfe; Cheleswer Sahu
Cc: HYPERLINK 
"mailto:serviceability-dev@openjdk.java.net"serviceability-dev@openjdk.java.net;
HYPERLINK 
"mailto:hotspot-runtime-...@openjdk.java.net"hotspot-runtime-...@openjdk.java.net
Subject: Re: RFR[9u-dev]: 8146683: check_addr0 should be more efficient

On 2/23/2016 10:07 AM, Daniel D. Daugherty wrote:

On 2/23/16 5:56 AM, Thomas Stüfe wrote:

Hi Cheleswer,


On Tue, Feb 23, 2016 at 9:38 AM, Cheleswer Sahu
mailto:cheleswer.s...@oracle.com"cheleswer.s...@oracle.com>
   wrote:

Hi,



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



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

JBS Link: https://bugs.openjdk.java.net/browse/JDK-8146683



Bug Brief:

While investigating  bug
https://bugs.openjdk.java.net/browse/JDK-8144483
it has been observed that "check_addr0() " function  is not written
efficiently.

This function is trying to read each "prmap_t" entry in
"/proc/self/map"
file one by one which is time taking and can cause in long pauses.



Solution proposed:

Instead of reading each "prmap_t" entry in "/proc/self/map" file one
by one, read the entries in chunks. There can be many entries in
"map"
file,
so I have decided to read these

in chunks of 100  "prmap_t" entries. Reading entries in chunks saves
a lot of read calls and results in smaller pause times.





Regards,

Cheleswer

We saw cases, especially on Solaris, where the content of a file in
the
proc file system changed under us while we were reading it. So this
should
be kept in mind. The original code seems also broken in that regard,
because it assumes the ::read() call to read the full size of a
prmap_t
entry, but it may theoretically read an incomplete entry.

As for your coding, you first estimate the size of the mapping file
and
then use this to determine how many entries to read; but when
reading, this
information may already be stale. I think it would be more robust and
also
simpler to just try to read as many entries as possible - in
chunks, to
make reading faster - until you get EOF.

Kind Regards, Thomas

I'm really sure that we've been down this road before. In particular,
Dmitry Samersoff has worked on this issue (or one very much like it)
before, but he is on vacation until the end of the month.

There was a

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

2016-03-10 Thread Cheleswer Sahu

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[9u-dev]: 8151509: In check_addr0() function pointer is not updated correctly

2016-03-11 Thread Cheleswer Sahu

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/

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 mailto:thomas.stu...@gmail.com"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 mailto:dmitry.dmitr...@oracle.com"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/>

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 Cheleswer Sahu

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

HYPERLINK 
"http://cr.openjdk.java.net/%7Ecsahu/8151509/webrev.01/"http://cr.openjdk.java.net/~csahu/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; HYPERLINK 
"mailto:serviceability-dev@openjdk.java.net"serviceability-dev@openjdk.java.net;
 HYPERLINK 
"mailto:hotspot-runtime-...@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 mailto:thomas.stu...@gmail.com"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 mailto:dmitry.dmitr...@oracle.com"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: HYPERLINK 
"http://cr.openjdk.java.net/%7Ecsahu/8151509/"http://cr.openjdk.java.net/~csahu/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-14 Thread Cheleswer Sahu

Thanks David.
I have created change-set against hs-rt which is identical to jdk9/dev. 
Copyright year is already updated in hs-rt file.  Just for reference I have 
uploaded the latest change-set in the below link.
http://cr.openjdk.java.net/~csahu/8151509/webrev.02/

Thanks ,
Cheleswer 

-Original Message-
From: David Holmes 
Sent: Monday, March 14, 2016 7:19 AM
To: Cheleswer Sahu; Dmitry Dmitriev; 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

On 11/03/2016 9:38 PM, Cheleswer Sahu wrote:
> Thanks Dmitry and Thomas for reviews. After adding space I will  request for 
> the push.

Consider this Reviewed.

But this needs to pushed to hs-rt and currently the webrev is against jdk9/dev.

Thanks,
David

>
>
> 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
>
> HYPERLINK 
> "http://cr.openjdk.java.net/%7Ecsahu/8151509/webrev.01/"http://cr.open
> jdk.java.net/~csahu/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; HYPERLINK 
> "mailto:serviceability-dev@openjdk.java.net"serviceability-dev@openjdk
> .java.net; HYPERLINK 
> "mailto:hotspot-runtime-...@openjdk.java.net"hotspot-runtime-dev@openj
> dk.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  "mailto:thomas.stu...@gmail.com"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  "mailto:dmitry.dmitr...@oracle.com"dmitry.dmit

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

2016-03-19 Thread Cheleswer Sahu

Thanks Dmitry for review. I will file CR and let you know once done.


Cheleswer


-Original Message-
From: Dmitry Samersoff 
Sent: Friday, March 18, 2016 2:35 PM
To: Cheleswer Sahu; hotspot-runtime-...@openjdk.java.net; 
serviceability-dev@openjdk.java.net
Subject: Re: RFR[9u-dev]: 8151442: jstack doesn't close quotation marks 
properly with threads' name greater than 1996 characters

Cheleswer,

Fix (as immediate solution) looks good for me.

But IMHO, silent truncation of the output inside output stream is not a correct 
behavior. So please file a follow-up CR to have it addressed.

-Dmitry

On 2016-03-18 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
> 
>  
> 
>  
> 
>  
> 


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

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

2016-03-19 Thread Cheleswer Sahu

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]: 8151442: jstack doesn't close quotation marks properly with threads' name greater than 1996 characters

2016-03-19 Thread Cheleswer Sahu

Hi David,

-Original Message-
From: David Holmes 
Sent: Friday, March 18, 2016 2:42 PM
To: Cheleswer Sahu; hotspot-runtime-...@openjdk.java.net; 
serviceability-dev@openjdk.java.net
Subject: Re: RFR[9u-dev]: 8151442: jstack doesn't close quotation marks 
properly with threads' name greater than 1996 characters

On 18/03/2016 5:54 PM, 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.

Anyone giving a thread a name that long deserves to get a core dump! ;-)

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

Can't we just limit the number of characters read by %s?
 
Yes we can do it, but just one thing which I think of is, if the truncation of 
output inside  output stream issue get resolves as Dmritry suggested or 
O_BUFFLEN size gets increased in any  future fix then we have to again make 
changes in this code, as limiting the no. of character read by %s will give 
truncated output . In such case present fix will have no effect.
 
David

> Regards,
>
> Cheleswer
>

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

2016-03-31 Thread Cheleswer Sahu

Hi ,

I would like to go with the "print_raw()" option as this can print any length 
of thread name. I have modified the code and written a test case also for this 
bug. Please review the code changes from the below link 

http://cr.openjdk.java.net/~csahu/8151442/webrev.01/ 

Regards,
Cheleswer

-Original Message-
From: Mattis Castegren 
Sent: Wednesday, March 30, 2016 10:42 PM
To: Kevin Walls; David Holmes; Daniel Daugherty; Dmitry Samersoff; Cheleswer 
Sahu; hotspot-runtime-...@openjdk.java.net; serviceability-dev@openjdk.java.net
Cc: Mattis Castegren
Subject: RE: RFR[9u-dev]: 8151442: jstack doesn't close quotation marks 
properly with threads' name greater than 1996 characters

Hi

It seems there are two approaches here, either we truncate long thread names, 
or we make sure to print the full thread name.

I agree with Dmitry and Kirk that if the API allows these long names, the 
tooling should do the right thing (even though one has to wonder what these 
long names are).

I suggest we move ahead with the print_raw approach.

If we believe there should be a limit in the Thread name lenghts, I suggest we 
file a new bug for that.

Kind Regards
/Mattis

-Original Message-
From: Kevin Walls 
Sent: den 24 mars 2016 21:06
To: David Holmes; Daniel Daugherty; Dmitry Samersoff; Cheleswer Sahu; 
hotspot-runtime-...@openjdk.java.net; serviceability-dev@openjdk.java.net
Subject: Re: RFR[9u-dev]: 8151442: jstack doesn't close quotation marks 
properly with threads' name greater than 1996 characters

Hi

I didn't think of %.s when Cheleswer and I discussed this briefly.  
I'd like to have suggested that, with the idea that the 2k long thread name is 
extreme, and it's so important that we preserve the format of the output, and 
keep that closing quote, even if we lost some of the thread name.  We currently 
and probably always have truncated such names, the problem that triggered this 
was mainly that the format was broken.

As there are several places we pass the name to the stream and could hit the 
length limit, should we have a THREAD_NAME_FORMAT defined for such use instead 
of using %s though the actual length can't be 1996, it's BUFLEN minus whatever 
else we expect to be printed in the same print call.  We might guess as low as 
1024?

(Retaining one st->print() also minimises any risk of concurrent prints 
jumbling up the output.)

Thanks
Kevin

On 21/03/2016 21:24, David Holmes wrote:
> On 22/03/2016 2:31 AM, Daniel D. Daugherty wrote:
>> On 3/21/16 2:39 AM, Dmitry Samersoff wrote:
>>> David,
>>>
>>>> I still see %.Ns as the simplest solution - but whatever.
>>> It spreads artificial limitation of thread name length across 
>>> hotspot sources.
>>>
>>> Brief grepping[1] shows couple of other places with the same problem 
>>> and if some days we decide to change default O_BUFLEN, we have to 
>>> not forget to change .N value in couple of places as well.
>>
>> There should be a way to pass the precision value as a parameter 
>> instead of hardcoding it in the format string. Something like:
>>
>>  sprintf("%.*s", precision_value, string);
>
> Yes the length limit can be passed as a variable. But I think Dmitry 
> just wants to allow for unlimited lengths. Not sure how to achieve 
> that at the lowest level though as we need to avoid complex 
> allocations etc in these print routines.
>
> David
>
>
>> Dan
>>
>>>
>>> 1.
>>> ./share/vm/prims/jni.cpp
>>> 673: "in thread \"%s\" ", thread->get_thread_name());
>>>
>>> ./share/vm/runtime/thread.cpp
>>> 1766: get_thread_profiler()->print(get_thread_name());
>>> 1974: get_thread_profiler()->print(get_thread_name());
>>> 2896: st->print("\"%s\" ", get_thread_name());
>>> 2926: st->print("%s", get_thread_name_string(buf, buflen));
>>> 2932: st->print("JavaThread \"%s\"", get_thread_name_string(buf, 
>>> buflen));
>>>
>>>
>>> ./share/vm/services/threadService.cpp
>>> 882: ... st->print_cr("\"%s\":", currentThread->get_thread_name());
>>> 919: ..."%s \"%s\"", owner_desc, currentThread->get_thread_name());
>>> 932: ... st->print_cr("\"%s\":", currentThread->get_thread_name());
>>>
>>> -Dmitry
>>>
>>>
>>> On 2016-03-19 00:37, David Holmes wrote:
>>>>
>>>> On 18/03/2016 11:28 PM, Dmitry Samersoff wrote:
>>>>> David,
>>>>>
>>>>>> Ignoring Dmitry's issue it still seems simpler/cleaner

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

2016-03-31 Thread Cheleswer Sahu

Thanks Dmitry, Kevin for review.

Regards,
Cheleswer

-Original Message-
From: Kevin Walls 
Sent: Friday, April 01, 2016 1:02 AM
To: Dmitry Samersoff; Cheleswer Sahu; Mattis Castegren; David Holmes; Daniel 
Daugherty; hotspot-runtime-...@openjdk.java.net; 
serviceability-dev@openjdk.java.net
Subject: Re: RFR[9u-dev]: 8151442: jstack doesn't close quotation marks 
properly with threads' name greater than 1996 characters

Yes, looks good. 8-)


On 31/03/2016 19:54, Dmitry Samersoff wrote:
> Cheleswer,
>
> Looks good for me! (R)
>
> -Dmitry
>
>
> On 2016-03-31 12:46, Cheleswer Sahu wrote:
>> Hi ,
>>
>> I would like to go with the "print_raw()" option as this can print 
>> any length of thread name. I have modified the code and written a 
>> test case also for this bug. Please review the code changes from the 
>> below link
>>
>> http://cr.openjdk.java.net/~csahu/8151442/webrev.01/
>>
>> Regards,
>> Cheleswer
>>
>> -Original Message-
>> From: Mattis Castegren
>> Sent: Wednesday, March 30, 2016 10:42 PM
>> To: Kevin Walls; David Holmes; Daniel Daugherty; Dmitry Samersoff; 
>> Cheleswer Sahu; hotspot-runtime-...@openjdk.java.net; 
>> serviceability-dev@openjdk.java.net
>> Cc: Mattis Castegren
>> Subject: RE: RFR[9u-dev]: 8151442: jstack doesn't close quotation 
>> marks properly with threads' name greater than 1996 characters
>>
>> Hi
>>
>> It seems there are two approaches here, either we truncate long thread 
>> names, or we make sure to print the full thread name.
>>
>> I agree with Dmitry and Kirk that if the API allows these long names, the 
>> tooling should do the right thing (even though one has to wonder what these 
>> long names are).
>>
>> I suggest we move ahead with the print_raw approach.
>>
>> If we believe there should be a limit in the Thread name lenghts, I suggest 
>> we file a new bug for that.
>>
>> Kind Regards
>> /Mattis
>>
>> -Original Message-
>> From: Kevin Walls
>> Sent: den 24 mars 2016 21:06
>> To: David Holmes; Daniel Daugherty; Dmitry Samersoff; Cheleswer Sahu; 
>> hotspot-runtime-...@openjdk.java.net; 
>> serviceability-dev@openjdk.java.net
>> Subject: Re: RFR[9u-dev]: 8151442: jstack doesn't close quotation 
>> marks properly with threads' name greater than 1996 characters
>>
>>
>> Hi
>>
>> I didn't think of %.s when Cheleswer and I discussed this briefly.
>> I'd like to have suggested that, with the idea that the 2k long thread name 
>> is extreme, and it's so important that we preserve the format of the output, 
>> and keep that closing quote, even if we lost some of the thread name.  We 
>> currently and probably always have truncated such names, the problem that 
>> triggered this was mainly that the format was broken.
>>
>> As there are several places we pass the name to the stream and could hit the 
>> length limit, should we have a THREAD_NAME_FORMAT defined for such use 
>> instead of using %s though the actual length can't be 1996, it's BUFLEN 
>> minus whatever else we expect to be printed in the same print call.  We 
>> might guess as low as 1024?
>>
>> (Retaining one st->print() also minimises any risk of concurrent 
>> prints jumbling up the output.)
>>
>> Thanks
>> Kevin
>>
>>
>> On 21/03/2016 21:24, David Holmes wrote:
>>> On 22/03/2016 2:31 AM, Daniel D. Daugherty wrote:
>>>> On 3/21/16 2:39 AM, Dmitry Samersoff wrote:
>>>>> David,
>>>>>
>>>>>> I still see %.Ns as the simplest solution - but whatever.
>>>>> It spreads artificial limitation of thread name length across 
>>>>> hotspot sources.
>>>>>
>>>>> Brief grepping[1] shows couple of other places with the same 
>>>>> problem and if some days we decide to change default O_BUFLEN, we 
>>>>> have to not forget to change .N value in couple of places as well.
>>>> There should be a way to pass the precision value as a parameter 
>>>> instead of hardcoding it in the format string. Something like:
>>>>
>>>>   sprintf("%.*s", precision_value, string);
>>> Yes the length limit can be passed as a variable. But I think Dmitry 
>>> just wants to allow for unlimited lengths. Not sure how to achieve 
>>> that at the lowest level though as we need to avoid complex 
>>> allocations etc in these print routines.
>>

RFR[9u-dev]: 8054326: Confusing message in "Current rem set statistics"

2016-04-01 Thread Cheleswer Sahu

Hi,

 

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

 

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

 

Bug Brief: In output of remset statistics "Max"  is printing as 0k, which is 
confusing for user.

 

Problem Identified : "Max" value is rounded to KB, which result  in 0k, if the 
value is less than 1024B.

 

Solution Proposed:  If the value for "Max" is less than 1 KB (1024 B),  print 
the value in bytes (i.e. 1023B.) else

print the value in KB.

 

 

 

Regards,

Cheleswer

RFR[9u-dev]: 8153319: new test serviceability/tmtools/jstack/JstackThreadTest.java fails

2016-04-05 Thread Cheleswer Sahu

Hi,

 

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

 

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

 

 

Bug Brief: Test is failing on some platforms.

 

Problem Identified:  Newly created child thread (NamedThread) is finished its 
execution before main thread calls "jstack", which result in test failure.

 

Solution Proposed:  Set the child thread in sleep state for forever and make 
sure that "jstack " tool always gets executed after " NamedThread"  is started. 

 

 

Regards,

Cheleswer

RE: RFR[9u-dev]: 8054326: Confusing message in "Current rem set statistics"

2016-04-05 Thread Cheleswer Sahu

Hi,

 

This fix is waiting for review, can somebody please review this change.

 

Regards,

Cheleswer

 

From: Cheleswer Sahu 
Sent: Friday, April 01, 2016 6:01 PM
To: hotspot-runtime-...@openjdk.java.net; serviceability-dev@openjdk.java.net
Cc: Mattis Castegren
Subject: RFR[9u-dev]: 8054326: Confusing message in "Current rem set statistics"

 

Hi,

 

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

 

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

 

Bug Brief: In output of remset statistics "Max"  is printing as 0k, which is 
confusing for user.

 

Problem Identified : "Max" value is rounded to KB, which result  in 0k, if the 
value is less than 1024B.

 

Solution Proposed:  If the value for "Max" is less than 1 KB (1024 B),  print 
the value in bytes (i.e. 1023B.) else

print the value in KB.

 

 

 

Regards,

Cheleswer

RE: RFR[9u-dev]: 8153319: new test serviceability/tmtools/jstack/JstackThreadTest.java fails

2016-04-07 Thread Cheleswer Sahu

Hi ,
Thanks for your review and suggestion. I agree that sleep is not the best and 
reliable way to achieve the objective of test case. I also found the idea of 
using  j.u.c.CountDownLatch very easy and effective. I have made some changes 
in the code. Please review the code changes in the below link

http://cr.openjdk.java.net/~csahu/8153319/webrev.01/

Regards,
Cheleswer
-Original Message-
From: Leonid Mesnik 
Sent: Tuesday, April 05, 2016 7:00 PM
To: Cheleswer Sahu; hotspot-runtime-...@openjdk.java.net; 
serviceability-dev@openjdk.java.net
Subject: Re: RFR[9u-dev]: 8153319: new test 
serviceability/tmtools/jstack/JstackThreadTest.java fails

Hi

I don't think that sleep is a good way to ensure that thread is started. 
It is not reliable on the slow host / VM under stress and just waste of time on 
fast host.
Is it possible just to add any explicit synchronization to ensure that 
NamedThread is started?

Leonid

On 05.04.2016 13:23, Cheleswer Sahu wrote:
> Hi,
>
>   
>
> Please review the code changes for 
> https://bugs.openjdk.java.net/browse/JDK-8153319.
>
>   
>
> Webrev link: http://cr.openjdk.java.net/~csahu/8153319/
>
>   
>
>   
>
> Bug Brief: Test is failing on some platforms.
>
>   
>
> Problem Identified:  Newly created child thread (NamedThread) is finished its 
> execution before main thread calls "jstack", which result in test failure.
>
>   
>
> Solution Proposed:  Set the child thread in sleep state for forever and make 
> sure that "jstack " tool always gets executed after " NamedThread"  is 
> started.
>
>   
>
>   
>
> Regards,
>
> Cheleswer
>
>   
>
>

RE: RFR[9u-dev]: 8153319: new test serviceability/tmtools/jstack/JstackThreadTest.java fails

2016-04-07 Thread Cheleswer Sahu

Thanks Dmitry and Leonid for review. Thanks Dan for information, I will do the 
needful before pushing this fix.


Regards,
Cheleswer

-Original Message-
From: Daniel D. Daugherty 
Sent: Thursday, April 07, 2016 7:52 PM
To: Dmitry Samersoff; Cheleswer Sahu; Leonid Mesnik; 
hotspot-runtime-...@openjdk.java.net; serviceability-dev@openjdk.java.net; 
Filipp Zhinkin
Subject: Re: RFR[9u-dev]: 8153319: new test 
serviceability/tmtools/jstack/JstackThreadTest.java fails

Cheleswer,

When you re-sync with the current JDK9-hs-rt, you'll have to re-enable this 
test. It was quarantined yesterday with this fix:

JDK-8153671 Quarantine serviceability/tmtools/jstack/JstackThreadTest.java
 until JDK-8153319 is fixed
https://bugs.openjdk.java.net/browse/JDK-8153671

You just need to remove the @ignore line.

Dan



On 4/7/16 8:12 AM, Dmitry Samersoff wrote:
> Cheleswer,
>
> Looks good for me. Reviewed.
>
> -Dmitry
>
> On 2016-04-07 16:50, Cheleswer Sahu wrote:
>> Hi ,
>> Thanks for your review and suggestion. I agree that sleep is not the 
>> best and reliable way to achieve the objective of test case. I also 
>> found the idea of using  j.u.c.CountDownLatch very easy and 
>> effective. I have made some changes in the code. Please review the 
>> code changes in the below link
>>
>> http://cr.openjdk.java.net/~csahu/8153319/webrev.01/
>>   
>>
>> Regards,
>> Cheleswer
>> -----Original Message-
>> From: Leonid Mesnik
>> Sent: Tuesday, April 05, 2016 7:00 PM
>> To: Cheleswer Sahu; hotspot-runtime-...@openjdk.java.net; 
>> serviceability-dev@openjdk.java.net
>> Subject: Re: RFR[9u-dev]: 8153319: new test 
>> serviceability/tmtools/jstack/JstackThreadTest.java fails
>>
>> Hi
>>
>> I don't think that sleep is a good way to ensure that thread is started.
>> It is not reliable on the slow host / VM under stress and just waste of time 
>> on fast host.
>> Is it possible just to add any explicit synchronization to ensure that 
>> NamedThread is started?
>>
>> Leonid
>>
>> On 05.04.2016 13:23, Cheleswer Sahu wrote:
>>> Hi,
>>>
>>>
>>>
>>> Please review the code changes for 
>>> https://bugs.openjdk.java.net/browse/JDK-8153319.
>>>
>>>
>>>
>>> Webrev link: http://cr.openjdk.java.net/~csahu/8153319/
>>>
>>>
>>>
>>>
>>>
>>> Bug Brief: Test is failing on some platforms.
>>>
>>>
>>>
>>> Problem Identified:  Newly created child thread (NamedThread) is finished 
>>> its execution before main thread calls "jstack", which result in test 
>>> failure.
>>>
>>>
>>>
>>> Solution Proposed:  Set the child thread in sleep state for forever and 
>>> make sure that "jstack " tool always gets executed after " NamedThread"  is 
>>> started.
>>>
>>>
>>>
>>>
>>>
>>> Regards,
>>>
>>> Cheleswer
>>>
>>>
>>>
>>>
>

RFR[9u-dev]: 8150900: Implement diagnostic_pd

2016-05-02 Thread Cheleswer Sahu

Hi,

 

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

 

Webrev Link: http://cr.openjdk.java.net/~csahu/8150900/webrev.00/

 

Enhancement Brief:  A new variant of flag "diagnostic_pd" is implemented. All 
flags which are diagnostic in nature and platform dependent can be placed

under this variant. These flags can be enable using  
"-XX:+UnlockDiagnosticVMOptions".

At present I have placed 4 flags under "diagnostic_pd" 

1.1. InitArrayShortSize

2.2. ImplicitNullChecks

3.3. InlineFrequencyCount

4.4. PostLoopMultiversioning

 

 

Regards,

Cheleswer

RE: RFR[9u-dev]: 8150900: Implement diagnostic_pd

2016-05-02 Thread Cheleswer Sahu

Hi Gerard,


> -Original Message-
> From: Gerard Ziemski
> Sent: Monday, May 02, 2016 9:07 PM
> To: Cheleswer Sahu
> Cc: hotspot-runtime-...@openjdk.java.net; serviceability-
> d...@openjdk.java.net
> Subject: Re: RFR[9u-dev]: 8150900: Implement diagnostic_pd
> 
> hi Cheleswer,
> 
> 
> #1 Shouldn’t the following files be modified as well? :
> 
> open:
> 
> src/cpu/sparc/vm/globals_sparc.hpp
> src/cpu/x86/vm/globals_x86.hpp
> src/cpu/zero/vm/globals_zero.hpp
> 
> closed:
> 
> cpu/arm/vm/globals_arm.hpp

I have implemented  "diagnostic_pd" using "product_pd" as a reference 
implementation. "product_pd" is not implemented for " ARCH_FLAGS ",  therefore 
I have also not implemented "diagnostic_pd" for "ARCH_FLAGS"  type.

> share/vm/runtime/globals_ext.hpp
> share/vm/runtime/os_ext.hpp

These 2 files are under closed repository, so I have initiated a separate 
internal review request for those changes.

> 
> 
> #2 Bunch of header files need to be updated with 2016 for Copyright:
> 
> /*
> - * Copyright (c) 2011, 2015, Oracle and/or its affiliates. All rights 
> reserved.
> + * Copyright (c) 2011, 2016, Oracle and/or its affiliates. All rights 
> reserved.
>  * ORACLE PROPRIETARY/CONFIDENTIAL.  Use is subject to license terms.
>  */
> 
> 
I agree, I will update the copyright headers.

> #3 What tests have you run? Did you do:
> 
> a) JPRT hotspot
> b) RBT hs-nightly-runtime
>
I have run JPRT hostspot tests for this. It shows no error.
 
> Please email me if you need help with those.
> 
> 
> #4 Just heads up that I will be shortly asking for review of my fix
> (https://bugs.openjdk.java.net/browse/JDK-8073500), which touches many
> of the same file, so one of us will have a tricky merge
> 
Thanks for informing about this. 


Regards,
Cheleswer 

> 
> cheers
> 
> > On May 2, 2016, at 4:51 AM, Cheleswer Sahu
>  wrote:
> >
> > Hi,
> >
> >
> >
> > Please review the code changes for
> https://bugs.openjdk.java.net/browse/JDK-8150900.
> >
> >
> >
> > Webrev Link: http://cr.openjdk.java.net/~csahu/8150900/webrev.00/
> >
> >
> >
> > Enhancement Brief:  A new variant of flag "diagnostic_pd" is implemented.
> All flags which are diagnostic in nature and platform dependent can be placed
> >
> > under this variant. These flags can be enable using  "-
> XX:+UnlockDiagnosticVMOptions".
> >
> > At present I have placed 4 flags under "diagnostic_pd"
> >
> > 1.1. InitArrayShortSize
> >
> > 2.2. ImplicitNullChecks
> >
> > 3.3. InlineFrequencyCount
> >
> > 4.4. PostLoopMultiversioning
> >
> >
> >
> >
> >
> > Regards,
> >
> > Cheleswer
>

RE: RFR[9u-dev]: 8150900: Implement diagnostic_pd

2016-05-10 Thread Cheleswer Sahu

Hi, 
I need one reviewer (R) to review these changes before pushing in JDK9.  Can 
somebody please review the changes.

Regards,
Cheleswer

> -Original Message-
> From: Kevin Walls
> Sent: Friday, May 06, 2016 3:53 PM
> To: Cheleswer Sahu; Gerard Ziemski
> Cc: serviceability-dev@openjdk.java.net; hotspot-runtime-
> d...@openjdk.java.net
> Subject: Re: RFR[9u-dev]: 8150900: Implement diagnostic_pd
> 
> 
> Thanks Cheleswer, looks good to me too, have been over the macros as
> much as I can!
> 
> Thanks
> Kevin
> 
> 
> 
> On 03/05/2016 07:34, Cheleswer Sahu wrote:
> > Hi Gerard,
> >
> >
> >> -Original Message-
> >> From: Gerard Ziemski
> >> Sent: Monday, May 02, 2016 9:07 PM
> >> To: Cheleswer Sahu
> >> Cc: hotspot-runtime-...@openjdk.java.net; serviceability-
> >> d...@openjdk.java.net
> >> Subject: Re: RFR[9u-dev]: 8150900: Implement diagnostic_pd
> >>
> >> hi Cheleswer,
> >>
> >>
> >> #1 Shouldn’t the following files be modified as well? :
> >>
> >> open:
> >>
> >> src/cpu/sparc/vm/globals_sparc.hpp
> >> src/cpu/x86/vm/globals_x86.hpp
> >> src/cpu/zero/vm/globals_zero.hpp
> >>
> >> closed:
> >>
> >> cpu/arm/vm/globals_arm.hpp
> > I have implemented  "diagnostic_pd" using "product_pd" as a reference
> implementation. "product_pd" is not implemented for " ARCH_FLAGS ",
> therefore I have also not implemented "diagnostic_pd" for "ARCH_FLAGS"
> type.
> >
> >> share/vm/runtime/globals_ext.hpp
> >> share/vm/runtime/os_ext.hpp
> > These 2 files are under closed repository, so I have initiated a separate
> internal review request for those changes.
> >
> >>
> >> #2 Bunch of header files need to be updated with 2016 for Copyright:
> >>
> >> /*
> >> - * Copyright (c) 2011, 2015, Oracle and/or its affiliates. All rights 
> >> reserved.
> >> + * Copyright (c) 2011, 2016, Oracle and/or its affiliates. All rights 
> >> reserved.
> >>   * ORACLE PROPRIETARY/CONFIDENTIAL.  Use is subject to license terms.
> >>   */
> >>
> >>
> > I agree, I will update the copyright headers.
> >
> >> #3 What tests have you run? Did you do:
> >>
> >> a) JPRT hotspot
> >> b) RBT hs-nightly-runtime
> >>
> > I have run JPRT hostspot tests for this. It shows no error.
> >
> >> Please email me if you need help with those.
> >>
> >>
> >> #4 Just heads up that I will be shortly asking for review of my fix
> >> (https://bugs.openjdk.java.net/browse/JDK-8073500), which touches
> >> many of the same file, so one of us will have a tricky merge
> >>
> > Thanks for informing about this.
> >
> >
> > Regards,
> > Cheleswer
> >
> >> cheers
> >>
> >>> On May 2, 2016, at 4:51 AM, Cheleswer Sahu
> >>  wrote:
> >>> Hi,
> >>>
> >>>
> >>>
> >>> Please review the code changes for
> >> https://bugs.openjdk.java.net/browse/JDK-8150900.
> >>>
> >>>
> >>> Webrev Link: http://cr.openjdk.java.net/~csahu/8150900/webrev.00/
> >>>
> >>>
> >>>
> >>> Enhancement Brief:  A new variant of flag "diagnostic_pd" is
> implemented.
> >> All flags which are diagnostic in nature and platform dependent can
> >> be placed
> >>> under this variant. These flags can be enable using  "-
> >> XX:+UnlockDiagnosticVMOptions".
> >>> At present I have placed 4 flags under "diagnostic_pd"
> >>>
> >>> 1.1. InitArrayShortSize
> >>>
> >>> 2.2. ImplicitNullChecks
> >>>
> >>> 3.3. InlineFrequencyCount
> >>>
> >>> 4.4. PostLoopMultiversioning
> >>>
> >>>
> >>>
> >>>
> >>>
> >>> Regards,
> >>>
> >>> Cheleswer
>

RE: RFR[9u-dev]: 8150900: Implement diagnostic_pd

2016-05-11 Thread Cheleswer Sahu

Thanks Christian for review. I will correct the alignment.

 

Regards,

Cheleswer

 

From: Christian Thalinger 
Sent: Wednesday, May 11, 2016 1:00 AM
To: Cheleswer Sahu
Cc: Kevin Walls; Gerard Ziemski; serviceability-dev@openjdk.java.net; 
hotspot-runtime-...@openjdk.java.net
Subject: Re: RFR[9u-dev]: 8150900: Implement diagnostic_pd

 


src/share/vm/runtime/globals.hpp

-  develop_pd(bool, ImplicitNullChecks,  \
+  diagnostic_pd(bool, ImplicitNullChecks,  
\
   "Generate code for implicit null checks") \

Align the \

 

On May 10, 2016, at 1:47 AM, Cheleswer Sahu mailto:cheleswer.s...@oracle.com"cheleswer.s...@oracle.com> wrote:

 

Hi, 
I need one reviewer (R) to review these changes before pushing in JDK9.  Can 
somebody please review the changes.

Regards,
Cheleswer




-Original Message-
From: Kevin Walls
Sent: Friday, May 06, 2016 3:53 PM
To: Cheleswer Sahu; Gerard Ziemski
Cc: HYPERLINK 
"mailto:serviceability-dev@openjdk.java.net"serviceability-dev@openjdk.java.net;
 hotspot-runtime-
HYPERLINK "mailto:d...@openjdk.java.net"d...@openjdk.java.net
Subject: Re: RFR[9u-dev]: 8150900: Implement diagnostic_pd


Thanks Cheleswer, looks good to me too, have been over the macros as
much as I can!

Thanks
Kevin



On 03/05/2016 07:34, Cheleswer Sahu wrote:



Hi Gerard,





-Original Message-
From: Gerard Ziemski
Sent: Monday, May 02, 2016 9:07 PM
To: Cheleswer Sahu
Cc: HYPERLINK 
"mailto:hotspot-runtime-...@openjdk.java.net"hotspot-runtime-...@openjdk.java.net;
 serviceability-
HYPERLINK "mailto:d...@openjdk.java.net"d...@openjdk.java.net
Subject: Re: RFR[9u-dev]: 8150900: Implement diagnostic_pd

hi Cheleswer,


#1 Shouldn’t the following files be modified as well? :

open:

src/cpu/sparc/vm/globals_sparc.hpp
src/cpu/x86/vm/globals_x86.hpp
src/cpu/zero/vm/globals_zero.hpp

closed:

cpu/arm/vm/globals_arm.hpp

I have implemented  "diagnostic_pd" using "product_pd" as a reference

implementation. "product_pd" is not implemented for " ARCH_FLAGS ",
therefore I have also not implemented "diagnostic_pd" for "ARCH_FLAGS"
type.







share/vm/runtime/globals_ext.hpp
share/vm/runtime/os_ext.hpp

These 2 files are under closed repository, so I have initiated a separate

internal review request for those changes.








#2 Bunch of header files need to be updated with 2016 for Copyright:

/*
- * Copyright (c) 2011, 2015, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2011, 2016, Oracle and/or its affiliates. All rights reserved.
 * ORACLE PROPRIETARY/CONFIDENTIAL.  Use is subject to license terms.
 */



I agree, I will update the copyright headers.




#3 What tests have you run? Did you do:

a) JPRT hotspot
b) RBT hs-nightly-runtime

I have run JPRT hostspot tests for this. It shows no error.




Please email me if you need help with those.


#4 Just heads up that I will be shortly asking for review of my fix
(https://bugs.openjdk.java.net/browse/JDK-8073500), which touches
many of the same file, so one of us will have a tricky merge

Thanks for informing about this.


Regards,
Cheleswer




cheers




On May 2, 2016, at 4:51 AM, Cheleswer Sahu

mailto:cheleswer.s...@oracle.com"cheleswer.s...@oracle.com> wrote:



Hi,



Please review the code changes for

https://bugs.openjdk.java.net/browse/JDK-8150900.





Webrev Link: http://cr.openjdk.java.net/~csahu/8150900/webrev.00/



Enhancement Brief:  A new variant of flag "diagnostic_pd" is

implemented.



All flags which are diagnostic in nature and platform dependent can
be placed



under this variant. These flags can be enable using  "-

XX:+UnlockDiagnosticVMOptions".



At present I have placed 4 flags under "diagnostic_pd"

1.    1. InitArrayShortSize

2.    2. ImplicitNullChecks

3.    3. InlineFrequencyCount

4.    4. PostLoopMultiversioning





Regards,

Cheleswer

RE: RFR[9u-dev]: 8150900: Implement diagnostic_pd

2016-05-24 Thread Cheleswer Sahu

Hi,

 

I just wanted to let you know that since review there has been one new file 
added  “commandLineFlagWriteableList.cpp”,  and this files also needs to be 
modified/updated for implementing “diagnostic_pd”. This is just one additional 
change over what was reviewed before,  so I am  going ahead with this fix and 
need not a new review. I have tested this change and its working fine as before.

 

Webrev link: http://cr.openjdk.java.net/~csahu/8150900/webrev.01/

 

Regards, 

Cheleswer

 

From: Cheleswer Sahu 
Sent: Wednesday, May 11, 2016 2:29 PM
To: Christian Thalinger
Cc: Kevin Walls; Gerard Ziemski; serviceability-dev@openjdk.java.net; 
hotspot-runtime-...@openjdk.java.net
Subject: RE: RFR[9u-dev]: 8150900: Implement diagnostic_pd

 

Thanks Christian for review. I will correct the alignment.

 

Regards,

Cheleswer

 

From: Christian Thalinger 
Sent: Wednesday, May 11, 2016 1:00 AM
To: Cheleswer Sahu
Cc: Kevin Walls; Gerard Ziemski; HYPERLINK 
"mailto:serviceability-dev@openjdk.java.net"serviceability-dev@openjdk.java.net;
 HYPERLINK 
"mailto:hotspot-runtime-...@openjdk.java.net"hotspot-runtime-...@openjdk.java.net
Subject: Re: RFR[9u-dev]: 8150900: Implement diagnostic_pd

 


src/share/vm/runtime/globals.hpp

-  develop_pd(bool, ImplicitNullChecks,  \
+  diagnostic_pd(bool, ImplicitNullChecks,  
\
   "Generate code for implicit null checks") \

Align the \

 

On May 10, 2016, at 1:47 AM, Cheleswer Sahu mailto:cheleswer.s...@oracle.com"cheleswer.s...@oracle.com> wrote:

 

Hi, 
I need one reviewer (R) to review these changes before pushing in JDK9.  Can 
somebody please review the changes.

Regards,
Cheleswer



-Original Message-
From: Kevin Walls
Sent: Friday, May 06, 2016 3:53 PM
To: Cheleswer Sahu; Gerard Ziemski
Cc: HYPERLINK 
"mailto:serviceability-dev@openjdk.java.net"serviceability-dev@openjdk.java.net;
 hotspot-runtime-
HYPERLINK "mailto:d...@openjdk.java.net"d...@openjdk.java.net
Subject: Re: RFR[9u-dev]: 8150900: Implement diagnostic_pd


Thanks Cheleswer, looks good to me too, have been over the macros as
much as I can!

Thanks
Kevin



On 03/05/2016 07:34, Cheleswer Sahu wrote:

Hi Gerard,




-Original Message-
From: Gerard Ziemski
Sent: Monday, May 02, 2016 9:07 PM
To: Cheleswer Sahu
Cc: HYPERLINK 
"mailto:hotspot-runtime-...@openjdk.java.net"hotspot-runtime-...@openjdk.java.net;
 serviceability-
HYPERLINK "mailto:d...@openjdk.java.net"d...@openjdk.java.net
Subject: Re: RFR[9u-dev]: 8150900: Implement diagnostic_pd

hi Cheleswer,


#1 Shouldn’t the following files be modified as well? :

open:

src/cpu/sparc/vm/globals_sparc.hpp
src/cpu/x86/vm/globals_x86.hpp
src/cpu/zero/vm/globals_zero.hpp

closed:

cpu/arm/vm/globals_arm.hpp

I have implemented  "diagnostic_pd" using "product_pd" as a reference

implementation. "product_pd" is not implemented for " ARCH_FLAGS ",
therefore I have also not implemented "diagnostic_pd" for "ARCH_FLAGS"
type.

 

share/vm/runtime/globals_ext.hpp
share/vm/runtime/os_ext.hpp

These 2 files are under closed repository, so I have initiated a separate

internal review request for those changes.

 


#2 Bunch of header files need to be updated with 2016 for Copyright:

/*
- * Copyright (c) 2011, 2015, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2011, 2016, Oracle and/or its affiliates. All rights reserved.
 * ORACLE PROPRIETARY/CONFIDENTIAL.  Use is subject to license terms.
 */

I agree, I will update the copyright headers.



#3 What tests have you run? Did you do:

a) JPRT hotspot
b) RBT hs-nightly-runtime

I have run JPRT hostspot tests for this. It shows no error.



Please email me if you need help with those.


#4 Just heads up that I will be shortly asking for review of my fix
(https://bugs.openjdk.java.net/browse/JDK-8073500), which touches
many of the same file, so one of us will have a tricky merge

Thanks for informing about this.


Regards,
Cheleswer



cheers



On May 2, 2016, at 4:51 AM, Cheleswer Sahu

mailto:cheleswer.s...@oracle.com"cheleswer.s...@oracle.com> wrote:

Hi,



Please review the code changes for

https://bugs.openjdk.java.net/browse/JDK-8150900.



Webrev Link: http://cr.openjdk.java.net/~csahu/8150900/webrev.00/



Enhancement Brief:  A new variant of flag "diagnostic_pd" is

implemented.

All flags which are diagnostic in nature and platform dependent can
be placed

under this variant. These flags can be enable using  "-

XX:+UnlockDiagnosticVMOptions".

At present I have placed 4 flags under "diagnostic_pd"

1.    1. InitArrayShortSize

2.    2. ImplicitNullChecks

3.    3. InlineFrequencyCount

4.    4. PostLoopMultiversioning





Regards,

Cheleswer

RFR[ 9u-dev] JDK-8072863 - Replace fatal() with vm_exit_during_initialization() when an incorrect class is found on the bootclasspath

2015-03-27 Thread cheleswer sahu


Hi,
Please review the code changes for 
https://bugs.openjdk.java.net/browse/JDK-8072863. I have built the JDK9 with 
fix successfully. As I do not have account for OpenJDK, David Buck will push
the fix into jdk9/hs-rt/.

Web review link: http://cr.openjdk.java.net/~dbuck/8072863/webrev.00/

Regards,
Cheleswer

RFR[ 9u-dev] JDK-8072863 - Replace fatal() with vm_exit_during_initialization() when an incorrect class is found on the bootclasspath

2015-04-02 Thread cheleswer sahu


Hi,

Please review the new code changes. I have incorporated the suggestion 
received from 'David Holmes'  in code and built successfully. As I do 
not have account for OpenJDK, David Buck will push the fix into jdk9/hs-rt/.

web review link: http://cr.openjdk.java.net/~dbuck/8072863/webrev.02/

Regards,
Cheleswer

On 3/30/2015 2:14 AM, David Holmes wrote:

On 27/03/2015 5:36 PM, cheleswer sahu wrote:

Hi,
Please review the code changes for
https://bugs.openjdk.java.net/browse/JDK-8072863. I have built the JDK9
with fix successfully. As I do not have account for OpenJDK, David Buck
will push
the fix into jdk9/hs-rt/.

Web review link: http://cr.openjdk.java.net/~dbuck/8072863/webrev.00/


Is it possible to include information about where the class was loaded 
from in the message that precedes the exit:


 121 tty->print_cr("Invalid layout of %s at %s", 
ik->external_name(), name_symbol->as_C_string());


The main cause of this problem is android.jar on the bootclasspath. It 
would be good to make that explicit as well. Otherwise we will still 
get bug reports because noone will know what this error means.


Thanks,
David



Regards,
Cheleswer

[8u60] request for approval: 8072863: Replace fatal() with vm_exit_during_initialization() when an incorrect class is found on the bootclasspath

2015-04-27 Thread cheleswer sahu


Hi!

May I please have approval to backport this fix from JDK9 to JDK8. I have built 
the JDK-8 hotspot and tested already. JDK9 fix applies cleanly to JDK8. As I do 
not have an account for OPENJDK, David Buck will push the fix into 
jdk8u/hs-dev/hotspot.

BUGURL: https://bugs.openjdk.java.net/browse/JDK-8072863

JDK9 changeset: http://hg.openjdk.java.net/jdk9/hs-rt/hotspot/rev/f47b463a95b8

Review thread: 
http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2015-April/014392.html

Regards,
Cheleswer

RFR[ 9u-dev] JDK-8075773 - jps running as root fails with 7u75, works fine with 7u72

2015-05-14 Thread cheleswer sahu


Hi,
Please review the code changes for 
https://bugs.openjdk.java.net/browse/JDK-8075773. I have built and tested JDK9 
with fix successfully. As I do not have an account for OpenJDK,
David Buck will push the fix into jdk9/hs-rt/.

Web review link: http://cr.openjdk.java.net/~dbuck/8075773/webrev.01/

Regards,
Cheleswer

RFR[ 9u-dev] JDK-8075773 - jps running as root fails with 7u75, works fine with 7u72

2015-05-14 Thread cheleswer sahu


Hi,
Please review the code changes for 
https://bugs.openjdk.java.net/browse/JDK-8075773. I have built and tested JDK9 
with fix successfully. As I do not have an account for OpenJDK,
David Buck will push the fix into jdk9/hs-rt/.

Web review link: http://cr.openjdk.java.net/~dbuck/8075773/webrev.01/

Regards,
Cheleswer

Re: RFR[ 9u-dev] JDK-8075773 - jps running as root fails with 7u75, works fine with 7u72

2015-05-14 Thread cheleswer sahu


Dear Dan & Dmitry,
Thanks for pointing out the security vulnerability in the fix and for 
your precise review. I am also agree with Dmitry's fix. I will fix the 
code and resend the review request.


Regards,
Cheleswer

On 5/14/2015 9:46 PM, Gerald Thornbrugh wrote:

Hi Dan,

When Cheleswer and I discussed this fix my interpretation had a slightly 
different goal:

Prior to the initial security fix any user could execute "jps" and get the user 
names associated
with other user's perf data (i.e. the call to get_user_name_slow() would 
succeed.). My initial
thought was that this was a regression for all users not just "root" and this 
goal led to this fix.
At the time I did not see this as a security vulnerability, your review has 
changed my mind.

I agree that Dmitry's fix is a more secure fix for the issue and I think we 
should use it.

Let me know if you have any questions.

Thanks!

Jerry


Hi,
Please review the code changes for
https://bugs.openjdk.java.net/browse/JDK-8075773. I have built and
tested JDK9 with fix successfully. As I do not have an account for
OpenJDK,
David Buck will push the fix into jdk9/hs-rt/.

Web review link: http://cr.openjdk.java.net/~dbuck/8075773/webrev.01/

Regards,
Cheleswer

Cheleswer,

Sorry for the lengthy review, but since this is security related,
I have to be complete.

The goal: Add a policy by-pass for the 'root' user in order to
  fix the regression in jps(1) behavior.

The core of this policy by-pass is the change to this function:

   205 static bool is_statbuf_secure(struct stat *statp, int mode) {
   206   if (S_ISLNK(statp->st_mode) || !S_ISDIR(statp->st_mode)) {
   207 // The path represents a link or some non-directory file type,
   208 // which is not what we expected. Declare it insecure.
   209 //
   210 return false;
   211   }
   212   // If the directory is going to be opened readonly, we consider
this as secure operation
   213   // we do not need to do any more checks.
   214   //
   215   if ((mode & O_ACCMODE) == O_RDONLY) {
   216 return true;
   217   }
   218   // We have an existing directory, check if the permissions are safe.
   219   //
   220   if ((statp->st_mode & (S_IWGRP|S_IWOTH)) != 0) {
   221 // The directory is open for writing and could be subjected
   222 // to a symlink or a hard link attack. Declare it insecure.
   223 //
   224 return false;
   225   }
   226   // See if the uid of the directory matches the effective uid of
the process.
   227   //
   228   if (statp->st_uid != geteuid()) {
   229 // The directory was not created by this user, declare it
insecure.
   230 //
   231 return false;
   232   }
   233   return true;
   234 }

Lines 212-217 are added which allows a caller that passes in O_RDONLY
to by-pass the security checks on lines 220-225 and 228-232. This
implementation is using an attribute of _how_ the data is accessed
to override security policy instead of an attribute of _who_ is
accessing the data.

Here are the code paths that access the modified policy code:

is_statbuf_secure() is called by:

- is_directory_secure()
- is_dirfd_secure()

is_directory_secure() is called by:

- get_user_name_slow() with O_RDONLY
- make_user_tmp_dir() with O_RDWR
- mmap_attach_shared() with (O_RDONLY | O_NOFOLLOW)

is_dirfd_secure() is called by:

- open_directory_secure() with a mode parameter

open_directory_secure() is called by:

- open_directory_secure_cwd() with O_RDWR
- get_user_name_slow() with O_RDONLY

Only the code paths that specify O_RDWR make use of
the new policy by-pass code so it looks like only

- get_user_name_slow() with O_RDONLY
- mmap_attach_shared() with (O_RDONLY | O_NOFOLLOW)

are interesting.

The new security policy by-pass will allow get_user_name_slow():

- to process directory entries in a directory that is writable
which makes this use subject to a symlink or hard link attack.
- to process directory entries in a directory that the calling
user does not own; the intent of the policy by-pass is to
allow this for the 'root' user, but this implementation
allows the by-pass for any user.

It looks like the get_user_name_slow() code is written safely
enough such that any symlink or hard link attack should not
cause any issues.

The new policy by-pass will allow any user to determine the
user name associated with VMs owned by another user. This is
a broader policy by-pass than was intended.


The new security policy by-pass will allow mmap_attach_shared():

- to process directory entries in a directory that is writable
which makes this use subject to a symlink or hard link attack.
- to process directory entries in a directory that the calling
user does not own; the intent of the policy by-pass is to
allow this for the 'root' user, but this implementation allows
the by-pass for any user.

The mmap_attach_shared() code protects itself from a symlink
attack by including the 'O_NOFOLLOW' flag when opening the
PerfData fil

Re: RFR[ 9u-dev] JDK-8075773 - jps running as root fails with 7u75, works fine with 7u72

2015-05-18 Thread cheleswer sahu


Hi,
I have fixed the code and tested. It's working fine. Please review the 
changes.

Web review link: http://cr.openjdk.java.net/~dbuck/8075773/webrev.02/

Regards,
Cheleswer

On 5/15/2015 12:26 PM, cheleswer sahu wrote:

Dear Dan & Dmitry,
Thanks for pointing out the security vulnerability in the fix and for 
your precise review. I am also agree with Dmitry's fix. I will fix the 
code and resend the review request.


Regards,
Cheleswer

On 5/14/2015 9:46 PM, Gerald Thornbrugh wrote:

Hi Dan,

When Cheleswer and I discussed this fix my interpretation had a 
slightly different goal:


Prior to the initial security fix any user could execute "jps" and 
get the user names associated
with other user's perf data (i.e. the call to get_user_name_slow() 
would succeed.). My initial
thought was that this was a regression for all users not just "root" 
and this goal led to this fix.
At the time I did not see this as a security vulnerability, your 
review has changed my mind.


I agree that Dmitry's fix is a more secure fix for the issue and I 
think we should use it.


Let me know if you have any questions.

Thanks!

Jerry


Hi,
Please review the code changes for
https://bugs.openjdk.java.net/browse/JDK-8075773. I have built and
tested JDK9 with fix successfully. As I do not have an account for
OpenJDK,
David Buck will push the fix into jdk9/hs-rt/.

Web review link: http://cr.openjdk.java.net/~dbuck/8075773/webrev.01/

Regards,
Cheleswer

Cheleswer,

Sorry for the lengthy review, but since this is security related,
I have to be complete.

The goal: Add a policy by-pass for the 'root' user in order to
  fix the regression in jps(1) behavior.

The core of this policy by-pass is the change to this function:

   205 static bool is_statbuf_secure(struct stat *statp, int mode) {
   206   if (S_ISLNK(statp->st_mode) || !S_ISDIR(statp->st_mode)) {
   207 // The path represents a link or some non-directory file 
type,

   208 // which is not what we expected. Declare it insecure.
   209 //
   210 return false;
   211   }
   212   // If the directory is going to be opened readonly, we consider
this as secure operation
   213   // we do not need to do any more checks.
   214   //
   215   if ((mode & O_ACCMODE) == O_RDONLY) {
   216 return true;
   217   }
   218   // We have an existing directory, check if the permissions 
are safe.

   219   //
   220   if ((statp->st_mode & (S_IWGRP|S_IWOTH)) != 0) {
   221 // The directory is open for writing and could be subjected
   222 // to a symlink or a hard link attack. Declare it insecure.
   223 //
   224 return false;
   225   }
   226   // See if the uid of the directory matches the effective uid of
the process.
   227   //
   228   if (statp->st_uid != geteuid()) {
   229 // The directory was not created by this user, declare it
insecure.
   230 //
   231 return false;
   232   }
   233   return true;
   234 }

Lines 212-217 are added which allows a caller that passes in O_RDONLY
to by-pass the security checks on lines 220-225 and 228-232. This
implementation is using an attribute of _how_ the data is accessed
to override security policy instead of an attribute of _who_ is
accessing the data.

Here are the code paths that access the modified policy code:

is_statbuf_secure() is called by:

- is_directory_secure()
- is_dirfd_secure()

is_directory_secure() is called by:

- get_user_name_slow() with O_RDONLY
- make_user_tmp_dir() with O_RDWR
- mmap_attach_shared() with (O_RDONLY | O_NOFOLLOW)

is_dirfd_secure() is called by:

- open_directory_secure() with a mode parameter

open_directory_secure() is called by:

- open_directory_secure_cwd() with O_RDWR
- get_user_name_slow() with O_RDONLY

Only the code paths that specify O_RDWR make use of
the new policy by-pass code so it looks like only

- get_user_name_slow() with O_RDONLY
- mmap_attach_shared() with (O_RDONLY | O_NOFOLLOW)

are interesting.

The new security policy by-pass will allow get_user_name_slow():

- to process directory entries in a directory that is writable
which makes this use subject to a symlink or hard link attack.
- to process directory entries in a directory that the calling
user does not own; the intent of the policy by-pass is to
allow this for the 'root' user, but this implementation
allows the by-pass for any user.

It looks like the get_user_name_slow() code is written safely
enough such that any symlink or hard link attack should not
cause any issues.

The new policy by-pass will allow any user to determine the
user name associated with VMs owned by another user. This is
a broader policy by-pass than was intended.


The new security policy by-pass will allow mmap_attach_shared():

- to process directory entries in a directory that is writable
which makes this use subject to a symlink or hard link attack.
- to process directory entries in a dire

[8u60] request for approval: 8075773 : jps running as root fails after the fix of JDK-8050807

2015-08-12 Thread cheleswer sahu


Hi!

May I please have approval to backport this fix from JDK9 to JDK8. I have built 
the JDK-8 hotspot and tested already. JDK9 fix applies cleanly to JDK8.
As I do not have an account for OPENJDK, Kevin Walls will push the fix into 
jdk8u/hs-dev/hotspot.

BUGURL: https://bugs.openjdk.java.net/browse/JDK-8075773

JDK9 changeset: http://hg.openjdk.java.net/jdk9/hs-rt/hotspot/rev/0762dac9

Review thread: 
http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2015-August/015593.html

Regards,
Cheleswer

RFR[ 9u-dev] JDK-8129348: Debugger hangs in trace mode with TRACE_SENDS

2015-10-22 Thread cheleswer sahu


Hi,

Please review the code changes for 
"https://bugs.openjdk.java.net/browse/JDK-8129348";.
Web review link: http://cr.openjdk.java.net/~kevinw/8129348/webrev.01/

Bug Brief: Debugger hangs in trace mode when TRACE_SENDS flag is enabled. To 
print the argument information, it does a remote call from a
synchronized method which causes a deadlock .

Fix proposed:
Cache the value of args by calling the toString() method on args before 
entering into synchronized method. This cached values can be used later
to print the values when needed and avoid deadlock.
The bug is introduced in build time generated code (JDWP.java), and changing 
the generator code is problematic so cache value has been chosen as a solution.
 


Regards,
Cheleswer

37 matches

Mail list logo