RFR(S): JDK-8042155 Tests for stack guard pages have to be cleaned up
Daniel D. Daugherty
daniel.daugherty at oracle.com
Wed May 14 17:12:55 UTC 2014
Thumbs up.
Dan
On 5/14/14 3: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