RFR(S): JDK-8042155 Tests for stack guard pages have to be cleaned up
Dmitry Samersoff
dmitry.samersoff at oracle.com
Tue May 20 12:04:38 UTC 2014
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)
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.
-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
>>>>>>
>>
>
--
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.
More information about the hotspot-runtime-dev
mailing list