<html>
<head>
<meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
1. check overflow routine is wrapped up.<br>
<br>
2. The reduction of code duplication is adopted.<br>
<br>
3. Develop a way to intake 0~multiple external vm options in jtreg
main(). Hope it helps others implement similar functionality.<br>
<br>
webrev:<br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~tamao/6761744/webrev.02/">http://cr.openjdk.java.net/~tamao/6761744/webrev.02/</a><br>
<br>
test:<br>
JTREG: passed.<br>
jtreg -jdk:/home/tamao/jdk1.7.0_04-i586 -vmoptions:"-tamao
<GC_OPTION>"
/home/tamao/src/6761744CrashIfProcessSizeLimitExceeded_hsx24/test/gc/init/TestHandleExceedingProcessSizeLimitIn32BitBuilds.java<br>
<br>
where GC_OPTION rotates in -XX:+UseParallelGC -XX:+UseG1GC
-XX:+UseSerialGC -XX:+UseParNewGC -XX:+UseConcMarkSweepGC<br>
<br>
<br>
Thanks.<br>
Tao<br>
<br>
On 4/15/13 1:37 AM, Bengt Rutisson wrote:
<blockquote cite="mid:516BBC64.6060509@oracle.com" type="cite">
<meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
<div class="moz-cite-prefix"><br>
Hi Tao,<br>
<br>
I agree with John. The changes look fine and I'm ok with them.
But it would look nicer to add the
CollectedHeap::add_and_verify_no_overflow() method.<br>
<br>
Also, regarding the test, a couple of minor things.<br>
<br>
First, with the new naming convention I think the test should be
called something like:<br>
<br>
test/gc/init/TestHandleExceedingProcessSizeLimitOn32BitSystems.java<br>
<br>
Then I think you can reduce the code duplication a bit. You
create the same ProcessBuilder and OutputAnalyzer in both 32 and
64 bit cases. So maybe it could look like this instead:<br>
<br>
<tt>public class
TestHandleExceedingProcessSizeLimitOn32BitSystems {</tt><tt><br>
</tt><tt> public static void main(String args[]) throws
Exception {</tt><tt><br>
</tt><tt> ProcessBuilder pb =</tt><tt><br>
</tt><tt>
ProcessTools.createJavaProcessBuilder(System.getProperty("test.vm.opts"),</tt><tt><br>
</tt><tt>
"-Xmx3072m",</tt><tt><br>
</tt><tt>
"-XX:MaxPermSize=1024m",</tt><tt><br>
</tt><tt>
"-version");</tt><tt><br>
</tt><tt> OutputAnalyzer output = new
OutputAnalyzer(pb.start());</tt><tt><br>
</tt><tt><br>
</tt><tt> String dataModel =
System.getProperty("sun.arch.data.model");</tt><tt><br>
</tt><tt> if (dataModel.equals("32")) {</tt><tt><br>
</tt><tt> output.shouldContain("The size of the object heap
+ perm gen exceeds the maximum representable size");</tt><tt><br>
</tt><tt> if (output.getExitValue() == 0) {</tt><tt><br>
</tt><tt> throw new RuntimeException("Not expected to get
exit value 0");</tt><tt><br>
</tt><tt> }</tt><tt><br>
</tt><tt> } else if (dataModel.equals("64")) {</tt><tt><br>
</tt><tt> output.shouldHaveExitValue(0);</tt><tt><br>
</tt><tt> }</tt><tt><br>
</tt><tt> }</tt><tt><br>
</tt><tt>}</tt><tt><br>
</tt><br>
There is also a small bug in the test. If you run JTreg without
passing any vmoptions the test will fail. The reason is that
System.getProperty("test.vm.opts") will evaluate to the empty
string. This will be passed as the first argument to the Java
process, which will assume that this is the class name. So, it
fails to start because it can't load the class <empty
string>.<br>
<br>
To reproduce the problem use this command line:<br>
<br>
java -jar <path_to_jtreg>/lib/jtreg.jar
test/gc/6761744/TestHandleExceedingProcessSizeLimitOn32BitSystems.java<br>
<br>
I guess the fix is to check that
System.getProperty("test.vm.opts") is not empty before passing
it on to createJavaProcessBuilder().<br>
<br>
Thanks,<br>
Bengt<br>
<br>
On 4/13/13 3:15 AM, John Cuthbertson wrote:<br>
</div>
<blockquote cite="mid:5168B1AD.7060302@oracle.com" type="cite">
<meta content="text/html; charset=UTF-8"
http-equiv="Content-Type">
Hi Tao,<br>
<br>
This looks OK to me.<br>
<br>
I can see what Thomas is saying though. All of the checks
involve something like:<br>
<br>
total += some_value;<br>
if (total < some_value) {<br>
// We must have overflowed<br>
vm_exit(...);<br>
}<br>
<br>
So a function in CollectedHeap (the base class of SharedHeap and
ParallelScavengeHeap) might make sense. For example:<br>
<br>
total_reserved = 0;<br>
total_reserved = add_and_verify_no_overflow(total_reserved,
max_heap_size);<br>
total_reserved = add_and_verify_no_overflow(total_reserved,
max_perm_size);<br>
<br>
Where the function is:<br>
<br>
size_t add_and_verify_no_overflow(size_t x, size_t y) {<br>
const char* msg = "...";<br>
x += y;<br>
if (x < y) {<br>
vm_exit(msg);<br>
}<br>
return x;<br>
}<br>
<br>
But I can live with your current changes.<br>
<br>
JohnC<br>
<br>
<div class="moz-cite-prefix">On 4/10/2013 9:52 AM, Tao Mao
wrote:<br>
</div>
<blockquote cite="mid:516598D3.1020809@oracle.com" type="cite">
<meta content="text/html; charset=UTF-8"
http-equiv="Content-Type">
Hi Bengt,<br>
<br>
Thank you for reviewing. A new webrev is updated.<br>
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Etamao/6761744/webrev.01/">http://cr.openjdk.java.net/~tamao/6761744/webrev.01/</a><br>
<br>
Cheers,<br>
Tao<br>
<br>
On 4/10/13 1:54 AM, Bengt Rutisson wrote:
<blockquote cite="mid:516528C2.8080709@oracle.com" type="cite">
<meta content="text/html; charset=UTF-8"
http-equiv="Content-Type">
<div class="moz-cite-prefix"><br>
Hi Tao,<br>
<br>
This change looks good. Thanks for adding the JTReg test,
it looks good!<br>
<br>
One minor nit:<br>
<br>
In src/share/vm/gc_implementation/g1/g1CollectedHeap.cpp I
would suggest to
<meta http-equiv="content-type" content="text/html;
charset=UTF-8">
store "(size_t) align_size_up(pgs->max_size(),
HeapRegion::GrainBytes)" in a local variable rather than
duplicating the calculation.<br>
<br>
Thanks,<br>
Bengt<br>
<br>
On 4/8/13 7:22 AM, Tao Mao wrote:<br>
</div>
<blockquote cite="mid:51625424.4050209@oracle.com"
type="cite">
<meta http-equiv="content-type" content="text/html;
charset=UTF-8">
6761744 Hotspot crashes if process size limit is exceeded<br>
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="https://jbs.oracle.com/bugs/browse/JDK-6761744">https://jbs.oracle.com/bugs/browse/JDK-6761744</a><br>
<br>
webrev:<br>
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Etamao/6761744/webrev.00/">http://cr.openjdk.java.net/~tamao/6761744/webrev.00/</a><br>
<br>
changeset:<br>
The fix only needs to go to hsx24 since there's no perm
gen in hotspot-25. Thus, the webrev is based on hsx24
repo.<br>
<br>
It provides for 32-bit builds a preventive check of the
size of "the object heap + perm gen" before reserving VM
memory. The total size should not exceed 4096MB (i.e. 4GB)
for 32-bit builds; otherwise, the total doesn't make sense
and, what's worse, overflow occurs. It will
consequentially trigger anther error of memory access
violation, which was not protected<span style="color:
rgb(0, 0, 0); font-family: Arial, FreeSans, Helvetica,
sans-serif; font-size: 12.727272033691406px; font-style:
normal; font-variant: normal; font-weight: normal;
letter-spacing: normal; line-height:
15.454545021057129px; orphans: auto; text-align: start;
text-indent: 0px; text-transform: none; white-space:
normal; widows: auto; word-spacing: 0px;
-webkit-text-size-adjust: auto;
-webkit-text-stroke-width: 0px; background-color:
rgb(240, 240, 240); display: inline !important; float:
none;"></span>.<br>
<br>
jtreg testing java code is also written, checking both
32-bit and (trivially) 64-bit builds.<br>
<br>
testing:<br>
check jtreg tests with flags -XX:+UseParallelGC,
-XX:+UseG1GC, -XX:+UseParNewGC, -XX:+UseConcMarkSweepGC,
-XX:+UseSerialGC and builds of 32-bit and 64-bit. All
passed.<br>
<br>
Needs JPRT test when pushing.<br>
</blockquote>
<br>
</blockquote>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
</body>
</html>