RFR(S): JDK-8042155 Tests for stack guard pages have to be cleaned up

[Dmitry Samersoff]

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/

-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.