Re: [RFC v2 02/10] Drop unused static function return values

[Alberto Faria]

On Wed, Aug 3, 2022 at 12:15 PM Richard W.M. Jones <rjones@redhat.com> wrote:
> If it helps to think about this, Coverity checks for consistency.
> Across the whole code base, is the return value of a function used or
> ignored consistently.  You will see Coverity errors like:
>
>       Error: CHECKED_RETURN (CWE-252): [#def37]
>       libnbd-1.12.5/fuse/operations.c:180: check_return: Calling "nbd_poll" 
> without checking return value (as is done elsewhere 5 out of 6 times).
>       libnbd-1.12.5/examples/aio-connect-read.c:96: example_checked: Example 
> 1: "nbd_poll(nbd, -1)" has its value checked in "nbd_poll(nbd, -1) == -1".
>       libnbd-1.12.5/examples/aio-connect-read.c:128: example_checked: Example 
> 2: "nbd_poll(nbd, -1)" has its value checked in "nbd_poll(nbd, -1) == -1".
>       libnbd-1.12.5/examples/strict-structured-reads.c:246: example_checked: 
> Example 3: "nbd_poll(nbd, -1)" has its value checked in "nbd_poll(nbd, -1) == 
> -1".
>       libnbd-1.12.5/ocaml/nbd-c.c:2599: example_assign: Example 4: Assigning: 
> "r" = return value from "nbd_poll(h, timeout)".
>       libnbd-1.12.5/ocaml/nbd-c.c:2602: example_checked: Example 4 (cont.): 
> "r" has its value checked in "r == -1".
>       libnbd-1.12.5/python/methods.c:2806: example_assign: Example 5: 
> Assigning: "ret" = return value from "nbd_poll(h, timeout)".
>       libnbd-1.12.5/python/methods.c:2808: example_checked: Example 5 
> (cont.): "ret" has its value checked in "ret == -1".
>       #  178|       /* Dispatch work while there are commands in flight. */
>       #  179|       while (thread->in_flight > 0)
>       #  180|->       nbd_poll (h, -1);
>       #  181|     }
>       #  182|
>
> What it's saying is that in this code base, nbd_poll's return value
> was checked by the caller 5 out of 6 times, but ignored here.  (This
> turned out to be a real bug which we fixed).
>
> It seems like the check implemented in your patch is: If the return
> value is used 0 times anywhere in the code base, change the return
> value to 'void'.  Coverity would not flag this.
>
> Maybe a consistent use check is better?

Note that the analyzer is currently limited to analyzing a single
translation unit at a time, so we would only be able to implement a
consistent use check for static functions (this is why the current
"return-value-never-used" check only applies to static functions).

It may be worthwhile exploring cross-translation unit analysis,
although it may be difficult to accomplish while also avoiding
reanalyzing the entire tree every time a single translation unit is
modified.

Alberto