Re: [9] RFR (S) 6762191: Setting stack size to 16K causes segmentation fault
On 12/3/14 4:56 AM, Alan Bateman wrote: On 02/12/2014 02:39, 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/ This looks to me. A minor comment for java.c is that this code uses 4-space indent (different to hotspot). The test looks okay too, you might just checking the copyright date as I assume was not written in 2010. Also I think the import of java.io.File may be left behind from the previous round. -Alan Hi Alan, While removing the java.io.File import, I also questioned why I had java.io.IOException being imported. There were a couple of methods that declared it thrown, and the main method therefore had to catch it, but it turns out this was just copy/paste from the Settings.java test I used as a template, and is not actually needed. I removed the import, throws, and try/catch of IOException. All the other issues mentioned by others have also been addressed. A new webrev can be found at: http://cr.openjdk.java.net/~cjplummer/6762191/webrev.03/ thanks, Chris
Re: [9] RFR (S) 6762191: Setting stack size to 16K causes segmentation fault
On 04/12/2014 08:12, Chris Plummer wrote: Hi Alan, While removing the java.io.File import, I also questioned why I had java.io.IOException being imported. There were a couple of methods that declared it thrown, and the main method therefore had to catch it, but it turns out this was just copy/paste from the Settings.java test I used as a template, and is not actually needed. I removed the import, throws, and try/catch of IOException. All the other issues mentioned by others have also been addressed. A new webrev can be found at: http://cr.openjdk.java.net/~cjplummer/6762191/webrev.03/ This looks good to me. -Alan.
Re: [9] RFR (S) 6762191: Setting stack size to 16K causes segmentation fault
Looks good to me too Chris - sorry for the delay getting back to you. But at least Kumar spotted all the typos :) David On 4/12/2014 6:12 PM, Chris Plummer wrote: On 12/3/14 4:56 AM, Alan Bateman wrote: On 02/12/2014 02:39, 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/ This looks to me. A minor comment for java.c is that this code uses 4-space indent (different to hotspot). The test looks okay too, you might just checking the copyright date as I assume was not written in 2010. Also I think the import of java.io.File may be left behind from the previous round. -Alan Hi Alan, While removing the java.io.File import, I also questioned why I had java.io.IOException being imported. There were a couple of methods that declared it thrown, and the main method therefore had to catch it, but it turns out this was just copy/paste from the Settings.java test I used as a template, and is not actually needed. I removed the import, throws, and try/catch of IOException. All the other issues mentioned by others have also been addressed. A new webrev can be found at: http://cr.openjdk.java.net/~cjplummer/6762191/webrev.03/ thanks, Chris
Re: [9] RFR (S) 6762191: Setting stack size to 16K causes segmentation fault
It still looks good to me too. :) Thanks, Serguei On 12/4/14 3:46 PM, David Holmes wrote: Looks good to me too Chris - sorry for the delay getting back to you. But at least Kumar spotted all the typos :) David On 4/12/2014 6:12 PM, Chris Plummer wrote: On 12/3/14 4:56 AM, Alan Bateman wrote: On 02/12/2014 02:39, 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/ This looks to me. A minor comment for java.c is that this code uses 4-space indent (different to hotspot). The test looks okay too, you might just checking the copyright date as I assume was not written in 2010. Also I think the import of java.io.File may be left behind from the previous round. -Alan Hi Alan, While removing the java.io.File import, I also questioned why I had java.io.IOException being imported. There were a couple of methods that declared it thrown, and the main method therefore had to catch it, but it turns out this was just copy/paste from the Settings.java test I used as a template, and is not actually needed. I removed the import, throws, and try/catch of IOException. All the other issues mentioned by others have also been addressed. A new webrev can be found at: http://cr.openjdk.java.net/~cjplummer/6762191/webrev.03/ thanks, Chris
Re: [9] RFR (S) 6762191: Setting stack size to 16K causes segmentation fault
On 02/12/2014 02:39, 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/ This looks to me. A minor comment for java.c is that this code uses 4-space indent (different to hotspot). The test looks okay too, you might just checking the copyright date as I assume was not written in 2010. Also I think the import of java.io.File may be left behind from the previous round. -Alan
Re: [9] RFR (S) 6762191: Setting stack size to 16K causes segmentation fault
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. 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. 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-dev@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
Re: [9] RFR (S) 6762191: Setting stack size to 16K causes segmentation fault
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-dev@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
Re: [9] RFR (S) 6762191: Setting stack size to 16K causes segmentation fault
The fix still looks good to me. Thanks, Serguei On 12/1/14 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-dev@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 -XXssminsize, 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
Re: [9] RFR (S) 6762191: Setting stack size to 16K causes segmentation fault
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-dev@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 -XXssminsize, 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
Re: [9] RFR (S) 6762191: Setting stack size to 16K causes segmentation fault
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. 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-dev@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 -XXssminsize, 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
Re: [9] RFR (S) 6762191: Setting stack size to 16K causes segmentation fault
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? 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-dev@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 -XXssminsize, 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
Re: [9] RFR (S) 6762191: Setting stack size to 16K causes segmentation fault
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-dev@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 -XXssminsize, 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
Re: [9] RFR (S) 6762191: Setting stack size to 16K causes segmentation fault
Adding core-libs-dev@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 -XXssminsize, 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
