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

[Karen Kinnear]

#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
>>> 
>> 
>