[Bug middle-end/110617] New: RFE: Add a diagnostic-only variant of nonnull attribute

[Bug middle-end/110617] New: RFE: Add a diagnostic-only variant of nonnull attribute

[xry111 at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110617

            Bug ID: 110617
           Summary: RFE: Add a diagnostic-only variant of nonnull
                    attribute
           Product: gcc
           Version: 14.0
            Status: UNCONFIRMED
          Severity: enhancement
          Priority: P3
         Component: middle-end
          Assignee: unassigned at gcc dot gnu.org
          Reporter: xry111 at gcc dot gnu.org
  Target Milestone: ---

Currently nonnull serves as both a diagnostic attribute and an optimization
attribute.  But sometimes we want only the effect for diagnostic, but not the
effect for code generation.

For example, Glibc developers implements many functions as "crash the program
immediately if an unexpected NULL pointer is passed" [1].  So it would be
useful to make the potential crash detectable via -Wnonnull and/or
-Wanalyzer-null-argument.  However they don't like the nonnull attribute on the
function prototype, because the nonnull attribute may cause the optimizer to
break their "immediately crash" design [2].

I'm not sure how to name this variant of nonnull precisely. (Maybe "hate_null
or something?)

[Bug middle-end/110617] RFE: Add a diagnostic-only variant of nonnull attribute

