Patchwork Add and move fall-through comments in system-specific code

login
register
mail settings
Submitter Joseph Myers
Date Feb. 18, 2019, 6:26 p.m.
Message ID <alpine.DEB.2.21.1902181826140.12080@digraph.polyomino.org.uk>
Download mbox | patch
Permalink /patch/31505/
State New
Headers show

Comments

Joseph Myers - Feb. 18, 2019, 6:26 p.m.
This patch fixes -Wimplicit-fallthrough warnings in system-specific
code that show up building glibc with -Wextra, by adding fall-through
comments, or moving existing such comments to the place required for
them to work (immediately before the case label being fallen through).

Tested with build-many-glibcs.py.

2019-02-18  Joseph Myers  <joseph@codesourcery.com>

	* sysdeps/i386/dl-machine.h (elf_machine_rela): Add fall-through
	comments.
	* sysdeps/m68k/m680x0/fpu/s_cexp_template.c (s(__cexp)): Likewise.
	* sysdeps/m68k/memcopy.h (WORD_COPY_FWD): Likewise.
	(WORD_COPY_BWD): Likewise.
	* sysdeps/mach/hurd/ioctl.c (__ioctl): Likewise.
	* sysdeps/powerpc/powerpc64/dl-machine.h (elf_machine_rela):
	Likewise.
	* sysdeps/s390/iso-8859-1_cp037_z900.c (TR_LOOP): Likewise.
	* sysdeps/mips/dl-machine.h (elf_machine_reloc): Move fall-through
	comment.
	* sysdeps/mips/dl-trampoline.c (__dl_runtime_resolve): Likewise.
Joseph Myers - Feb. 25, 2019, 1:24 p.m.
Ping.  This patch 
<https://sourceware.org/ml/libc-alpha/2019-02/msg00453.html> is pending 
review.
Gabriel F. T. Gomes - Feb. 26, 2019, 1:31 a.m.
On Mon, Feb 18 2019, Joseph Myers wrote:
> 
> 	* sysdeps/i386/dl-machine.h (elf_machine_rela): Add fall-through
> 	comments.
> 	* sysdeps/m68k/m680x0/fpu/s_cexp_template.c (s(__cexp)): Likewise.
> 	* sysdeps/m68k/memcopy.h (WORD_COPY_FWD): Likewise.
> 	(WORD_COPY_BWD): Likewise.
> 	* sysdeps/mach/hurd/ioctl.c (__ioctl): Likewise.
> 	* sysdeps/powerpc/powerpc64/dl-machine.h (elf_machine_rela):
> 	Likewise.
> 	* sysdeps/s390/iso-8859-1_cp037_z900.c (TR_LOOP): Likewise.
> 	* sysdeps/mips/dl-machine.h (elf_machine_reloc): Move fall-through
> 	comment.
> 	* sysdeps/mips/dl-trampoline.c (__dl_runtime_resolve): Likewise.

Looks good to me.

