Re: [PATCH 4/4] Add -Wmisleading-indentation to -Wall

[David Malcolm <dmalcolm at redhat dot com>]

On Mon, 2015-11-02 at 13:39 -0500, Patrick Palka wrote:
> On Mon, 2 Nov 2015, David Malcolm wrote:
> 
> > On Sun, 2015-11-01 at 17:06 -0500, Patrick Palka wrote:
> >> On Fri, Oct 30, 2015 at 5:03 AM, Richard Biener
> >> <richard.guenther@gmail.com> wrote:
> >>> On Thu, Oct 29, 2015 at 6:38 PM, Jeff Law <law@redhat.com> wrote:
> >>>> On 10/29/2015 10:49 AM, David Malcolm wrote:
> >>>>>
> >>>>> Our documentation describes -Wall as enabling "all the warnings about
> >>>>> constructions that some users consider questionable, and that are easy to
> >>>>> avoid
> >>>>> (or modify to prevent the warning), even in conjunction with macros."
> >>>>>
> >>>>> I believe that -Wmisleading-indentation meets these criteria, and is
> >>>>> likely to be of benefit to users who may not read release notes; it
> >>>>> warns for indentation that's misleading, but not for indentation
> >>>>> that's merely bad: the former are places where a user will likely
> >>>>> want to fix the code.
> >>>>>
> >>>>> The fix is usually easy and obvious: fix the misleadingly-indented
> >>>>> code.  If that isn't an option for some reason, pragmas can be used to
> >>>>> turn off the warning for a particular fragment of code:
> >>>>>
> >>>>>    #pragma GCC diagnostic push
> >>>>>    #pragma GCC diagnostic ignored "-Wmisleading-indentation"
> >>>>>      if (flag)
> >>>>>        x = 3;
> >>>>>        y = 2;
> >>>>>    #pragma GCC diagnostic pop
> >>>>>
> >>>>> -Wmisleading-indentation has been tested with a variety of indentation
> >>>>> styles (see gcc/testsuite/c-c++-common/Wmisleading-indentation.c)
> >>>>> and on a variety of real-world projects.  For example, in:
> >>>>>    https://www.mail-archive.com/gcc-patches@gcc.gnu.org/msg119790.html
> >>>>> Patrick reports:
> >>>>> "Tested by building the linux, git, vim, sqlite and gdb-binutils sources
> >>>>>   with -Wmisleading-indentation."
> >>>>>
> >>>>> With the tweak earlier in this kit I believe we now have a good
> >>>>> enough signal:noise ratio for this warning to be widely used; hence this
> >>>>> patch adds the warning to -Wall.
> >>>>>
> >>>>> Bootstrapped&regrtested with x86_64-pc-linux-gnu.
> >>>>>
> >>>>> OK for trunk?
> >>>>>
> >>>>> gcc/c-family/ChangeLog:
> >>>>>         * c.opt (Wmisleading-indentation): Add to -Wall for C and C++.
> >>>>>
> >>>>> gcc/ChangeLog:
> >>>>>         * doc/invoke.texi (-Wall): Add -Wmisleading-indentation to the
> >>>>>         list.
> >>>>>         (-Wmisleading-indentation): Update documentation to reflect
> >>>>>         being enabled by -Wall in C/C++.
> >>>>
> >>>> I'm sure we'll get some grief for this :-)
> >>>>
> >>>> Approved once we're clean in GCC.  I'm going to explicitly say that we'll
> >>>> have to watch for fallout, particularly as we start getting feedback from
> >>>> Debian & Fedora mass-rebuilds as we approach release time.  If the fallout
> >>>> is too bad, we'll have to reconsider.
> >>>>
> >>>> I'll pre-approve patches which fix anything caught by this option in GCC as
> >>>> long as the fix just adjusts whitespace :-)
> >>>
> >>> Please at least check also binutils and gdb and other packages that use -Werror
> >>> (well, just rebuild Fedora world).
> >>
> >> FYI binutils, gdb and glibc, from git, all fail to build due to
> >> -Wmisleading-indentation warnings/errors. (None of the warnings are
> >> bogus (IMO), though I don't think any of the warnings expose a real
> >> bug either.)
> >
> > Bother.   Do you happen to have the logs handy? (or could you upload the
> > somewhere?)
> >
> > I tried building binutils+gdb 854eb72b00ba46d65ce36dc3432f01e223ce44cb
> > with gcc6+this kit (on x86_64) but I get:
> > In file included from ../../src/bfd/archive.c:143:0:
> > ../../src/bfd/../include/bfdlink.h:452:38: error: field âcompress_debugâ
> > has incomplete type
> >   enum compressed_debug_section_type compress_debug;
> >                                      ^
> > ../../src/bfd/archive.c: In function âopen_nested_fileâ:
> > ../../src/bfd/archive.c:393:12: error: âbfd {aka struct bfd}â has no
> > member named âlto_outputâ
> >       n_bfd->lto_output = archive->lto_output;
> >            ^
> > which appears to be unrelated snafu from the binutils+gdb side.
> >
> > Thanks
> > Dave
> >
> >
> 
> I don't have build logs but I have diffs that indicates where in the
> code the warnings appear and how to address the warnings (roughly).

