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

[Linus Torvalds]

On Fri, Apr 28, 2023 at 2:03 PM Nick Desaulniers
<ndesaulniers@xxxxxxxxxx> wrote:
>
> >
> >    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."

No, no, you misunderstand.

"foo()" is the one that is unconditionally _Nonnull. It never returns NULL.

But *bar()* is not. There's no _Nonnull about 'bar()'. Never was, never will be.

We are *not* looking to statically mark everything that never returns
NULL as _Nonnull. Only some core helper functions.

If "bar()" is a complicated function that can under some circumstances
return NULL, then it's clearly not _Nonnull.

It does not matter one whit that maybe in some simplified config,
bar() might never return NULL. That simply does *not* make it
_Nonnull.

But my point is that for a *compiler*, this is not actually easy at all.

Because a compiler might inline 'bar()' entirely (it's a trivial
function that only calls 'foo()', after all, so it *should* be
inlined.

But now that 'bar()' has been inlined into some other call-site, that
*other* call site will have a test for "is the result NULL?"

And that other call site *should* have that test. Because it didn't
call "foo()", it called "bar()".

But with the inlining, the compiler will likely see just the call to
foo(), and I suspect your patch would make it then complain about how
the result is tested for NULL.

So it would need to have that special logic of "only warn if the test
is at the same level".

> 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 solve the problem, yes. But I suspect it would be very
inconvenient for users.

In particular, it would have made it totally pointless for the issue
at hand: finding *existing* users of  __filemap_get_folio() (that used
to return NULL for errors), and finding the cases where the NULL check
still exists now that it's no longer the right thign to do.

So I think it would largely defeat the use-case. It would only warn
for cases that have already been annotated.

So to be useful, I think it would have to be a "does automagically the
right thing" kind of feature.

                 Linus