RFR(S): JDK-8042155 Tests for stack guard pages have to be cleaned up
Coleen Phillimore
coleen.phillimore at oracle.com
Wed May 21 18:25:39 UTC 2014
This is good, Dmitry. Thank you for making the changes.
Coleen
On 5/21/14, 1:41 PM, Dmitry Samersoff wrote:
> Thank you!
>
> On 2014-05-21 20:09, Daniel D. Daugherty wrote:
>> Thumbs up on the new version of test...
>>
>> Dan
>>
>>
>> On 5/21/14 6:09 AM, Dmitry Samersoff wrote:
>>> Coleen,
>>>
>>> Please, see updated webrev:
>>>
>>> http://cr.openjdk.java.net/~dsamersoff/JDK-8042155/webrev.03/
>>>
>>> only invoke.c is changed (added -Xint and limited recursion depth for
>>> second run).
>>>
>>>>> It's guaranteed. We always have libjvm mapped after stack of initial
>>>>> thread.
>>>> We do? Really?
>>> Sure! Memory layout *of this test* is always the same. Unless we change
>>> JNI_CreateJavaVM() implementation drastically.
>>>
>>> -Dmitry
>>>
>>>
>>>
>>> On 2014-05-20 18:59, Coleen Phillimore wrote:
>>>> On 5/20/14, 10:16 AM, Dmitry Samersoff wrote:
>>>>> Coleen,
>>>>>
>>>>>> Is this guaranteed? How many recursions of do_overflow will this
>>>>>> take? I still prefer my suggestion of limiting the recursions in the
>>>>>> second case of do_overfflow so the test doesn't rely on some
>>>>>> environmental setting that may not be true.
>>>>> It's guaranteed. We always have libjvm mapped after stack of initial
>>>>> thread.
>>>> We do? Really?
>>>>> Typically we crashes after 136000 recursive calls and it takes 1.071
>>>>> sec.
>>>>>
>>>>> But it's worth to limit recursion in second case. I'll do it.
>>>> Yes, please do this!
>>>>
>>>> Coleen
>>>>> -Dmitry
>>>>>
>>>>>
>>>>> On 2014-05-20 17:54, Coleen Phillimore wrote:
>>>>>> Dmitry, some comments below.
>>>>>>
>>>>>> On 5/20/14, 8:04 AM, Dmitry Samersoff wrote:
>>>>>>> Coleen,
>>>>>>>
>>>>>>> Thank you for the review!
>>>>>>>
>>>>>>> Please see below.
>>>>>>>
>>>>>>> On 2014-05-20 06:19, Coleen Phillimore wrote:
>>>>>>>> I have reviewed the test. I'm happy that you are fixing this test
>>>>>>>> because it's been really problematic, but I have a couple of
>>>>>>>> concerns.
>>>>>>>> First the java program overflow doesn't allocate anything on the
>>>>>>>> stack
>>>>>>>> so when it gets compiled it might take a really long time to
>>>>>>>> run. Can
>>>>>>>> you pass -Xint in invoke.c in the option string?
>>>>>>> Sure. Will do.
>>>>>>>
>>>>>>>> Second, the second do_overflow() in run_native_overflow must hit
>>>>>>>> some
>>>>>>>> sort of protected page causing it to terminate, why isn't it
>>>>>>>> SIG_ACCERR? Maybe the second do_overflow should only recurse the
>>>>>>>> same
>>>>>>>> number of times as the first + some small number so it doesn't keep
>>>>>>>> running up the stack (until ???)
>>>>>>> Both first and second call to do_overflow can:
>>>>>>>
>>>>>>> 1. *hit a guard page* - we get SIGSEGV with SIG_ACCERR. (OK in first
>>>>>>> case, ERR in second)
>>>>>>>
>>>>>>> 2. *hit non-mapped page* - we get a SIGSEGV with code other than
>>>>>>> SIG_ACCERR. (ERR in first case, OK in second)
>>>>>> Is this guaranteed? How many recursions of do_overflow will this
>>>>>> take? I still prefer my suggestion of limiting the recursions in the
>>>>>> second case of do_overfflow so the test doesn't rely on some
>>>>>> environmental setting that may not be true.
>>>>>>> There is a case that is not covered by this test explicitly - if test
>>>>>>> guard page was never been installed and stack starts immediately
>>>>>>> after
>>>>>>> non-stack page it's possible that native do_verflow overwrite whole
>>>>>>> non-stack page and hit another protected page and we get SIGSEGV with
>>>>>>> SIG_ACCERR.
>>>>>>>
>>>>>>> Most probably, hotspot would crash in this case.
>>>>>>>
>>>>>>> Let me know if you like me to check it explicitly. We can read
>>>>>>> /proc/self/map and check what page cause SIGSEGV.
>>>>>> This seems more complicated and more likely to be affected by the
>>>>>> environment, so I think you should not do this for now.
>>>>>>
>>>>>> thanks,
>>>>>> Coleen
>>>>>>
>>>>>>> -Dmitry
>>>>>>>
>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Coleen
>>>>>>>>
>>>>>>>> On 5/14/14, 5:42 AM, Dmitry Samersoff wrote:
>>>>>>>>> Dan,
>>>>>>>>>
>>>>>>>>> Fixed (in-place, press shift-reload)
>>>>>>>>>
>>>>>>>>> http://cr.openjdk.java.net/~dsamersoff/JDK-8042155/webrev.02/
>>>>>>>>>
>>>>>>>>>> One thing I did notice is that some of the error
>>>>>>>>>> messages are
>>>>>>>>>> identical which would make it more difficult for someone
>>>>>>>>>> investigating a future failure to know which failure
>>>>>>>>>> happened:
>>>>>>>>> It shouldn't be an issue because a) test prints banner to stdout
>>>>>>>>> with
>>>>>>>>> the name of the function it's running b) test terminates after
>>>>>>>>> first
>>>>>>>>> error
>>>>>>>>>
>>>>>>>>> -Dmitry
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 2014-05-13 21:12, Daniel D. Daugherty wrote:
>>>>>>>>>> On 5/13/14 4:18 AM, Dmitry Samersoff wrote:
>>>>>>>>>>> Dan,
>>>>>>>>>>>
>>>>>>>>>>> Thank you for the review!
>>>>>>>>>>>
>>>>>>>>>>> 1. I use exit(7) to make the situation when the test fails
>>>>>>>>>>> because of
>>>>>>>>>>> some error condition clear visible.
>>>>>>>>>>>
>>>>>>>>>>> 2. I'm not sure it's a good idea to pass test if compilation
>>>>>>>>>>> fails, but
>>>>>>>>>>> I make this changes to handle it the same way as other tests does
>>>>>>>>>>>
>>>>>>>>>>> 3. Agree with rest of comments. See new webrev.
>>>>>>>>>>>
>>>>>>>>>>> http://cr.openjdk.java.net/~dsamersoff/JDK-8042155/webrev.02/
>>>>>>>>>> test/runtime/StackGuardPages/DoOverflow.java
>>>>>>>>>> No comments.
>>>>>>>>>>
>>>>>>>>>> test/runtime/StackGuardPages/invoke.c
>>>>>>>>>> I'm good with most of the changes.
>>>>>>>>>>
>>>>>>>>>> One thing I did notice is that some of the error
>>>>>>>>>> messages are
>>>>>>>>>> identical which would make it more difficult for someone
>>>>>>>>>> investigating a future failure to know which failure
>>>>>>>>>> happened:
>>>>>>>>>>
>>>>>>>>>> 102 fprintf(stderr, "Test ERROR. Can't load class
>>>>>>>>>> DoOverflow\n");
>>>>>>>>>> 144 printf("Test ERROR. Can't load class
>>>>>>>>>> DoOverflow\n");
>>>>>>>>>> You can make them unique by including the function
>>>>>>>>>> name that generated them
>>>>>>>>>>
>>>>>>>>>> I just noticed that some error messages still go to
>>>>>>>>>> stdout,
>>>>>>>>>> e.g., line 144 above.
>>>>>>>>>> Looks like this comment from the first round was also
>>>>>>>>>> missed
>>>>>>>>>> (the line number is now 243):
>>>>>>>>>>
>>>>>>>>>>> line 225: You'll end up here if no arguments are
>>>>>>>>>>> passed or
>>>>>>>>>>> if the
>>>>>>>>>>> wrong arguments are passed. You should have a better
>>>>>>>>>>> usage
>>>>>>>>>>> message, e.g.:
>>>>>>>>>>>
>>>>>>>>>>> void usage() {
>>>>>>>>>>> fprintf(stderr, "Usage: invoke
>>>>>>>>>>> test_java_overflow\n");
>>>>>>>>>>> fprintf(stderr, " invoke
>>>>>>>>>>> test_native_overflow\n");
>>>>>>>>>>> exit(1);
>>>>>>>>>>> }
>>>>>>>>>> test/runtime/StackGuardPages/testme.sh
>>>>>>>>>> No comments.
>>>>>>>>>>
>>>>>>>>>> test/runtime/6929067/T.java
>>>>>>>>>> test/runtime/6929067/Test6929067.sh
>>>>>>>>>> test/runtime/6929067/invoke.c
>>>>>>>>>> test/runtime/InitialThreadOverflow/DoOverflow.java
>>>>>>>>>> test/runtime/InitialThreadOverflow/invoke.c
>>>>>>>>>> test/runtime/InitialThreadOverflow/testme.sh
>>>>>>>>>> No comments on the deleted files.
>>>>>>>>>>
>>>>>>>>>> Dan
>>>>>>>>>>
>>>>>>>>>>> -Dmitry
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> On 2014-05-13 01:51, Daniel D. Daugherty wrote:
>>>>>>>>>>>> On 5/1/14 8:21 AM, Dmitry Samersoff wrote:
>>>>>>>>>>>>> Hi Everyone,
>>>>>>>>>>>>>
>>>>>>>>>>>>> Please, review test changes:
>>>>>>>>>>>>>
>>>>>>>>>>>>> http://cr.openjdk.java.net/~dsamersoff/JDK-8042155/webrev.01/
>>>>>>>>>>>> test/runtime/StackGuardPages/testme.sh
>>>>>>>>>>>> You need to check the command status after running
>>>>>>>>>>>> gcc_cmd
>>>>>>>>>>>> and if it is not zero, then gratuitously pass the test.
>>>>>>>>>>>> Jerry made a similar test fix last week:
>>>>>>>>>>>>
>>>>>>>>>>>> +if [ $? -ne 0 ] ; then
>>>>>>>>>>>> + echo "Compile failed, Ignoring failed compilation and
>>>>>>>>>>>> forcing the
>>>>>>>>>>>> test to pass"
>>>>>>>>>>>> + exit 0
>>>>>>>>>>>> +fi
>>>>>>>>>>>>
>>>>>>>>>>>> This will catch the case where a 64-bit Linux test
>>>>>>>>>>>> machine
>>>>>>>>>>>> has a compiler, but not the 32-bit build env.
>>>>>>>>>>>>
>>>>>>>>>>>> test/runtime/StackGuardPages/DoOverflow.java
>>>>>>>>>>>> No comments.
>>>>>>>>>>>>
>>>>>>>>>>>> test/runtime/StackGuardPages/invoke.c
>>>>>>>>>>>> line 28: * than detaching from vm thread and overflow
>>>>>>>>>>>> stack once
>>>>>>>>>>>> again.
>>>>>>>>>>>> Typo?: 'than detaching' -> 'Then we detach'
>>>>>>>>>>>>
>>>>>>>>>>>> line 92: (*_jvm)->AttachCurrentThread(_jvm,
>>>>>>>>>>>> (void**)&env,
>>>>>>>>>>>> NULL);
>>>>>>>>>>>> line 125: (*_jvm)->AttachCurrentThread(_jvm, (void
>>>>>>>>>>>> **)&env,
>>>>>>>>>>>> NULL);
>>>>>>>>>>>> What happens if the JNI AttachCurrentThread() call
>>>>>>>>>>>> fails?
>>>>>>>>>>>>
>>>>>>>>>>>> line 96: printf("Test ERROR. Can't load class
>>>>>>>>>>>> DoOverflow\n");
>>>>>>>>>>>> line 102: printf("Test ERROR. Can't find method
>>>>>>>>>>>> DoOverflow.printIt\n");
>>>>>>>>>>>> line 129: printf("Test ERROR. Can't load class
>>>>>>>>>>>> DoOverflow\n");
>>>>>>>>>>>> line 135: printf("Test ERROR. Can't find method
>>>>>>>>>>>> DoOverflow.printAlive\n");
>>>>>>>>>>>> line 192: printf("Test ERROR. Can't create JavaVM\n");
>>>>>>>>>>>> line 224: printf("Test ERROR. Unknown parameter
>>>>>>>>>>>> should be
>>>>>>>>>>>> test_java_overflow or test_native_overflow\n");
>>>>>>>>>>>> Don't errors usually get printed on stderr?
>>>>>>>>>>>>
>>>>>>>>>>>> line 106: (*env)->CallStaticVoidMethod(env, class_id,
>>>>>>>>>>>> method_id, NULL);
>>>>>>>>>>>> What happens if this call fails? Other than the
>>>>>>>>>>>> intentional
>>>>>>>>>>>> StackOverflowError, but wait, you catch and swallow
>>>>>>>>>>>> that
>>>>>>>>>>>> error...
>>>>>>>>>>>>
>>>>>>>>>>>> line 108: (*_jvm)->DetachCurrentThread(_jvm);
>>>>>>>>>>>> line 150 (*_jvm)->DetachCurrentThread(_jvm);
>>>>>>>>>>>> What happens if this call fails?
>>>>>>>>>>>>
>>>>>>>>>>>> line 139: (*env)->CallStaticVoidMethod (env, class_id,
>>>>>>>>>>>> method_id,
>>>>>>>>>>>> NULL);
>>>>>>>>>>>> What happens if this call fails?
>>>>>>>>>>>>
>>>>>>>>>>>> line 153: // For non-initial thread we doesn't unmap
>>>>>>>>>>>> Typo: "we doesn't unmap"
>>>>>>>>>>>> -> "we don't unmap"
>>>>>>>>>>>>
>>>>>>>>>>>> line 154: ...same SEGV_ACCERR tring to access it
>>>>>>>>>>>> Typos, perhaps: same SEGV signal trying to access it
>>>>>>>>>>>>
>>>>>>>>>>>> line 156: // We have no ways to check this, so bail out,
>>>>>>>>>>>> marking
>>>>>>>>>>>> test as succeded
>>>>>>>>>>>> Typo: 'no ways' -> 'no way'
>>>>>>>>>>>> Typo: 'as succeded' -> 'as succeeded'
>>>>>>>>>>>>
>>>>>>>>>>>> line 157: printf("Test OK. ...
>>>>>>>>>>>> line 171: printf("Test OK. ...
>>>>>>>>>>>> Should "OK" be "PASSED"?
>>>>>>>>>>>>
>>>>>>>>>>>> line 168: printf("Test FAILED. Stack guard page is still
>>>>>>>>>>>> there\n");
>>>>>>>>>>>> Don't failures usually get printed on stderr?
>>>>>>>>>>>>
>>>>>>>>>>>> line 225: You'll end up here if no arguments are
>>>>>>>>>>>> passed or
>>>>>>>>>>>> if the
>>>>>>>>>>>> wrong arguments are passed. You should have a better
>>>>>>>>>>>> usage
>>>>>>>>>>>> message, e.g.:
>>>>>>>>>>>>
>>>>>>>>>>>> void usage() {
>>>>>>>>>>>> fprintf(stderr, "Usage: invoke
>>>>>>>>>>>> test_java_overflow\n");
>>>>>>>>>>>> fprintf(stderr, " invoke
>>>>>>>>>>>> test_native_overflow\n");
>>>>>>>>>>>> exit(1);
>>>>>>>>>>>> }
>>>>>>>>>>>>
>>>>>>>>>>>> I noticed that all the exit() calls pass '7'. Why '7'?
>>>>>>>>>>>>
>>>>>>>>>>>> Also, why 'return 0' or 'return 1' instead of
>>>>>>>>>>>> 'exit(0)' or
>>>>>>>>>>>> 'exit(1)'?
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> test/runtime/6929067/T.java
>>>>>>>>>>>> test/runtime/6929067/Test6929067.sh
>>>>>>>>>>>> test/runtime/6929067/invoke.c
>>>>>>>>>>>> test/runtime/InitialThreadOverflow/DoOverflow.java
>>>>>>>>>>>> test/runtime/InitialThreadOverflow/invoke.c
>>>>>>>>>>>> test/runtime/InitialThreadOverflow/testme.sh
>>>>>>>>>>>> OK I can see why you want to combine the two tests.
>>>>>>>>>>>> I'm guessing that 'InitialThreadOverflow' was
>>>>>>>>>>>> derived from the much older '6929067'...
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> Dan
>>>>>>>>>>>>
>>>>>>>>>>>>> The fix combine tests for 6929067 and 8009062 into single, more
>>>>>>>>>>>>> sophisticated test.
>>>>>>>>>>>>>
>>>>>>>>>>>>> -Dmitry
>>>>>>>>>>>>>
>
More information about the hotspot-runtime-dev
mailing list