Thanks.

> For binutils-gdb:
> 
> diff --git a/bfd/coff-i386.c b/bfd/coff-i386.c
> index a9725c4..fca7887 100644
> --- a/bfd/coff-i386.c
> +++ b/bfd/coff-i386.c
> @@ -139,7 +139,7 @@ coff_i386_reloc (bfd *abfd,
>   #define DOIT(x) \
>     x = ((x & ~howto->dst_mask) | (((x & howto->src_mask) + diff) & howto->dst_mask))
> 
> -    if (diff != 0)
> +  if (diff != 0)
>         {
>   	reloc_howto_type *howto = reloc_entry->howto;
>   	unsigned char *addr = (unsigned char *) data + reloc_entry->address;

This one has a fully blank line, so patch [1/4] would have suppressed
it.

> diff --git a/bfd/coff-x86_64.c b/bfd/coff-x86_64.c
> index 4e6420a..23ffecb 100644
> --- a/bfd/coff-x86_64.c
> +++ b/bfd/coff-x86_64.c
> @@ -138,7 +138,7 @@ coff_amd64_reloc (bfd *abfd,
>   #define DOIT(x) \
>     x = ((x & ~howto->dst_mask) | (((x & howto->src_mask) + diff) & howto->dst_mask))
> 
> -    if (diff != 0)
> +  if (diff != 0)
>         {
>   	reloc_howto_type *howto = reloc_entry->howto;
>   	unsigned char *addr = (unsigned char *) data + reloc_entry->address;

Likewise.

> diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
> index fff4862..2559a36 100644
> --- a/gdb/ada-lang.c
> +++ b/gdb/ada-lang.c
> @@ -11359,9 +11359,11 @@ ada_evaluate_subexp (struct type *expect_type, struct expression *exp,
>             return value_zero (ada_aligned_type (type), lval_memory);
>           }
>         else
> -        arg1 = ada_value_struct_elt (arg1, &exp->elts[pc + 2].string, 0);
> -        arg1 = unwrap_value (arg1);
> -        return ada_to_fixed_value (arg1);
> +	{
> +	  arg1 = ada_value_struct_elt (arg1, &exp->elts[pc + 2].string, 0);
> +	  arg1 = unwrap_value (arg1);
> +	  return ada_to_fixed_value (arg1);
> +	}
> 
>       case OP_TYPE:
>         /* The value is not supposed to be used.  This is here to make it

Interesting.  It's not technically a bug, since the "if true" clause has
an unconditional return, but it looks like a time-bomb to me.  I'm happy
that we warn for it.


> diff --git a/gdb/c-typeprint.c b/gdb/c-typeprint.c
> index 1af477c..1bd3b12 100644
> --- a/gdb/c-typeprint.c
> +++ b/gdb/c-typeprint.c
> @@ -1305,26 +1305,26 @@ c_type_print_base (struct type *type, struct ui_file *stream,
>   	      if (TYPE_NFIELDS (type) != 0 || TYPE_NFN_FIELDS (type) != 0)
>   		fprintf_filtered (stream, "\n");
> 
> -		for (i = 0; i < TYPE_TYPEDEF_FIELD_COUNT (type); i++)
> -		  {
> -		    struct type *target = TYPE_TYPEDEF_FIELD_TYPE (type, i);
> -
> -		    /* Dereference the typedef declaration itself.  */
> -		    gdb_assert (TYPE_CODE (target) == TYPE_CODE_TYPEDEF);
> -		    target = TYPE_TARGET_TYPE (target);
> -
> -		    print_spaces_filtered (level + 4, stream);
> -		    fprintf_filtered (stream, "typedef ");
> -
> -		    /* We want to print typedefs with substitutions
> -		       from the template parameters or globally-known
> -		       typedefs but not local typedefs.  */
> -		    c_print_type (target,
> -				  TYPE_TYPEDEF_FIELD_NAME (type, i),
> -				  stream, show - 1, level + 4,
> -				  &semi_local_flags);
> -		    fprintf_filtered (stream, ";\n");
> -		  }
> +	      for (i = 0; i < TYPE_TYPEDEF_FIELD_COUNT (type); i++)
> +		{
> +		  struct type *target = TYPE_TYPEDEF_FIELD_TYPE (type, i);
> +
> +		  /* Dereference the typedef declaration itself.  */
> +		  gdb_assert (TYPE_CODE (target) == TYPE_CODE_TYPEDEF);
> +		  target = TYPE_TARGET_TYPE (target);
> +
> +		  print_spaces_filtered (level + 4, stream);
> +		  fprintf_filtered (stream, "typedef ");
> +
> +		  /* We want to print typedefs with substitutions
> +		     from the template parameters or globally-known
> +		     typedefs but not local typedefs.  */
> +		  c_print_type (target,
> +				TYPE_TYPEDEF_FIELD_NAME (type, i),
> +				stream, show - 1, level + 4,
> +				&semi_local_flags);
> +		  fprintf_filtered (stream, ";\n");
> +		}
>   	      }

Likewise, the blank line means patch [1/4] would have suppressed it.

>   	    fprintfi_filtered (level, stream, "}");
> diff --git a/gdb/inflow.c b/gdb/inflow.c
> index d38a43d..87988ea 100644
> --- a/gdb/inflow.c
> +++ b/gdb/inflow.c
> @@ -413,7 +413,7 @@ child_terminal_ours_1 (int output_only)
>     if (tinfo->run_terminal != NULL || gdb_has_a_terminal () == 0)
>       return;
> 
> -    {
> +  {
>   #ifdef SIGTTOU
>         /* Ignore this signal since it will happen when we try to set the
>            pgrp.  */
> @@ -497,7 +497,7 @@ child_terminal_ours_1 (int output_only)
>         result = fcntl (0, F_SETFL, our_terminal_info.tflags);
>         result = fcntl (0, F_SETFL, our_terminal_info.tflags);
>   #endif
> -    }
> +  }
>   }

again, another blank line.

>   /* Per-inferior data key.  */
> diff --git a/gdb/linux-record.c b/gdb/linux-record.c
> index 091ac8a..18f8fbf 100644
> --- a/gdb/linux-record.c
> +++ b/gdb/linux-record.c
> @@ -112,7 +112,7 @@ record_linux_sockaddr (struct regcache *regcache,
>                               "memory at addr = 0x%s len = %d.\n",
>                               phex_nz (len, tdep->size_pointer),
>                               tdep->size_int);
> -        return -1;
> +      return -1;
>       }
>     addrlen = (int) extract_unsigned_integer (a, tdep->size_int, byte_order);
>     if (addrlen <= 0 || addrlen > tdep->size_sockaddr)

