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

[Calvin Cheung]

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.

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
>