> diff --git a/sysdeps/i386/dl-machine.h b/sysdeps/i386/dl-machine.h
> index 13cb03a7ab..1566d1282a 100644
> --- a/sysdeps/i386/dl-machine.h
> +++ b/sysdeps/i386/dl-machine.h
> @@ -522,6 +522,7 @@ elf_machine_rela (struct link_map *map, const Elf32_Rela *reloc,
>  	case R_386_SIZE32:
>  	  /* Set to symbol size plus addend.  */
>  	  value = sym->st_size;
> +	  /* Fall through.  */
>  	case R_386_GLOB_DAT:
>  	case R_386_JMP_SLOT:
>  	case R_386_32:

Looks good (size + addend).

> diff --git a/sysdeps/m68k/m680x0/fpu/s_cexp_template.c b/sysdeps/m68k/m680x0/fpu/s_cexp_template.c
> index d214f5925b..13befb2d29 100644
> --- a/sysdeps/m68k/m680x0/fpu/s_cexp_template.c
> +++ b/sysdeps/m68k/m680x0/fpu/s_cexp_template.c
> @@ -93,6 +93,7 @@ s(__cexp) (CFLOAT x)
>  	      break;
>  	    case 2:
>  	      __real__ retval = -__real__ retval;
> +	      /* Fall through.  */

Looks good (the -/- quadrant).

>  	    case 3:
>  	      __imag__ retval = -__imag__ retval;
>  	      break;
> diff --git a/sysdeps/m68k/memcopy.h b/sysdeps/m68k/memcopy.h
> index 66c39649da..aa4a1ab651 100644
> --- a/sysdeps/m68k/memcopy.h
> +++ b/sysdeps/m68k/memcopy.h
> @@ -39,20 +39,28 @@
>  	do								      \
>  	  {								      \
>  	    ((op_t *) dst_bp)[0] = ((op_t *) src_bp)[0];		      \
> +	    /* Fall through.  */					      \
>  	  case 7:							      \
>  	    ((op_t *) dst_bp)[1] = ((op_t *) src_bp)[1];		      \
> +	    /* Fall through.  */					      \
>  	  case 6:							      \
>  	    ((op_t *) dst_bp)[2] = ((op_t *) src_bp)[2];		      \
> +	    /* Fall through.  */					      \
>  	  case 5:							      \
>  	    ((op_t *) dst_bp)[3] = ((op_t *) src_bp)[3];		      \
> +	    /* Fall through.  */					      \
>  	  case 4:							      \
>  	    ((op_t *) dst_bp)[4] = ((op_t *) src_bp)[4];		      \
> +	    /* Fall through.  */					      \
>  	  case 3:							      \
>  	    ((op_t *) dst_bp)[5] = ((op_t *) src_bp)[5];		      \
> +	    /* Fall through.  */					      \
>  	  case 2:							      \
>  	    ((op_t *) dst_bp)[6] = ((op_t *) src_bp)[6];		      \
> +	    /* Fall through.  */					      \
>  	  case 1:							      \
>  	    ((op_t *) dst_bp)[7] = ((op_t *) src_bp)[7];		      \
> +	    /* Fall through.  */					      \
>  	  case 0:							      \
>  	    src_bp += 32;						      \
>  	    dst_bp += 32;						      \

Looks good (copy 1, 2, ... or 8 items).

> @@ -73,20 +81,28 @@
>  	do								      \
>  	  {								      \
>  	    *--__dst_ep = *--__src_ep;					      \
> +	    /* Fall through.  */					      \
>  	  case 7:							      \
>  	    *--__dst_ep = *--__src_ep;					      \
> +	    /* Fall through.  */					      \
>  	  case 6:							      \
>  	    *--__dst_ep = *--__src_ep;					      \
> +	    /* Fall through.  */					      \
>  	  case 5:							      \
>  	    *--__dst_ep = *--__src_ep;					      \
> +	    /* Fall through.  */					      \
>  	  case 4:							      \
>  	    *--__dst_ep = *--__src_ep;					      \
> +	    /* Fall through.  */					      \
>  	  case 3:							      \
>  	    *--__dst_ep = *--__src_ep;					      \
> +	    /* Fall through.  */					      \
>  	  case 2:							      \
>  	    *--__dst_ep = *--__src_ep;					      \
> +	    /* Fall through.  */					      \
>  	  case 1:							      \
>  	    *--__dst_ep = *--__src_ep;					      \
> +	    /* Fall through.  */					      \
>  	  case 0:							      \
>  	    __nblocks--;						      \

Likewise.

>  	  }								      \
> diff --git a/sysdeps/mach/hurd/ioctl.c b/sysdeps/mach/hurd/ioctl.c
> index 625333110f..b0ad14a8e2 100644
> --- a/sysdeps/mach/hurd/ioctl.c
> +++ b/sysdeps/mach/hurd/ioctl.c
> @@ -177,6 +177,7 @@ __ioctl (int fd, unsigned long int request, ...)
>  	case MACH_SEND_INVALID_REPLY:
>  	case MACH_RCV_INVALID_NAME:
>  	  __mig_dealloc_reply_port (m->msgh_local_port);
> +	  /* Fall through.  */
>  	default:
>  	  return err;
>  	}

Looks correct for an invalid path.

> @@ -318,6 +319,7 @@ __ioctl (int fd, unsigned long int request, ...)
>      case EOPNOTSUPP:
>        /* The server didn't understand the RPC.  */
>        err = ENOTTY;
> +      /* Fall through.  */
>      default:
>        return __hurd_fail (err);
>      }

Likewise.

