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

[Karen Kinnear]

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
>