Обсуждение: Add support for __attribute__((returns_nonnull))

Here is a patch which adds support for the returns_nonnull attribute
alongside all the other attributes we optionally support.

I recently wound up in a situation where I was checking for NULL return
values of a function that couldn't ever return NULL because the
inability to allocate memory was always elog(ERROR)ed (aborted).

I didn't go through and mark anything, but I feel like it could be
useful for people going forward, including myself.

--
Tristan Partin
Neon (https://neon.tech)

On 19.12.23 21:43, Tristan Partin wrote:
> Here is a patch which adds support for the returns_nonnull attribute 
> alongside all the other attributes we optionally support.
> 
> I recently wound up in a situation where I was checking for NULL return 
> values of a function that couldn't ever return NULL because the 
> inability to allocate memory was always elog(ERROR)ed (aborted).
> 
> I didn't go through and mark anything, but I feel like it could be 
> useful for people going forward, including myself.

I think it would be useful if this patch series contained a patch that 
added some initial uses of this.  That way we can check that the 
proposed definition actually works, and we can observe what it does, 
with respect to warnings, static analysis, etc.

On Wed Dec 27, 2023 at 6:42 AM CST, Peter Eisentraut wrote:
> On 19.12.23 21:43, Tristan Partin wrote:
> > Here is a patch which adds support for the returns_nonnull attribute
> > alongside all the other attributes we optionally support.
> >
> > I recently wound up in a situation where I was checking for NULL return
> > values of a function that couldn't ever return NULL because the
> > inability to allocate memory was always elog(ERROR)ed (aborted).
> >
> > I didn't go through and mark anything, but I feel like it could be
> > useful for people going forward, including myself.
>
> I think it would be useful if this patch series contained a patch that
> added some initial uses of this.  That way we can check that the
> proposed definition actually works, and we can observe what it does,
> with respect to warnings, static analysis, etc.

Good point. Patch attached.

I tried to find some ways to prove some value, but I couldn't. Take this
example for instance:

    static const char word[] = { 'h', 'e', 'l', 'l', 'o' };

    const char * __attribute__((returns_nonnull))
    hello()
    {
        return word;
    }

    int
    main(void)
    {
        const char *r;

        r = hello();
        if (r == NULL)
            return 1;

        return 0;
    }

I would have thought I could get gcc or clang to warn on a wasteful NULL
check, but alas. I also didn't see any code generation improvements, but
I am assuming that the example is too contrived. I couldn't find any
good things online that had examples of when such an annotation forced
the compiler to warn or create more optimized code.

If you return NULL from the hello() function, clang will warn that the
attribute doesn't match reality.

--
Tristan Partin
Neon (https://neon.tech)

On Thu, Dec 28, 2023 at 1:20 AM Tristan Partin <tristan@neon.tech> wrote:
> I recently wound up in a situation where I was checking for NULL return
> values of a function that couldn't ever return NULL because the
> inability to allocate memory was always elog(ERROR)ed (aborted).

It sounds like you have a ready example to test, so what happens with the patch?

As for whether any code generation changed, I'd start by checking if
anything in a non-debug binary changed at all.

On Sun Dec 31, 2023 at 9:29 PM CST, John Naylor wrote:
> On Thu, Dec 28, 2023 at 1:20 AM Tristan Partin <tristan@neon.tech> wrote:
> > I recently wound up in a situation where I was checking for NULL return
> > values of a function that couldn't ever return NULL because the
> > inability to allocate memory was always elog(ERROR)ed (aborted).
>
> It sounds like you have a ready example to test, so what happens with the patch?
>
> As for whether any code generation changed, I'd start by checking if
> anything in a non-debug binary changed at all.

The idea I had in mind initially was PGLC_localeconv(), but I couldn't
prove that anything changed with the annotation added. The second patch
in my previous email was attempt at deriving real-world benefit, but
nothing I did seemed to change anything. Perhaps you can test it and see
if anything changes for you.

--
Tristan Partin
Neon (https://neon.tech)

On Mon, Jan 08, 2024 at 05:04:58PM -0600, Tristan Partin wrote:
> The idea I had in mind initially was PGLC_localeconv(), but I couldn't
> prove that anything changed with the annotation added. The second patch
> in my previous email was attempt at deriving real-world benefit, but
> nothing I did seemed to change anything. Perhaps you can test it and see
> if anything changes for you.

There are a bunch of places in the tree where we return NULL just to
keep the compiler quiet, and where we should never return NULL, as in:
    return NULL; /* keep compiler quiet */

See bbstreamer_gzip.c as one example for non-gzip builds.  Perhaps you
could look at one of these places and see if the marker shows
benefits?
--
Michael