svn commit: r333860 - head/sys/kern

[Warner Losh]

On Thu, May 24, 2018 at 12:53 AM, Matthew Macy <mmacy at freebsd.org> wrote:

> On Wed, May 23, 2018 at 11:42 PM, Michael Tuexen
> <Michael.Tuexen at macmic.franken.de> wrote:
> >> On 24. May 2018, at 08:36, Matthew Macy <mmacy at freebsd.org> wrote:
> >>
> >> On Wed, May 23, 2018 at 11:35 PM, Michael Tuexen
> >> <Michael.Tuexen at macmic.franken.de> wrote:
> >>>> On 24. May 2018, at 06:51, Matthew Macy <mmacy at freebsd.org> wrote:
> >>>>
> >>>> Warnings find bugs PERIOD. Although most are not useful, I've found
> >>> Some warnings indicate bugs, some warnings are just wrong. If you
> >>> have a "may be used uninitialized" warning being a false positive, you
> >>> may silences the warning by just set it to zero in the declaration and
> >>> you silence it. Other compilers might then correctly report an
> >>> assignment without affect...
> >>
> >> I have yet to see a double assignment be flagged as assignment without
> >> effect. If it _does_ occur then we have to disable the warning on the
> >> compiler that we have less faith in.
> > Have seen it in the past in a difference project... But you miss my
> point:
> >
> > Not all warnings indicate bugs PERIOD. Some warning are just wrong...
>
> Have you read my follow up? _Many_ Many warnings are wrong. Please
> respond to that on what the global policy should be. The value of any
> one particular instance of a warning does not merit discussion.
>

The global policy has never been 'fix all warnings no matter what.' It's
been 'Look at the warning. If it's a false positive, use judgement about
whether or not to stifle the compiler.' There are cases I've run into that
it was impossible to silence the warnings (apart form adding command line
stuff) for a particular bit of code. Do it one way gcc 4.2 complains. Do it
another clang complains. appease both and gcc 6 had heart-burn.

So don't gratuitously commit code that fixes warnings on gcc 8. If the
warning points out a legitimate bug, then that's no brainer yes. If it's a
false positive, then it's less clear and often times many factors may need
to be weighed.

Warner


> -M
>
>
>
>
>
>
>
>
>
>
>
> , in this case the assignment should be added with
> >>>>> a comment /* pacify gcc */.
> >>>>>
> >>>>> On Wed, May 23, 2018 at 03:59:33PM -0700, Matthew Macy wrote:
> >>>>> M> On Wed, May 23, 2018 at 3:57 PM, Gleb Smirnoff <
> glebius at freebsd.org> wrote:
> >>>>> M> > The initialization isn't useful.
> >>>>> M>
> >>>>> M> It silences a gcc warning. So yes it is. It's this exchange which
> is not useful.
> >>>>> M>
> >>>>> M> -M
> >>>>> M>
> >>>>> M>
> >>>>> M> > On Wed, May 23, 2018 at 03:52:42PM -0700, Matthew Macy wrote:
> >>>>> M> > M> Talk to the gcc devs. The warning is useful even if there
> are false positives.
> >>>>> M> > M>
> >>>>> M> > M> On Wed, May 23, 2018 at 3:27 PM, Gleb Smirnoff <
> glebius at freebsd.org> wrote:
> >>>>> M> > M> >   Hi,
> >>>>> M> > M> >
> >>>>> M> > M> > On Sat, May 19, 2018 at 05:10:52AM +0000, Matt Macy wrote:
> >>>>> M> > M> > M> Author: mmacy
> >>>>> M> > M> > M> Date: Sat May 19 05:10:51 2018
> >>>>> M> > M> > M> New Revision: 333860
> >>>>> M> > M> > M> URL: https://svnweb.freebsd.org/changeset/base/333860
> >>>>> M> > M> > M>
> >>>>> M> > M> > M> Log:
> >>>>> M> > M> > M>   sendfile: annotate unused value and ensure that
> npages is actually initialized
> >>>>> M> > M> > M>
> >>>>> M> > M> > M> Modified:
> >>>>> M> > M> > M>   head/sys/kern/kern_sendfile.c
> >>>>> M> > M> > M>
> >>>>> M> > M> > M> Modified: head/sys/kern/kern_sendfile.c
> >>>>> M> > M> > M> ==============================
> ================================================
> >>>>> M> > M> > M> --- head/sys/kern/kern_sendfile.c    Sat May 19
> 05:09:10 2018        (r333859)
> >>>>> M> > M> > M> +++ head/sys/kern/kern_sendfile.c    Sat May 19
> 05:10:51 2018        (r333860)
> >>>>> M> > M> > M> @@ -341,7 +341,7 @@ sendfile_swapin(vm_object_t obj,
> struct sf_io *sfio, o
> >>>>> M> > M> > M>      }
> >>>>> M> > M> > M>
> >>>>> M> > M> > M>      for (int i = 0; i < npages;) {
> >>>>> M> > M> > M> -            int j, a, count, rv;
> >>>>> M> > M> > M> +            int j, a, count, rv __unused;
> >>>>> M> > M> > M>
> >>>>> M> > M> > M>              /* Skip valid pages. */
> >>>>> M> > M> > M>              if (vm_page_is_valid(pa[i], vmoff(i, off)
> & PAGE_MASK,
> >>>>> M> > M> > M> @@ -688,6 +688,7 @@ retry_space:
> >>>>> M> > M> > M>                      if (space == 0) {
> >>>>> M> > M> > M>                              sfio = NULL;
> >>>>> M> > M> > M>                              nios = 0;
> >>>>> M> > M> > M> +                            npages = 0;
> >>>>> M> > M> > M>                              goto prepend_header;
> >>>>> M> > M> > M>                      }
> >>>>> M> > M> > M>                      hdr_uio = NULL;
> >>>>> M> > M> >
> >>>>> M> > M> > This initialization is redundant and a compiler warning if
> exists is wrong.
> >>>>> M> > M> >
> >>>>> M> > M> > If we jump down to prepend_header with nios == 0, we won't
> ever use npages.
> >>>>> M> > M> >
> >>>>> M> > M> > --
> >>>>> M> > M> > Gleb Smirnoff
> >>>>> M> >
> >>>>> M> > --
> >>>>> M> > Gleb Smirnoff
> >>>>>
> >>>>> --
> >>>>> Gleb Smirnoff
> >>>>
> >>>
> >
>
>