RFR: 6536943: Bogus -Xcheck:jni warning for SIG_INT action for SIGINT in JVM started from non-interactive shell

Fri Mar 14 19:07:13 UTC 2014

Yumin,

This version is OK for me.

-Dmitry

On 2014-03-14 22:35, Yumin Qi wrote:
> Karen and all,
> 
>   http://cr.openjdk.java.net/~minqi/6536943/webrev03/
> 
>   This version only still keeps original version of checking for signal
> plus more info printout like:
> 
> Warning: SIGINT handler expected:libjvm.so+0x894d30
> found:0x0000000000000001
> Running in non-interactive shell, SIGINT handler is replaced by shell  
> <--------------------------- newly added output
> Signal Handlers:
> SIGSEGV: [libjvm.so+0x892040],
> sa_mask[0]=11111111011111111101111111111110, sa_flags=SA_RESTART|SA_SIGINFO
> SIGBUS: [libjvm.so+0x892040],
> sa_mask[0]=11111111011111111101111111111110, sa_flags=SA_RESTART|SA_SIGINFO
> SIGFPE: [libjvm.so+0x892040],
> sa_mask[0]=11111111011111111101111111111110, sa_flags=SA_RESTART|SA_SIGINFO
> SIGPIPE: [libjvm.so+0x892040],
> sa_mask[0]=11111111011111111101111111111110, sa_flags=SA_RESTART|SA_SIGINFO
> SIGXFSZ: [libjvm.so+0x892040],
> sa_mask[0]=11111111011111111101111111111110, sa_flags=SA_RESTART|SA_SIGINFO
> SIGILL: [libjvm.so+0x892040],
> sa_mask[0]=11111111011111111101111111111110, sa_flags=SA_RESTART|SA_SIGINFO
> SIGUSR1: SIG_DFL, sa_mask[0]=00000000000000000000000000000000,
> sa_flags=none
> SIGUSR2: [libjvm.so+0x893710],
> sa_mask[0]=00000000000000000000000000000000, sa_flags=SA_RESTART|SA_SIGINFO
> SIGHUP: [libjvm.so+0x894d30],
> sa_mask[0]=11111111011111111101111111111110, sa_flags=SA_RESTART|SA_SIGINFO
> SIGINT: SIG_IGN, sa_mask[0]=00000000000000000000000000000000,
> sa_flags=none <------------------------------------------------- user
> can get hint from here
> SIGTERM: [libjvm.so+0x894d30],
> sa_mask[0]=11111111011111111101111111111110, sa_flags=SA_RESTART|SA_SIGINFO
> SIGQUIT: [libjvm.so+0x894d30],
> sa_mask[0]=11111111011111111101111111111110, sa_flags=SA_RESTART|SA_SIGINFO
> 
> Let me know if it is OK.
> 
> Thanks
> Yumin
> 
> On 3/14/2014 10:22 AM, Karen Kinnear wrote:
>> Yumin,
>>
>> I had the same question.
>>
>> Another possibility would be to leave the check in, and to add a comment
>> after reporting SIGINT as SIG_IGN that this could be an artifact of
>> running in a non-interactive shell
>>
>> opinions?
>>
>> thanks,
>> Karen
>>
>> On Mar 14, 2014, at 1:17 PM, Yumin Qi wrote:
>>
>>> Karen,
>>>
>>>   I agree only report non-interactive shell warning --- if under
>>> other case user replace the signal handler, we should give warning.
>>> This lets me think of the fix one more time, do we really want to
>>> silent for non-reactive shell? It is no harm for customer knows what
>>> happened --- it is the shell replace the handler. If user understand
>>> that, it is OK I think.
>>>
>>> Thanks
>>> Yumin
>>>
>>> On 3/14/2014 10:10 AM, Karen Kinnear wrote:
>>>> #define SHUTDOWN_SIGNAL1 SIGINT
>>>>
>>>> I also wondered about checking if the current SIGINT interrupt
>>>> handler setting was
>>>> SIG_IGN.
>>>>
>>>> The question is - do we want to only not report for non-interactive
>>>> or for anyone who
>>>> has set SIG_IGN? I was leaning in the direction of non-interactive.
>>>>
>>>> thanks
>>>> Karen
>>>>
>>>> On Mar 14, 2014, at 12:57 PM, Yumin Qi wrote:
>>>>
>>>>> Dmitry,
>>>>>
>>>>>   #define INTERRUPT_SIGNAL SIGUSR1
>>>>>
>>>>>    I think this is not SIGINT.
>>>>>
>>>>> Thanks
>>>>> Yumin
>>>>>
>>>>> On 3/14/2014 9:27 AM, Yumin Qi wrote:
>>>>>> Dmitry,
>>>>>>
>>>>>>     case INTERRUPT_SIGNAL:
>>>>>>     jvmHandler = CAST_FROM_FN_PTR(address, SIG_DFL);
>>>>>>     break;
>>>>>>
>>>>>> Do you mean we use
>>>>>>     if (isatty(fileno(stdin)))
>>>>>>        jvmHandler = CAST_FROM_FN_PTR(address, SIG_DFL);
>>>>>>     else
>>>>>>        jvmHandler = CAST_FROM_FN_PTR(address, SIG_IGN);
>>>>>>
>>>>>> I think this will solve the problem. Need more opinions.
>>>>>>
>>>>>> Under redirecting,  isatty still can correctly tests for
>>>>>> non-interactive shell, please check bug comments.
>>>>>>
>>>>>> Thanks
>>>>>> Yumin
>>>>>>
>>>>>>
>>>>>> On 3/14/2014 9:05 AM, Dmitry Samersoff wrote:
>>>>>>> Yumin.
>>>>>>>
>>>>>>> I find the only place per os_*.cpp where we check for SIGINT
>>>>>>>
>>>>>>> os/linux/vm/os_linux.cpp
>>>>>>>
>>>>>>> 4498:  DO_SIGNAL_CHECK(INTERRUPT_SIGNAL);
>>>>>>>
>>>>>>> so it might make sense to place it under
>>>>>>>
>>>>>>>    if isatty() {}
>>>>>>>
>>>>>>> rather than change macro.
>>>>>>>
>>>>>>> Also isatty() call is expensive and will not give you expected
>>>>>>> result if
>>>>>>> stdin is redirected (e.g. by custom launcher)
>>>>>>>
>>>>>>>
>>>>>>> I think it's safe to check whether the handler for SIGINT is SIG_IGN
>>>>>>> and skip the check in this case.
>>>>>>>
>>>>>>> -Dmitry
>>>>>>>
>>>>>>>
>>>>>>> On 2014-03-14 08:18, Yumin Qi wrote:
>>>>>>>> Have changed, new URL at:
>>>>>>>> http://cr.openjdk.java.net/~minqi/6536943/webrev02
>>>>>>>>
>>>>>>>> Thanks
>>>>>>>> Yumin
>>>>>>>>
>>>>>>>> On 3/13/2014 4:10 PM, Yumin Qi wrote:
>>>>>>>>> Thanks for review. I will change the order.
>>>>>>>>>
>>>>>>>>> Yumin
>>>>>>>>>
>>>>>>>>> On 3/13/2014 3:28 PM, Dean Long wrote:
>>>>>>>>>> (isatty(fileno(stdin)) || sig != SIGINT)
>>>>>>>>>>
>>>>>>>>>> You might want to reverse that:
>>>>>>>>>>
>>>>>>>>>> (sig != SIGINT || isatty(fileno(stdin)))
>>>>>>>>>>
>>>>>>>>>> because isatty() could be expensive.
>>>>>>>>>>
>>>>>>>>>> dl
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On 3/13/2014 2:27 PM, Yumin Qi wrote:
>>>>>>>>>>> David and all,
>>>>>>>>>>>
>>>>>>>>>>>    I have changed to skip SIGINT only if run in a
>>>>>>>>>>> non-interactive
>>>>>>>>>>> shell. Please check webrev:
>>>>>>>>>>>
>>>>>>>>>>> http://cr.openjdk.java.net/~minqi/6536943/webrev01/
>>>>>>>>>>>
>>>>>>>>>>> Thanks
>>>>>>>>>>> Yumin
>>>>>>>>>>>
>>>>>>>>>>> On 3/13/2014 8:19 AM, Yumin Qi wrote:
>>>>>>>>>>>> On 3/12/2014 10:08 PM, David Holmes wrote:
>>>>>>>>>>>>> Hi Yumin,
>>>>>>>>>>>>>
>>>>>>>>>>>>> Not sure that disabling all signal checks because there is a
>>>>>>>>>>>>> problem with one signal is the right solution for this.
>>>>>>>>>>>>>
>>>>>>>>>>>> Or we check if the signal is SIGINT --- skip it only.
>>>>>>>>>>>>
>>>>>>>>>>>> Thanks
>>>>>>>>>>>> Yumin
>>>>>>>>>>>>> That said I don't know what utility the signal checks
>>>>>>>>>>>>> provide in
>>>>>>>>>>>>> the first place. Can anyone comment on that?
>>>>>>>>>>>>>
>>>>>>>>>>>>> David
>>>>>>>>>>>>>
>>>>>>>>>>>>> On 13/03/2014 9:55 AM, Yumin Qi wrote:
>>>>>>>>>>>>>> Forget to mention tests:
>>>>>>>>>>>>>> JPRT test;
>>>>>>>>>>>>>> manually testing on Java2Demo, for both shell and
>>>>>>>>>>>>>> non-interactive
>>>>>>>>>>>>>> shell.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> This is only disabling checking signal handlers under
>>>>>>>>>>>>>> non-interactive
>>>>>>>>>>>>>> shell, so no need to test more.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Thanks
>>>>>>>>>>>>>> Yumin
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> On 3/12/2014 2:52 PM, Yumin Qi wrote:
>>>>>>>>>>>>>>> Hi, Can I have your codereview for this simple change:
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> http://cr.openjdk.java.net/~minqi/6536943/webrev00/
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Summary: Under non-interactive shell, -Xcheck:jni output
>>>>>>>>>>>>>>> warning for
>>>>>>>>>>>>>>> SIGINT, since non-interactive shells set SIGINT to
>>>>>>>>>>>>>>> SIG_IGN as
>>>>>>>>>>>>>>> part of
>>>>>>>>>>>>>>> the job control features. The fix is that don't check signal
>>>>>>>>>>>>>>> handlers
>>>>>>>>>>>>>>> if java run in a non-interactive shell. This applies to all
>>>>>>>>>>>>>>> platforms
>>>>>>>>>>>>>>> except for Windows.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Thanks
>>>>>>>>>>>>>>> Yumin
> 

-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.