On 12/3/2014 11:26 AM, Chris Plummer wrote:
Hi Kumar,
On 12/3/14 10:58 AM, Kumar Srinivasan wrote:
Hi Chris,
Approved with some minor nits, typos which needs correction.
yes java.c follows the JDK indenting as Alan pointed out.
TooSmallStackSize.java
Copyright should be 2014,
Copy/paste error from example test I was referred to. I will fix, and
also remove the import if not needed.
1.
37 * stack size for the platform (as provided by the JVM error
message when a very
38 * small is used), and then verify that the JVM can be launched
with that stack
s/small is/small stack is/
ok
2.
54 * Returns the minimum stack size this platform will allowed
based on the
s/allowed/allow/
ok
3.
58 * The TestResult argument must containthe result of having
already run
s/containthe/contain the/
ok
4.
92 if (verbose) System.out.println("*** Testing " + stackSize);
I know this is allowed in the HotSpot world but in JDK land we always
use with a LF + Indent, also in other places.
Are curly braces needed then? I know some coding conventions require
them.
No not necessary for one liners.
5.
85 * Returns the mimumum allowed stack size gleaned from the
error message,
s/mimumum/minimum
ok.
Finally I am concerned with the integration, since it spans both
HotSpot and JDK, and it appears the test will fail if the HotSpot
changes are not integrated first, or has it already ?
There are no hotspot changes. java.c is where the fix is.
Great!.
Thanks
Kumar
thanks,
Chris
Thanks
Kumar
On 12/1/2014 6:39 PM, Chris Plummer wrote:
Sorry about the long delay in getting back to this. I ran into two
separate JPRT issues that were preventing me from testing these
changes, plus I was on vacation last week. Here's an updated webrev.
I'm not sure where we left things, so I'll just say what's changed
since the original version:
1. Rewrote the test to be in Java instead of a shell script.
2. Moved the test from hotspot/test/runtime/memory to
jdk/test/tools/launcher
3. Added STACK_SIZE_MINIMUM to java.c, allowing a makefile to
override the default 32k minimum value.
https://bugs.openjdk.java.net/browse/JDK-6762191
http://cr.openjdk.java.net/~cjplummer/6762191/webrev.02/
thanks,
Chris
On 11/19/14 7:52 AM, Chris Plummer wrote:
On 11/19/14 2:12 AM, David Holmes wrote:
On 19/11/2014 6:49 PM, Chris Plummer wrote:
I've update the webrev to add STACK_SIZE_MINIMUM in place of the 32k
references, and also moved the test from hotspot/test/runtime to
jdk/test/tools/launcher as David requested. That required some
adjustments to the test script, since test_env.sh does not exist in
jdk/test, so I had to pull in the bits I needed into the script.
Is there a reason this needs a shell script instead of using the
testlibrary tools to launch the VM and check the output?
Not that I'm aware of. I guess I just really didn't look at what it
would take to make it all in java. I'll have a look at java
examples and convert it.
Chris
Sorry that should have been mentioned much earlier.
David
http://cr.openjdk.java.net/~cjplummer/6762191/webrev.01/
I still need to rerun through JPRT. I'll do so once there are no
more
suggested changes.
thanks,
Chris
On 11/18/14 2:08 PM, Chris Plummer wrote:
Adding core-libs-...@openjdk.java.net, since one of the changes
is in
java.c.
Chris
On 11/12/14 6:43 PM, David Holmes wrote:
Hi Chris,
Sorry for the delay.
On 13/11/2014 5:44 AM, Chris Plummer wrote:
Hi,
I'm still looking for reviewers.
As the change is to the launcher it needs to be reviewed by the
launcher owner - which I think is serviceability (though also cc'd
Kumar :) ).
Launcher change, and your rationale, seems okay to me. I'd
probably
put the test in to jdk/test/tools/launcher/ though.
Thanks,
David
thanks,
Chris
On 11/7/14 7:53 PM, Chris Plummer wrote:
This is an initial review for 6762191. I'm guessing there
will be
recommendations to fix in a different way, but thought this
would be a
good time to start the discussion.
https://bugs.openjdk.java.net/browse/JDK-6762191
http://cr.openjdk.java.net/~cjplummer/6762191/webrev.00.jdk/
http://cr.openjdk.java.net/~cjplummer/6762191/webrev.00.hotspot/
The bug is that if the -Xss size is set to something very
small (like
16k), on linux there will be a crash due to overwriting the
end of the
stack. This happens before hotspot can compute its stack
needs and
verify that the stack is big enough.
It didn't seem viable to move the hotspot stack size check
earlier. It
depends on too much other work done before that point, and
the changes
would have been disruptive. The stack size check is currently
done in
os::init_2().
What is needed is a check before the thread is created. That
way we
can create a thread with a big enough stack to handle all
needs up to
the point of the check in os::init_2(). This initial check
does not
need to be the final check. It just needs to confirm that we
have
enough stack to get us to the check in os::init_2().
I decided to check in java.c if the -Xss size is too small,
and set it
to a larger size if it is. I hard coded this size to 32k
(I'll explain
why 32k later). I suspect this is the part that will result
in some
debate. If you have better suggestions let me know. If it
does stay
here, then probably the 32k needs to be a #define, and maybe
even an
OS porting interface, but I'm not sure where to put it.
The reason I chose 32k is because this is big enough for all
platforms
to get to the stack size check in os::init_2(). It is also
smaller
than the actual minimum stack size allowed on any platform.
32-bit
windows has the smallest requirement at 64k. I add some
printfs to
print the minimum stack requirement, and then ran a simple
JTReg test
with every JPRT supported platform to get the results.
The TooSmallStackSize.sh will run "java -version" with -Xss16k,
-Xss32k, and -XXss<minsize>, where <minsize> is the size from
the
error message produced by the JVM, such as in the following:
$ java -Xss32k -version
The stack size specified is too small, Specify at least 100k
Error: Could not create the Java Virtual Machine.
Error: A fatal exception has occurred. Program will exit.
I ran this test through JPRT on all platforms, and they all
pass.
One thing to point out is that Windows behaves a bit
different than
the other platforms. It always rounds the stack size up to a
multiple
of 64k , so even if you specify -Xss16k, you get a 64k stack. On
32-bit Windows with C1, 64k is also the minimum requirement,
so there
is no error produced in this case. However, on 32-bit Windows
with C2,
68k is the minimum, so an error is produced since the stack
will only
be 64k. There is no bug here. It's just a bit confusing.
thanks,
Chris