[xry111 at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110617

--- Comment #1 from Xi Ruoyao <xry111 at gcc dot gnu.org> ---
[1]:
https://sourceware.org/glibc/wiki/Style_and_Conventions#Bugs_in_the_user_program
[2]: https://sourceware.org/pipermail/libc-alpha/2023-July/149893.html

[Bug middle-end/110617] RFE: Add a diagnostic-only variant of nonnull attribute

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110617

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |diagnostic

--- Comment #2 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
>But sometimes we want only the effect for diagnostic,

I think that is a bad idea.

[Bug middle-end/110617] RFE: Add a diagnostic-only variant of nonnull attribute

[alx at kernel dot org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110617

Alejandro Colomar <alx at kernel dot org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |alx at kernel dot org

--- Comment #3 from Alejandro Colomar <alx at kernel dot org> ---
Link:
<https://inbox.sourceware.org/libc-alpha/08d7552c-d90a-ae84-4b7e-2f6f2136dd66@kernel.org/T/#m95ea7dfa7f42100757e2794142b9eeb71dfce1f8>

[Bug middle-end/110617] RFE: Add a diagnostic-only variant of nonnull attribute

[sjames at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110617

Sam James <sjames at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |bugzilla at tecnocode dot co.uk,
                   |                            |sjames at gcc dot gnu.org,
                   |                            |tim.ruehsen at gmx dot de

--- Comment #4 from Sam James <sjames at gcc dot gnu.org> ---
There's a history of people objecting to use of nonnull, although I'm still
sceptical (not that it's my decision) - partly because nonnull has decent
takeup in general so changing its semantics now is not great.

It came up on the glib side at
https://gitlab.gnome.org/GNOME/glib/-/issues/2647, and wget2 considered/has
removed the attribute at https://gitlab.com/gnuwget/wget2/-/issues/200 too.

[Bug middle-end/110617] RFE: Add a diagnostic-only variant of nonnull attribute

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110617

--- Comment #5 from Richard Biener <rguenth at gcc dot gnu.org> ---
I think a -f... option to disable the code generation effects would make more
sense than adding another attribute kind.

[Bug middle-end/110617] RFE: Add a diagnostic-only variant of nonnull attribute

[xry111 at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110617

--- Comment #6 from Xi Ruoyao <xry111 at gcc dot gnu.org> ---
(In reply to Richard Biener from comment #5)
> I think a -f... option to disable the code generation effects would make
> more sense than adding another attribute kind.

Then maybe we'd just add a -D_GLIBC_NONNULL={0,1} (?) into Glibc cdefs.h
instead.  Anyway I'm already too frustrated about this so I'll not continue
working on nonnull within Glibc headers.  If you don't like this just close it
as WONTFIX.

[Bug middle-end/110617] RFE: Add a diagnostic-only variant of nonnull attribute

[alx at kernel dot org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110617

--- Comment #7 from Alejandro Colomar <alx at kernel dot org> ---
Hi Xi, Richard!

On 2023-07-11 10:34, xry111 at gcc dot gnu.org wrote:
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110617
>
> --- Comment #6 from Xi Ruoyao <xry111 at gcc dot gnu.org> ---
> Anyway I'm already too frustrated about this so I'll not continue
> working on nonnull within Glibc headers.  If you don't like this just close it
> as WONTFIX.
>

I understand your frustration.  I'll continue your work, if you
don't mind.

On 2023-07-11 09:46, rguenth at gcc dot gnu.org wrote:
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110617
>
> --- Comment #5 from Richard Biener <rguenth at gcc dot gnu.org> ---
> I think a -f... option to disable the code generation effects would make more
> sense than adding another attribute kind.
>

The idea of a -f... option makes sense.  However, I'm skeptical
about being able to reach null correctness via an attribute.  I
think a qualifier, similar to restrict or const, would be better
qualified (pun not intended 😄 for this task.

Clang's _Nonnull (and _Nullable, and friends) are such qualifiers,
similar to restrict.  I think they are better designed for the
goal of having diagnostics if null correctness is breached.

However, they have issues as qualifiers, since the standard says
they should be dropped in lvalue to rvalue conversions (restrict
shares this same issue).  There's been a suggestion in an LLVM
forum to add an _Optional qualifier to the pointee, which would
workaround the issue that qualifiers are dropped.  I'll put both
alternatives next to each other for comparison:

#pragma clang assume_nonnull begin

        int i;
        int *p;
        int *_Nullable q;
        _Optional int *r;

        p = NULL;  // warn
        q = NULL;  // Ok
        r = NULL;  // Ok

        p = &i;  // Ok
        q = &i;  // Ok
        r = &i;  // Ok

        p = r;  // warn: '_Optional' qualifier is discarded in assignment
        q = p;  // Ok
        r = p;  // Ok

#pragma clang assume_nonnull end

Cheers,
Alex

-- 
<http://www.alejandro-colomar.es/>
GPG key fingerprint: A9348594CE31283A826FBDD8D57633D441E25BB5

[Bug middle-end/110617] RFE: Add a diagnostic-only variant of nonnull attribute

[alx at kernel dot org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110617

--- Comment #8 from Alejandro Colomar <alx at kernel dot org> ---
Test

[Bug middle-end/110617] RFE: Add a diagnostic-only variant of nonnull attribute

[fw at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110617

--- Comment #9 from Florian Weimer <fw at gcc dot gnu.org> ---
(In reply to Xi Ruoyao from comment #6)
> (In reply to Richard Biener from comment #5)
> > I think a -f... option to disable the code generation effects would make
> > more sense than adding another attribute kind.
> 
> Then maybe we'd just add a -D_GLIBC_NONNULL={0,1} (?) into Glibc cdefs.h
> instead.  Anyway I'm already too frustrated about this so I'll not continue
> working on nonnull within Glibc headers.  If you don't like this just close
> it as WONTFIX.

For those who are not following libc-alpha, glibc already disables __nonnull
during its build, so it should be totally fine to use __nonnull in installed
headers to improve diagnostics.

We have this in include/sys/cdefs.h (which augments <sys/cdefs.h>):

/* The compiler will optimize based on the knowledge the parameter is
   not NULL.  This will omit tests.  A robust implementation cannot allow
   this so when compiling glibc itself we ignore this attribute.  */
# undef __nonnull
# define __nonnull(params)

We'd like the diagnostics for building glibc itself, and a new -f option would
help with that.

[Bug middle-end/110617] RFE: Add a diagnostic-only variant of nonnull attribute

[xry111 at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110617

--- Comment #10 from Xi Ruoyao <xry111 at gcc dot gnu.org> ---
(In reply to Florian Weimer from comment #9)
> (In reply to Xi Ruoyao from comment #6)
> > (In reply to Richard Biener from comment #5)
> > > I think a -f... option to disable the code generation effects would make
> > > more sense than adding another attribute kind.
> > 
> > Then maybe we'd just add a -D_GLIBC_NONNULL={0,1} (?) into Glibc cdefs.h
> > instead.  Anyway I'm already too frustrated about this so I'll not continue
> > working on nonnull within Glibc headers.  If you don't like this just close
> > it as WONTFIX.
> 
> For those who are not following libc-alpha, glibc already disables __nonnull
> during its build, so it should be totally fine to use __nonnull in installed
> headers to improve diagnostics.

But Zack's reason against using __nonnull is __nonnull may cause unwanted
optimizations to *the user code*.

[Bug middle-end/110617] RFE: Add a diagnostic-only variant of nonnull attribute

[fw at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110617

--- Comment #11 from Florian Weimer <fw at gcc dot gnu.org> ---
(In reply to Xi Ruoyao from comment #10)
> But Zack's reason against using __nonnull is __nonnull may cause unwanted
> optimizations to *the user code*.

GCC already offers options to control function call behavior in the presence of
nonnull attributes:

     For function calls:
        • If the compiler determines that a null pointer is passed in an
          argument slot marked as non-null, and the ‘-Wnonnull’ option
          is enabled, a warning is issued.  *Note Warning Options::.
        • The ‘-fisolate-erroneous-paths-attribute’ option can be
          specified to have GCC transform calls with null arguments to
          non-null functions into traps.  *Note Optimize Options::.
        • The compiler may also perform optimizations based on the
          knowledge that certain function arguments cannot be null.
          These optimizations can be disabled by the
          ‘-fno-delete-null-pointer-checks’ option.  *Note Optimize
          Options::.

So I don't think we need another way to disable nonnull attributes in installed
glibc headers.

[Bug middle-end/110617] RFE: Add a diagnostic-only variant of nonnull attribute

[xry111 at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110617

--- Comment #12 from Xi Ruoyao <xry111 at gcc dot gnu.org> ---
(In reply to Florian Weimer from comment #11)
> (In reply to Xi Ruoyao from comment #10)
> > But Zack's reason against using __nonnull is __nonnull may cause unwanted
> > optimizations to *the user code*.
> 
> GCC already offers options to control function call behavior in the presence
> of nonnull attributes:
> 
>      For function calls:
>         • If the compiler determines that a null pointer is passed in an
>           argument slot marked as non-null, and the ‘-Wnonnull’ option
>           is enabled, a warning is issued.  *Note Warning Options::.
>         • The ‘-fisolate-erroneous-paths-attribute’ option can be
>           specified to have GCC transform calls with null arguments to
>           non-null functions into traps.  *Note Optimize Options::.
>         • The compiler may also perform optimizations based on the
>           knowledge that certain function arguments cannot be null.
>           These optimizations can be disabled by the
>           ‘-fno-delete-null-pointer-checks’ option.  *Note Optimize
>           Options::.
> 
> So I don't think we need another way to disable nonnull attributes in
> installed glibc headers.

Ah indeed, I only remembered "-fisolate-erroneous-paths-attribute" but not
"-fno-delete-null-pointer-checks".