RFR: 6536943: Bogus -Xcheck:jni warning for SIG_INT action for SIGINT in JVM started from non-interactive shell
Yumin Qi
yumin.qi at oracle.com
Fri Mar 14 19:57:59 UTC 2014
Thanks for your review, I will change in the webrev and push work too.
No further notifications.
Yumin
On 3/14/2014 12:56 PM, Karen Kinnear wrote:
> Yumin,
>
> Thank you so much - I like this one better.
>
> Can you please fix two things in the comment lines.
> non-reactive is non-interactive
> SIGIGN is SIG_IGN
>
> thanks,
> Karen
>
> On Mar 14, 2014, at 2:35 PM, 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
More information about the hotspot-runtime-dev
mailing list