RFR(XS) 8020791: [TESTBUG] runtime/jsig/Test8017498.sh failed to compile

[Daniel D. Daugherty]

Thumbs up.

Forgot to ask you to add an @bug for this bug ID (8020791).

Dan


On 7/19/13 3:16 PM, Calvin Cheung wrote:
> Thanks to Dan's help, the .sh script is now simplified by:
> - using the timeout jtreg keyword so that the while loop isn't necessary;
> - using eval so that the LD_PRELOAD can be passed in the command line 
> without having to export the setting.
>
> webrev at the same location:
>     http://cr.openjdk.java.net/~ccheung/8020791/webrev/
>
> Calvin
>
> On 7/19/2013 1:06 PM, Daniel D. Daugherty wrote:
>>
>> On 7/19/13 1:07 PM, Yumin Qi wrote:
>>>
>>> On 7/19/2013 11:48 AM, Calvin Cheung wrote:
>>>> Thanks Dan and David for the reveiw.
>>>>
>>>> Dan,
>>>>
>>>> I've made all the changes as you've suggested except for the 
>>>> LD_PRELOAD="$MY_LD_PRELOAD" before the java command. The shell 
>>>> tried to execute the LD_PRELOAD="$MY_LD_PRELOAD" as a command and 
>>>> resulting in "No such file or directory" error.
>>>> So I'm keeping the "export" and "unset" of LD_PRELOAD for now.
>>>>
>>> You can put LD_PRELOAD="$MY_LD_PRELOAD" after command, that is
>>> shell cmd LD_PRELOAD="$MY_LD_PRELOAD"
>>
>> Yumin,
>>
>> I don't think that will work. The
>>
>>     VAR=<value> cmd ....
>>
>> is interpreted by the shell (sh in this case). Saying
>>
>>     cmd ... VAR=<value>
>>
>> will just pass "VAR=<value>" as an option to the cmd and
>> not set an environment variable.
>>
>> Dan
>>
>>
>>>
>>> Thanks
>>> Yumin
>>>> updated webrev is at the same location:
>>>>     http://cr.openjdk.java.net/~ccheung/8020791/webrev/
>>>>
>>>> thanks,
>>>> Calvin
>>>>
>>>> On 7/19/2013 7:22 AM, Daniel D. Daugherty wrote:
>>>>> On 7/18/13 6:12 PM, Calvin Cheung wrote:
>>>>>> Please review this small fix for a testcase.
>>>>>>
>>>>>> webrev:
>>>>>>     http://cr.openjdk.java.net/~ccheung/8020791/webrev/
>>>>>
>>>>> test/runtime/jsig/Test8017498.sh
>>>>>     line 54: export LD_PRELOAD=${LD_PRELOAD}
>>>>>         This export will affect every program run after this point.
>>>>>
>>>>>         Please change the variable name to MY_LD_PRELOAD" and don't
>>>>>         export it.
>>>>>
>>>>>     line 57: NULL=NUL
>>>>>     line 58: PS=";"
>>>>>     line 59: FS="\\"
>>>>>         These are not needed for your default case and they are
>>>>>         Windows specific. Your default case is non-Linux so these
>>>>>         are wrong in any case.
>>>>>
>>>>>     line 70: gcc -DLINUX -fPIC -shared -o 
>>>>> ${TESTSRC}${FS}libTestJNI.so ...
>>>>>         Please check the exit status of 'gcc' and do _something_
>>>>>         if the compile fails. You could even pass the test if there
>>>>>         is no compiler.
>>>>>
>>>>>     line 72: # run the java test in the background
>>>>>         Why do you have to run the test in the background?
>>>>>
>>>>>     line 73: echo ${TESTJAVA}${FS}bin${FS}java 
>>>>> -Djava.library.path=${TESTSRC}${FS} -server TestJNI 100 > test.out 
>>>>> 2>&1 &
>>>>>     line 74: ${TESTJAVA}${FS}bin${FS}java 
>>>>> -Djava.library.path=${TESTSRC}${FS} -server TestJNI 100 > test.out 
>>>>> 2>&1 &
>>>>>     line 75:
>>>>>     line 76: # obtain the process id
>>>>>     line 77: C_PID=$!
>>>>>
>>>>>         line 73 will run the echo in background and you're running 
>>>>> java in
>>>>>         the background. That might lead to a race for the C_PID 
>>>>> setting on
>>>>>         line 77.
>>>>>
>>>>>         Lines 73 and 74 would be better as:
>>>>>
>>>>>         cmd="LD_PRELOAD="$MY_LD_PRELOAD" \
>>>>>             ${TESTJAVA}${FS}bin${FS}java \
>>>>>             -Djava.library.path=${TESTSRC}${FS} -server TestJNI 100"
>>>>>         echo "$cmd > test.out 2>&1 &"
>>>>>         $cmd > test.out 2>&1 &
>>>>>
>>>>>     line 80: sleep 1
>>>>>         One second? This is just asking for intermittent failures of
>>>>>         this test. If you decide that you have to run this test in
>>>>>         background then something like this will be better:
>>>>>
>>>>>         count=0
>>>>>         # give the test 60 seconds to pass
>>>>>         while [ "$count" -lt 12 ]; do
>>>>>             sleep 5
>>>>>             grep "old handler" test.out > ${NULL}
>>>>>             if [ $? = 0 ]; then
>>>>>                 echo "Test Passed"
>>>>>                 exit 0
>>>>>             fi
>>>>>             count=`expr $count + 1`
>>>>>         done
>>>>>         kill -9 ${C_PID}
>>>>>         echo "Test Failed"
>>>>>         exit 1
>>>>>
>>>>>     line 82: # reset LD_PRELOAD
>>>>>     line 83: unset LD_PRELOAD
>>>>>         You don't need this if you switch to the MY_LD_PRELOAD logic
>>>>>         that I gave you above.
>>>>>
>>>>> test/runtime/jsig/TestJNI.c
>>>>>     line 45: pthread_attr_t attr;
>>>>>     line 46: stack_t stack;
>>>>>         These variables are not used.
>>>>>
>>>>> Dan
>>>>>
>>>>>
>>>>>
>>>>>>
>>>>>> Bug:
>>>>>> https://jbs.oracle.com/bugs/browse/JDK-8020791
>>>>>> http://bugs.sun.com/view_bug.do?bug_id=8020791
>>>>>>
>>>>>> When the testcase was developed, it was using an incomplete jni_md.h
>>>>>> without the following:
>>>>>> #if defined(SOLARIS) || defined(LINUX) || defined(_ALLBSD_SOURCE)
>>>>>> The default typedef long jlong will be used during compilation of 
>>>>>> the
>>>>>> native code of the testcase on linux_x64.
>>>>>>
>>>>>> With the above #if defined in the jni_md.h, if "LINUX" isn't 
>>>>>> defined,
>>>>>> the following typedef will be used and causing the compile error.
>>>>>>     typedef __int64 jlong;
>>>>>>
>>>>>> The fix is to define LINUX (-DLINUX) in the gcc command line.
>>>>>> The change in TestJNI.c is just a minor cleanup.
>>>>>>
>>>>>> Test:
>>>>>>    Ran jtreg on the testcase.
>>>>>>
>>>>>> thanks,
>>>>>> Calvin
>>>>>
>>>>
>>>
>>
>