[...snip...]

These three are all of the form:
   if (record_debug)
     fprint (...multiple lines...);
     return -1;

It seems reasonable to me to complain about the indentation of the
return -1;


> ====
> 
> And for glibc:
> 
> diff --git a/stdio-common/vfscanf.c b/stdio-common/vfscanf.c
> index 1382eb5..1963c31 100644
> --- a/stdio-common/vfscanf.c
> +++ b/stdio-common/vfscanf.c
> @@ -1711,7 +1711,7 @@ _IO_vfscanf_internal (_IO_FILE *s, const char *format, _IO_va_list argptr,
> 
>                        /* The last thousands character will be added back by
>                           the char_buffer_add below.  */
> -                       --charbuf.current;
> +                     --charbuf.current;
>   #endif
>                      }
>                    else

Another one where there's a blank line, which 1/4 would have suppressed.

> diff --git a/stdlib/strtol_l.c b/stdlib/strtol_l.c
> index 8f6163d..0fb9a92 100644
> --- a/stdlib/strtol_l.c
> +++ b/stdlib/strtol_l.c
> @@ -351,8 +351,8 @@ INTERNAL (__strtol_l) (const STRING_TYPE *nptr, STRING_TYPE **endptr,
>                  && (wchar_t) c != thousands
>   # else
>                  && ({ for (cnt = 0; cnt < thousands_len; ++cnt)
> -                     if (thousands[cnt] != end[cnt])
> -                       break;
> +                       if (thousands[cnt] != end[cnt])
> +                         break;
>                        cnt < thousands_len; })
>   # endif
>                  && (!ISALPHA (c)

This is the one I filed earlier today as PR 68187.

> diff --git a/sysdeps/ieee754/flt-32/k_rem_pio2f.c b/sysdeps/ieee754/flt-32/k_rem_pio2f.c
> index 0c7685c..a0d844c 100644
> --- a/sysdeps/ieee754/flt-32/k_rem_pio2f.c
> +++ b/sysdeps/ieee754/flt-32/k_rem_pio2f.c
> @@ -65,7 +65,8 @@ int __kernel_rem_pio2f(float *x, float *y, int e0, int nx, int prec, const int32
> 
>       /* compute q[0],q[1],...q[jk] */
>          for (i=0;i<=jk;i++) {
> -           for(j=0,fw=0.0;j<=jx;j++) fw += x[j]*f[jx+i-j]; q[i] = fw;
> +           for(j=0,fw=0.0;j<=jx;j++) fw += x[j]*f[jx+i-j];
> +           q[i] = fw;
>          }
> 
>          jz = jk;

I think it's very reasonable to complain about the above code.

So if I've counted things right the tally is:
* 5 dubious-looking though correct places, where it's reasonable to
issue a warning
* 5 places where there's a blank line between guarded and non-guarded
stmt, where patch 1 of the kit would have suppressed the warning
* one bug (PR 68187)

I think we want the first kind of thing at -Wall, but I'm not so keen on
the second kind at -Wall.  Is there precedent for "levels" of a warning?
(so e.g. pedantry level 1 at -Wall, level 2 at -Wextra, and have patch 1
be the difference between levels 1 and 2?)

Sorry for harping on about patch 1; thanks again for posting this

Dave