Re: [GIT PULL] ext4 changes for the 6.4 merge window

[Nick Desaulniers]

On Wed, Apr 26, 2023 at 3:31 PM Linus Torvalds
<torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
>
> On Wed, Apr 26, 2023 at 3:08 PM Nick Desaulniers
> <ndesaulniers@xxxxxxxxxx> wrote:
> >
> > Is this what you had in mind?
> > ```
> > void * _Nonnull foo (void)
> > ...
> > void bar (void) {
> >     if (foo() == NULL) // maybe should warn that foo() returns _Nonnull?
> >         bar();
> > ...
> > linus.c:8:15: warning: comparison of _Nonnull function call 'foo'
> > equal to a null pointer is always false
>
> Yes.
>
> HOWEVER.
>
> I suspect you will find that it gets complicated for more indirect
> uses, and that may be why people have punted on this.
>
> For example, let's say that you instead have
>
>    void *bar(void) { return foo(); }
>
> and 'bar()' gets inlined.
>
> The obvious reaction to that is "ok, clearly the result is still
> _Nonnull, and should warn if it is tested.
>
> But that obvious reaction is actually completely wrong, because it may
> be that the real code looks something like
>
>    void *bar(void) {
> #if CONFIG_XYZ
>     if (somecondition) return NULL;
> #endif
>     return foo(); }
>
> and the caller really *should* check for NULL - it's just that the
> compiler never saw that case.

I think having a return value be conditionally _Nonnull is "garbage
in; garbage out."  The straightforward case would be to have `bar` be
conditionally _Nonnull on the same preprocessor condition.

void *
#ifndef CONFIG_XYZ
_Nonnull
#endif
bar (void) {
#ifdef CONFIG_XYZ
  if (somecondition) return NULL;
#endif
  return foo(); }

Then code reviewers could go: "yikes; please make bar unconditionally
_Nonnull, or simply don't use _Nonnull here."

>
> So only testing the direct return value of a function should warn.
>
> And even that "direct return value" is not trivial. What happens if
> you have something like this:
>
>    void bar(void) { do_something(foo()); }
>
> and "do_something()" ends up being inlined - and checks for its
> argument for being NULL? Again, that "test against NULL" may well be
> absolutely required in that context - because *other* call-sites will
> pass in pointers that might be NULL.
>
> Now, I don't know how clang works internally, but I suspect based just
> on the size of your patch that your patch would get all of this
> horribly wrong.

Of course, it was a quick hack.

>
> So doing a naked
>
>     void *ptr = foo();
>     if (!ptr) ...
>
> should warn.
>
> But doing the exact same thing, except the test for NULL came in some
> other context that just got inlined, cannot warn.

Thinking more about this, I really think _Nonnull should behave like a
qualifier (const, restrict, volatile). So the above example would be:

void * _Nonnull ptr = foo();
if (!ptr) // warning: tautology

That would require changes to the variables that calls to functions
that return ERR_PTR's were stored in.  That simplifies the semantic
analysis, since the compiler can simply look at the declaration of
`ptr` and not try to see that `ptr`'s value at some point in the
control flow graph was something returned from calling a _Nonnull
returning function.

Because _Nonnull isn't modeled as a qualifier in clang today, this doesn't warn:
```
void foo(void* _Nonnull);
void *bar(void);

void baz (void) {
  void *x = bar();
  foo(x);
}
```
It would be nice to say that "baz calls foo which expects its
parameter to be _Nonnull, but you passed a pointer that could be
null".  I also wonder if casting works...

Rust got this right; pretty sure they have NonNull and NonZero types.


>
> I _suspect_ that the reason clang does what it does is that this is
> just very complicated to do well.
>
> It sounds easy to a human, but ...

The bones are there; we could finish the damned thing if it sounds
like something that would be useful/usable in the kernel?  I guess the
impetus is that ERR_PTR checks should be comparing against < 0 rather
than == NULL, since that's a tautology?

-- 
Thanks,
~Nick Desaulniers