[PATCH] display mask on enter to sigreturn, not exit

[Denys Vlasenko]

On 01/11/2012 01:27 AM, Jamie Lokier wrote:
> Denys Vlasenko wrote:
>> On 01/10/2012 09:19 PM, Dmitry V. Levin wrote:
>>>> While at it, I made it so that we do display returned value/errno.
>>>> I see no point in hiding it and showing uninformative "= ?" instead.
>>>
>>> Is there any actual difference?  Is there any case when sigreturn() may
>>> "return" something different from EINTR?
>>
>> Yes. -EINTR return means that the interrupted system call won't be restarted.
>> It looks like this:
>>
>> rt_sigsuspend([ABRT])                   = ? ERESTARTNOHAND (To be restarted)
>> --- {si_signo=SIGALRM, si_code=SI_KERNEL, si_value={int=134513920, ptr=0x8048500}} (Alarm clock) ---
>> alarm(1)                                = 0
>> sigreturn() (mask now [ABRT])           = -1 EINTR (Interrupted system call)
>>
>> Whereas the case when syscall is restarted looks like this:
>>
>> read(0, 0x8121750, 1024)                = ? ERESTARTSYS (To be restarted)
>> --- {si_signo=SIGALRM, si_code=SI_KERNEL, si_value={int=134513920, ptr=0x8048500}} (Alarm clock) ---
>> alarm(1)                                = 0
>> sigreturn() (mask now [ABRT])           = 3
>> read(0, 0x8121750, 1024)...
>>
>> This is a potentially useful bit of information we weren't showing.
>
> You are showing it earlier as ERESTARTNOHAND vs. ERESTARTSYS.

Which reminds me: there is widespread confusion about exact meaning
of ERESTARTxyz codes. We show all four of them as "(To be restarted)".

I looked at kernel code and I propose the following
change which prints better explanation of these codes,
and contains verbose comments which explain *why* we display
code that way - or else someone confused
is bound to come later and mangle them again:

diff -d -urpN strace.5/syscall.c strace.6/syscall.c
--- strace.5/syscall.c	2012-01-09 13:24:57.122811923 +0100
+++ strace.6/syscall.c	2012-01-11 14:53:52.939894558 +0100
@@ -2412,17 +2412,51 @@ trace_syscall_exiting(struct tcb *tcp)
  	else if (!(sys_res & RVAL_NONE) && u_error) {
  		switch (u_error) {
  #ifdef LINUX
+		/* Blocked signals do not interrupt any syscalls.
+		 * In this case syscalls don't return ERESTARTfoo codes.
+		 *
+		 * Deadly signals set to SIG_DFL interrupt syscalls
+		 * and kill the process regardless of which of the codes below
+		 * is returned by the interrupted syscall.
+		 * In some cases, kernel forces a kernel-generated deadly
+		 * signal to be unblocked and set to SIG_DFL (and thus cause
+		 * death) if it is blocked or SIG_IGNed: for example, SIGSEGV
+		 * or SIGILL. (The alternative is to leave process spinning
+		 * forever on the faulty instruction - not useful).
+		 *
+		 * SIG_IGNed signals and non-deadly signals set to SIG_DFL
+		 * (for example, SIGCHLD, SIGWINCH) interrupt syscalls,
+		 * but kernel will always restart them.
+		 */
  		case ERESTARTSYS:
-			tprints("= ? ERESTARTSYS (To be restarted)");
+			/* Most common type of signal-interrupted syscall exit code.
+			 * The system call will be restarted with the same arguments
+			 * if SA_RESTART bit is set; otherwise, system call fails with EINTR.
+			 */
+			tprints("= ? ERESTARTSYS (To be restarted if SA_RESTART is set)");
  			break;
  		case ERESTARTNOINTR:
+			/* Rare. For example, fork() returns this if interrupted.
+			 * SA_RESTART is ignored (assumed set): the restart is unconditional.
+			 */
  			tprints("= ? ERESTARTNOINTR (To be restarted)");
  			break;
  		case ERESTARTNOHAND:
-			tprints("= ? ERESTARTNOHAND (To be restarted)");
+			/* pause(), rt_sigsuspend() etc use this code.
+			 * SA_RESTART is ignored (assumed not set):
+			 * syscall won't restart (will return EINTR instead)
+			 * even after signal with SA_RESTART set.
+			 * However, after SIG_IGN or SIG_DFL signal it will.
+			 */
+			tprints("= ? ERESTARTNOHAND (Interrupted by signal)");
  			break;
  		case ERESTART_RESTARTBLOCK:
-			tprints("= ? ERESTART_RESTARTBLOCK (To be restarted)");
+			/* Syscalls like nanosleep(), poll() which can't be restarted with their
+			 * original arguments use this code. Kernel will execute restart_syscall()
+			 * instead, which changes arguments before restarting the syscall.
+			 * SA_RESTART is ignored (assumed not set) similarly to ERESTARTNOHAND.
+			 */
+			tprints("= ? ERESTART_RESTARTBLOCK (Interrupted by signal)");
  			break;
  #endif /* LINUX */
  		default:

-- 
vda