[PATCH] linux-record: Squash cases with identical handling

[PATCH] linux-record: Squash cases with identical handling

[Andreas Arnez]

While discussing a fix for a bad fall-through in linux-record.c, it was
pointed out that the cases for gdb_sys_pipe2 and gdb_sys_pipe can be
squashed into one.  Thus I promised a minor cleanup for cases with
identical handling:

  https://sourceware.org/ml/gdb-patches/2016-03/msg00310.html

I've had this in my pipe for some time, so here it is.  There should not
be any functional change.

-- >8 --
Subject: [PATCH] linux-record: Squash cases with identical handling

In record_linux_system_call there are some cases with identical
handling.  These are merged together to reduce code duplication.

gdb/ChangeLog:

	* linux-record.c (record_linux_system_call): Merge handling for
	readlink/recv/read and pipe/pipe2.  Also move handling for ppoll
	before the case for poll and use fall-through logic.
---
 gdb/linux-record.c | 40 ++++++++--------------------------------
 1 file changed, 8 insertions(+), 32 deletions(-)

diff --git a/gdb/linux-record.c b/gdb/linux-record.c
index fda7ada..6b84a18 100644
--- a/gdb/linux-record.c
+++ b/gdb/linux-record.c
@@ -264,6 +264,8 @@ record_linux_system_call (enum gdb_syscall syscall,
       break;
 
     case gdb_sys_read:
+    case gdb_sys_readlink:
+    case gdb_sys_recv:
       regcache_raw_read_unsigned (regcache, tdep->arg3, &tmpulongest);
       if (record_mem_at_reg (regcache, tdep->arg2, (int) tmpulongest))
 	return -1;
@@ -348,6 +350,7 @@ record_linux_system_call (enum gdb_syscall syscall,
       break;
 
     case gdb_sys_pipe:
+    case gdb_sys_pipe2:
       if (record_mem_at_reg (regcache, tdep->arg1, tdep->size_int * 2))
 	return -1;
       break;
@@ -645,12 +648,6 @@ record_linux_system_call (enum gdb_syscall syscall,
     case gdb_sys_symlink:
       break;
 
-    case gdb_sys_readlink:
-      regcache_raw_read_unsigned (regcache, tdep->arg3, &tmpulongest);
-      if (record_mem_at_reg (regcache, tdep->arg2, (int) tmpulongest))
-	return -1;
-      break;
-
     case gdb_sys_uselib:
     case gdb_sys_swapon:
       break;
@@ -742,12 +739,6 @@ Do you want to stop the program?"),
       }
       break;
 
-    case gdb_sys_recv:
-      regcache_raw_read_unsigned (regcache, tdep->arg3, &tmpulongest);
-      if (record_mem_at_reg (regcache, tdep->arg2, (int) tmpulongest))
-	return -1;
-      break;
-
     case gdb_sys_recvmsg:
       regcache_raw_read_unsigned (regcache, tdep->arg2, &tmpulongest);
       if (record_linux_msghdr (regcache, tdep, tmpulongest))
@@ -1364,6 +1355,11 @@ Do you want to stop the program?"),
     case gdb_sys_ni_syscall167:
       break;
 
+    case gdb_sys_ppoll:
+      if (record_mem_at_reg (regcache, tdep->arg3, tdep->size_timespec))
+	return -1;
+      /* Fall through.  */
+
     case gdb_sys_poll:
       regcache_raw_read_unsigned (regcache, tdep->arg1, &tmpulongest);
       if (tmpulongest)
@@ -1959,21 +1955,6 @@ Do you want to stop the program?"),
 	return -1;
       break;
 
-    case gdb_sys_ppoll:
-      regcache_raw_read_unsigned (regcache, tdep->arg1, &tmpulongest);
-      if (tmpulongest)
-	{
-	  ULONGEST nfds;
-
-	  regcache_raw_read_unsigned (regcache, tdep->arg2, &nfds);
-	  if (record_full_arch_list_add_mem ((CORE_ADDR) tmpulongest,
-					     tdep->size_pollfd * nfds))
-	    return -1;
-	}
-      if (record_mem_at_reg (regcache, tdep->arg3, tdep->size_timespec))
-	return -1;
-      break;
-
     case gdb_sys_unshare:
     case gdb_sys_set_robust_list:
       break;
@@ -2035,11 +2016,6 @@ Do you want to stop the program?"),
     case gdb_sys_dup3:
       break;
 
-    case gdb_sys_pipe2:
-      if (record_mem_at_reg (regcache, tdep->arg1, tdep->size_int * 2))
-	return -1;
-      break;
-
     case gdb_sys_inotify_init1:
       break;
 
-- 
2.5.0

Re: [PATCH] linux-record: Squash cases with identical handling

[Yao Qi]

Andreas Arnez <arnez@linux.vnet.ibm.com> writes:

Hi Andreas,

> While discussing a fix for a bad fall-through in linux-record.c, it was
> pointed out that the cases for gdb_sys_pipe2 and gdb_sys_pipe can be
> squashed into one.  Thus I promised a minor cleanup for cases with
> identical handling:
>
>   https://sourceware.org/ml/gdb-patches/2016-03/msg00310.html

I thought about squashing them too, but the reason I didn't do that is
these enum gdb_syscall in the switch block are listed in the numeric
order, so that it is quite easy to find whether a syscall is supported,
or add a new syscall.

I am not against your patch, but want to let people know why the code is
written that way, so that people can judge which one is better.

>  
>      case gdb_sys_read:
> +    case gdb_sys_readlink:
> +    case gdb_sys_recv:
>        regcache_raw_read_unsigned (regcache, tdep->arg3, &tmpulongest);
>        if (record_mem_at_reg (regcache, tdep->arg2, (int) tmpulongest))
>  	return -1;
> @@ -348,6 +350,7 @@ record_linux_system_call (enum gdb_syscall syscall,
>        break;
>  
>      case gdb_sys_pipe:
> +    case gdb_sys_pipe2:
>        if (record_mem_at_reg (regcache, tdep->arg1, tdep->size_int * 2))
>  	return -1;
>        break;

I don't mind this...  anyway, it can shorten the code.


> @@ -1364,6 +1355,11 @@ Do you want to stop the program?"),
>      case gdb_sys_ni_syscall167:
>        break;
>  
> +    case gdb_sys_ppoll:
> +      if (record_mem_at_reg (regcache, tdep->arg3, tdep->size_timespec))
> +	return -1;
> +      /* Fall through.  */
> +
>      case gdb_sys_poll:
>        regcache_raw_read_unsigned (regcache, tdep->arg1, &tmpulongest);
>        if (tmpulongest)
> @@ -1959,21 +1955,6 @@ Do you want to stop the program?"),

but, I don't like the fall-through.

-- 
Yao (齐尧)

Re: [PATCH] linux-record: Squash cases with identical handling

[Andreas Arnez]

On Wed, Apr 13 2016, Yao Qi wrote:

> I thought about squashing them too, but the reason I didn't do that is
> these enum gdb_syscall in the switch block are listed in the numeric
> order, so that it is quite easy to find whether a syscall is supported,
> or add a new syscall.

Ah, interesting point.  If we want to stick to this rule, maybe this
should be stated in a comment above the switch statement?

Or should we aim at getting GDB '-Wswitch'-clean?  (Probably a good idea
anyhow...)  Then we could replace the default label by explicit case
labels for all unsupported syscalls and rely on GCC to catch any further
missing case labels.  Once that's done, the order of case labels
wouldn't matter, IMO.

> but, I don't like the fall-through.

Yeah, it's kind of ugly.  I can certainly drop this change from the
patch if that helps.

--
Andreas

Re: [PATCH] linux-record: Squash cases with identical handling

[Yao Qi]

Andreas Arnez <arnez@linux.vnet.ibm.com> writes:

> On Wed, Apr 13 2016, Yao Qi wrote:
>
>> I thought about squashing them too, but the reason I didn't do that is
>> these enum gdb_syscall in the switch block are listed in the numeric
>> order, so that it is quite easy to find whether a syscall is supported,
>> or add a new syscall.
>
> Ah, interesting point.  If we want to stick to this rule, maybe this
> should be stated in a comment above the switch statement?
>

It is not my intention to stick to this rule.

> Or should we aim at getting GDB '-Wswitch'-clean?  (Probably a good idea

-Wswitch is enabled by -Wall, so gdb is '-Wswitch'-clean already.

> anyhow...)  Then we could replace the default label by explicit case
> labels for all unsupported syscalls and rely on GCC to catch any further
> missing case labels.  Once that's done, the order of case labels
> wouldn't matter, IMO.
>

That will be overkill compared with your patch, so ...

>> but, I don't like the fall-through.
>
> Yeah, it's kind of ugly.  I can certainly drop this change from the
> patch if that helps.
>

... your patch except the fall-through is good to me.

-- 
Yao (齐尧)

Re: [PATCH] linux-record: Squash cases with identical handling

[Andreas Arnez]

On Thu, Apr 14 2016, Yao Qi wrote:

> Andreas Arnez <arnez@linux.vnet.ibm.com> writes:
>
>> On Wed, Apr 13 2016, Yao Qi wrote:
>>
>>> I thought about squashing them too, but the reason I didn't do that is
>>> these enum gdb_syscall in the switch block are listed in the numeric
>>> order, so that it is quite easy to find whether a syscall is supported,
>>> or add a new syscall.
>>
>> Ah, interesting point.  If we want to stick to this rule, maybe this
>> should be stated in a comment above the switch statement?
>>
>
> It is not my intention to stick to this rule.
>
>> Or should we aim at getting GDB '-Wswitch'-clean?  (Probably a good idea
>
> -Wswitch is enabled by -Wall, so gdb is '-Wswitch'-clean already.

Well, GDB currently uses -Wno-switch.  And if I build GDB without that
option, I get lots of warnings for -Wswitch.

>
>> anyhow...)  Then we could replace the default label by explicit case
>> labels for all unsupported syscalls and rely on GCC to catch any further
>> missing case labels.  Once that's done, the order of case labels
>> wouldn't matter, IMO.
>>
>
> That will be overkill compared with your patch, so ...
>
>>> but, I don't like the fall-through.
>>
>> Yeah, it's kind of ugly.  I can certainly drop this change from the
>> patch if that helps.
>>
>
> ... your patch except the fall-through is good to me.

Thanks, pushed without that.

--
Andreas