> diff --git a/sysdeps/mips/dl-machine.h b/sysdeps/mips/dl-machine.h
> index e82891fa3f..f9e7e90b41 100644
> --- a/sysdeps/mips/dl-machine.h
> +++ b/sysdeps/mips/dl-machine.h
> @@ -712,8 +712,8 @@ elf_machine_reloc (struct link_map *map, ElfW(Addr) r_info,
>  	 it's totally unnecessary.  */
>        if (ELFW(R_SYM) (r_info) == 0)
>  	break;
> -      /* Fall through.  */
>  #endif
> +      /* Fall through.  */
>      default:
>        _dl_reloc_bad_type (map, r_type, 0);
>        break;

OK (comment placement).

> diff --git a/sysdeps/mips/dl-trampoline.c b/sysdeps/mips/dl-trampoline.c
> index 568c8a10ce..5a8cc7dc56 100644
> --- a/sysdeps/mips/dl-trampoline.c
> +++ b/sysdeps/mips/dl-trampoline.c
> @@ -166,8 +166,8 @@ __dl_runtime_resolve (ElfW(Word) sym_index,
>  
>  		break;
>  	      }
> -	    /* Fall through.  */
>  	  }
> +	  /* Fall through.  */
>  	case 0:
>  	  {
>            /* We need to keep the scope around so do some locking.  This is

Likewise.

> diff --git a/sysdeps/powerpc/powerpc64/dl-machine.h b/sysdeps/powerpc/powerpc64/dl-machine.h
> index bc8bd0230e..1d926e3dff 100644
> --- a/sysdeps/powerpc/powerpc64/dl-machine.h
> +++ b/sysdeps/powerpc/powerpc64/dl-machine.h
> @@ -902,6 +902,7 @@ elf_machine_rela (struct link_map *map,
>      case R_PPC64_ADDR16_HI:
>        if (dont_expect (value + 0x80000000 >= 0x100000000LL))
>  	_dl_reloc_overflow (map, "R_PPC64_ADDR16_HI", reloc_addr, refsym);
> +      /* Fall through.  */
>      case R_PPC64_ADDR16_HIGH:
>        *(Elf64_Half *) reloc_addr = PPC_HI (value);
>        break;

OK. R_PPC64_ADDR16_HI reports overflows, whereas R_PPC64_ADDR16_HIGH
don't, as explained in the commit 7ec07d9a7b50.

> @@ -909,6 +910,7 @@ elf_machine_rela (struct link_map *map,
>      case R_PPC64_ADDR16_HA:
>        if (dont_expect (value + 0x80008000 >= 0x100000000LL))
>  	_dl_reloc_overflow (map, "R_PPC64_ADDR16_HA", reloc_addr, refsym);
> +      /* Fall through.  */
>      case R_PPC64_ADDR16_HIGHA:
>        *(Elf64_Half *) reloc_addr = PPC_HA (value);
>        break;

Likewise, for *HA and *HIGHA.

> diff --git a/sysdeps/s390/iso-8859-1_cp037_z900.c b/sysdeps/s390/iso-8859-1_cp037_z900.c
> index b2d8f62570..2a373fe124 100644
> --- a/sysdeps/s390/iso-8859-1_cp037_z900.c
> +++ b/sysdeps/s390/iso-8859-1_cp037_z900.c
> @@ -227,12 +227,19 @@ __attribute__ ((aligned (8))) =
>      switch (length)							\
>        {									\
>        case 7: outptr[6] = TABLE[inptr[6]];				\
> +	/* Fall through.  */						\
>        case 6: outptr[5] = TABLE[inptr[5]];				\
> +	/* Fall through.  */						\
>        case 5: outptr[4] = TABLE[inptr[4]];				\
> +	/* Fall through.  */						\
>        case 4: outptr[3] = TABLE[inptr[3]];				\
> +	/* Fall through.  */						\
>        case 3: outptr[2] = TABLE[inptr[2]];				\
> +	/* Fall through.  */						\
>        case 2: outptr[1] = TABLE[inptr[1]];				\
> +	/* Fall through.  */						\
>        case 1: outptr[0] = TABLE[inptr[0]];				\
> +	/* Fall through.  */						\
>        case 0: break;							\
>        }									\
>      inptr += length;							\
> 

Looks good (proccess 1, 2, .. or 7 items).

Patch

diff --git a/sysdeps/i386/dl-machine.h b/sysdeps/i386/dl-machine.h
index 13cb03a7ab..1566d1282a 100644
--- a/sysdeps/i386/dl-machine.h
+++ b/sysdeps/i386/dl-machine.h
@@ -522,6 +522,7 @@  elf_machine_rela (struct link_map *map, const Elf32_Rela *reloc,
 	case R_386_SIZE32:
 	  /* Set to symbol size plus addend.  */
 	  value = sym->st_size;
+	  /* Fall through.  */
 	case R_386_GLOB_DAT:
 	case R_386_JMP_SLOT:
 	case R_386_32:
diff --git a/sysdeps/m68k/m680x0/fpu/s_cexp_template.c b/sysdeps/m68k/m680x0/fpu/s_cexp_template.c
index d214f5925b..13befb2d29 100644
--- a/sysdeps/m68k/m680x0/fpu/s_cexp_template.c
+++ b/sysdeps/m68k/m680x0/fpu/s_cexp_template.c
@@ -93,6 +93,7 @@  s(__cexp) (CFLOAT x)
 	      break;
 	    case 2:
 	      __real__ retval = -__real__ retval;
+	      /* Fall through.  */
 	    case 3:
 	      __imag__ retval = -__imag__ retval;
 	      break;
diff --git a/sysdeps/m68k/memcopy.h b/sysdeps/m68k/memcopy.h
index 66c39649da..aa4a1ab651 100644
--- a/sysdeps/m68k/memcopy.h
+++ b/sysdeps/m68k/memcopy.h
@@ -39,20 +39,28 @@ 
 	do								      \
 	  {								      \
 	    ((op_t *) dst_bp)[0] = ((op_t *) src_bp)[0];		      \
+	    /* Fall through.  */					      \
 	  case 7:							      \
 	    ((op_t *) dst_bp)[1] = ((op_t *) src_bp)[1];		      \
+	    /* Fall through.  */					      \
 	  case 6:							      \
 	    ((op_t *) dst_bp)[2] = ((op_t *) src_bp)[2];		      \
+	    /* Fall through.  */					      \
 	  case 5:							      \
 	    ((op_t *) dst_bp)[3] = ((op_t *) src_bp)[3];		      \
+	    /* Fall through.  */					      \
 	  case 4:							      \
 	    ((op_t *) dst_bp)[4] = ((op_t *) src_bp)[4];		      \
+	    /* Fall through.  */					      \
 	  case 3:							      \
 	    ((op_t *) dst_bp)[5] = ((op_t *) src_bp)[5];		      \
+	    /* Fall through.  */					      \
 	  case 2:							      \
 	    ((op_t *) dst_bp)[6] = ((op_t *) src_bp)[6];		      \
+	    /* Fall through.  */					      \
 	  case 1:							      \
 	    ((op_t *) dst_bp)[7] = ((op_t *) src_bp)[7];		      \
+	    /* Fall through.  */					      \
 	  case 0:							      \
 	    src_bp += 32;						      \
 	    dst_bp += 32;						      \
@@ -73,20 +81,28 @@ 
 	do								      \
 	  {								      \
 	    *--__dst_ep = *--__src_ep;					      \
+	    /* Fall through.  */					      \
 	  case 7:							      \
 	    *--__dst_ep = *--__src_ep;					      \
+	    /* Fall through.  */					      \
 	  case 6:							      \
 	    *--__dst_ep = *--__src_ep;					      \
+	    /* Fall through.  */					      \
 	  case 5:							      \
 	    *--__dst_ep = *--__src_ep;					      \
+	    /* Fall through.  */					      \
 	  case 4:							      \
 	    *--__dst_ep = *--__src_ep;					      \
+	    /* Fall through.  */					      \
 	  case 3:							      \
 	    *--__dst_ep = *--__src_ep;					      \
+	    /* Fall through.  */					      \
 	  case 2:							      \
 	    *--__dst_ep = *--__src_ep;					      \
+	    /* Fall through.  */					      \
 	  case 1:							      \
 	    *--__dst_ep = *--__src_ep;					      \
+	    /* Fall through.  */					      \
 	  case 0:							      \
 	    __nblocks--;						      \
 	  }								      \
diff --git a/sysdeps/mach/hurd/ioctl.c b/sysdeps/mach/hurd/ioctl.c
index 625333110f..b0ad14a8e2 100644
--- a/sysdeps/mach/hurd/ioctl.c
+++ b/sysdeps/mach/hurd/ioctl.c
@@ -177,6 +177,7 @@  __ioctl (int fd, unsigned long int request, ...)
 	case MACH_SEND_INVALID_REPLY:
 	case MACH_RCV_INVALID_NAME:
 	  __mig_dealloc_reply_port (m->msgh_local_port);
+	  /* Fall through.  */
 	default:
 	  return err;
 	}
@@ -318,6 +319,7 @@  __ioctl (int fd, unsigned long int request, ...)
     case EOPNOTSUPP:
       /* The server didn't understand the RPC.  */
       err = ENOTTY;
+      /* Fall through.  */
     default:
       return __hurd_fail (err);
     }
diff --git a/sysdeps/mips/dl-machine.h b/sysdeps/mips/dl-machine.h
index e82891fa3f..f9e7e90b41 100644
--- a/sysdeps/mips/dl-machine.h
+++ b/sysdeps/mips/dl-machine.h
@@ -712,8 +712,8 @@  elf_machine_reloc (struct link_map *map, ElfW(Addr) r_info,
 	 it's totally unnecessary.  */
       if (ELFW(R_SYM) (r_info) == 0)
 	break;
-      /* Fall through.  */
 #endif
+      /* Fall through.  */
     default:
       _dl_reloc_bad_type (map, r_type, 0);
       break;
diff --git a/sysdeps/mips/dl-trampoline.c b/sysdeps/mips/dl-trampoline.c
index 568c8a10ce..5a8cc7dc56 100644
--- a/sysdeps/mips/dl-trampoline.c
+++ b/sysdeps/mips/dl-trampoline.c
@@ -166,8 +166,8 @@  __dl_runtime_resolve (ElfW(Word) sym_index,
 
 		break;
 	      }
-	    /* Fall through.  */
 	  }
+	  /* Fall through.  */
 	case 0:
 	  {
           /* We need to keep the scope around so do some locking.  This is
diff --git a/sysdeps/powerpc/powerpc64/dl-machine.h b/sysdeps/powerpc/powerpc64/dl-machine.h
index bc8bd0230e..1d926e3dff 100644
--- a/sysdeps/powerpc/powerpc64/dl-machine.h
+++ b/sysdeps/powerpc/powerpc64/dl-machine.h
@@ -902,6 +902,7 @@  elf_machine_rela (struct link_map *map,
     case R_PPC64_ADDR16_HI:
       if (dont_expect (value + 0x80000000 >= 0x100000000LL))
 	_dl_reloc_overflow (map, "R_PPC64_ADDR16_HI", reloc_addr, refsym);
+      /* Fall through.  */
     case R_PPC64_ADDR16_HIGH:
       *(Elf64_Half *) reloc_addr = PPC_HI (value);
       break;
@@ -909,6 +910,7 @@  elf_machine_rela (struct link_map *map,
     case R_PPC64_ADDR16_HA:
       if (dont_expect (value + 0x80008000 >= 0x100000000LL))
 	_dl_reloc_overflow (map, "R_PPC64_ADDR16_HA", reloc_addr, refsym);
+      /* Fall through.  */
     case R_PPC64_ADDR16_HIGHA:
       *(Elf64_Half *) reloc_addr = PPC_HA (value);
       break;
diff --git a/sysdeps/s390/iso-8859-1_cp037_z900.c b/sysdeps/s390/iso-8859-1_cp037_z900.c
index b2d8f62570..2a373fe124 100644
--- a/sysdeps/s390/iso-8859-1_cp037_z900.c
+++ b/sysdeps/s390/iso-8859-1_cp037_z900.c
@@ -227,12 +227,19 @@  __attribute__ ((aligned (8))) =
     switch (length)							\
       {									\
       case 7: outptr[6] = TABLE[inptr[6]];				\
+	/* Fall through.  */						\
       case 6: outptr[5] = TABLE[inptr[5]];				\
+	/* Fall through.  */						\
       case 5: outptr[4] = TABLE[inptr[4]];				\
+	/* Fall through.  */						\
       case 4: outptr[3] = TABLE[inptr[3]];				\
+	/* Fall through.  */						\
       case 3: outptr[2] = TABLE[inptr[2]];				\
+	/* Fall through.  */						\
       case 2: outptr[1] = TABLE[inptr[1]];				\
+	/* Fall through.  */						\
       case 1: outptr[0] = TABLE[inptr[0]];				\
+	/* Fall through.  */						\
       case 0: break;							\
       }									\
     inptr += length;							\