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

Dmitry Samersoff dmitry.samersoff at oracle.com
Wed May 14 09:42:01 UTC 2014


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