Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20201205143250.2378b9f9@kicinski-fedora-pc1c0hjn.DHCP.thefacebook.com>
In-Reply-To: <20201205143250.2378b9f9@kicinski-fedora-pc1c0hjn.DHCP.thefacebook.com>
From:   Linus Torvalds <torvalds@linux-foundation.org>
Date:   Sat, 5 Dec 2020 15:10:15 -0800
Message-ID: <CAHk-=wjQYjRusiGfXTywSg5xC8knhP6uesDfO3J=bgu5uevtyA@mail.gmail.com>
Subject: Re: sparse annotation for error types?
To:     Jakub Kicinski <kuba@kernel.org>
Cc:     Sparse Mailing-list <linux-sparse@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        edwin.peer@broadcom.com,
        Zhang Changzhong <zhangchangzhong@huawei.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Sat, Dec 5, 2020 at 2:34 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> Am I the only one who thinks this would be a good idea?

I don't think it's likely to be very useful, because a very common
pattern is to not have that separate "return 0" in the middle, but
more along the lines of

        err = first_step(obj, 1);
        if (err)
                return err;

        if (some_check(obj)) {
                err = -EINVAL; /* need explicit error set! */
                goto err_undo_1s;
        }

        err = second_step(obj, param);
        if (err)
                goto err_undo_1s;

        err = third_step(obj, 0);

   err_undo_2s:
        second_undo(obj);
   err_undo_1s:
        first_undo(obj);
        return err;

iow, the "undo" parts are often done even for the success cases. This
is particularly true when those first steps are locking-related, and
the code always wants to unlock.

Sparse also doesn't really do any value analysis, so I suspect it
wouldn't be trivial to implement in sparse anyway.

Syntactically, I also think it's wrong to annotate the variable - I
think the place to annotate would be the return statement, and say
"must be negative" there. Kind of similar to having function arguments
annotated as "must not be NULL" (which sparse also doesn't do, but
some other checking tools do, and sparse can at least _parse_
"__nonnull" even if it ends up being ignored).

It's a bit similar to gcc's has a "returns_nonnull" function
attribute, but that one is not "per return", it's a global "this
function cannot return NULL" thing so that callers can then be
optimized and NULL checks removed. So it's very similar to the
"argument is non-null" in that it's for warnings at the *caller*, not
the function itself.

So if we want sparse support for this, I'd suggest something more akin
to a smarter compile-time assert, IOW more like having a

        compile_time_assert(err < 0);
        return err;

and then sparse (or any other checker) could warn when there's a path
that results in "err" not being negative.

Having some kind of smarter compile-time assert could be useful in
general, but as mentioned, sparse doesn't really do value range
propagation right now, so..

Luc, any reactions?

          Linus