On Fri, 30 Jul 2021, Martin Sebor wrote:
> On 7/30/21 2:21 PM, Tom de Vries wrote:
> > On 7/30/21 6:17 PM, Martin Sebor wrote:
> >> On 7/28/21 9:20 AM, Tom de Vries wrote:
> >>> Hi,
> >>>
> >>> Improve nonnull attribute documentation in a number of ways:
> >>>
> >>> Reorganize discussion of effects into:
> >>> - effects for calls to functions with nonnull-marked parameters, and
> >>> - effects for function definitions with nonnull-marked parameters.
> >>> This makes it clear that -fno-delete-null-pointer-checks has no effect
> >>> for
> >>> optimizations based on nonnull-marked parameters in function definitions
> >>> (see PR100404).
> >>
> >> This resolves half of PR 101665 that I raised the other day (i.e.,
> >> updates the docs). Thank you!
> >
> > You're welcome :)
> >
> >> Since PR 100404 was resolved as
> >> invalid,
> >
> > Yeah, I can also live with reopening that one as documentation PR.
> >
> >> can you please reference the other PR in the changelog?
> >
> > Done.
> >
> >> The other half (warning when attribute nonnull is specified along
> >> with attribute optimize "-fno-delete-null-pointer-checks") remains.
> >> I plan to look into it unless someone beats me to it or unless some
> >> other solution emerges.
> >>
> >
> > FWIW, In my reply to Richi here (
> > https://gcc.gnu.org/pipermail/gcc-patches/2021-July/576415.html ) I
> > proposed to split the existing nonnull attribute functionality into
> > assume_nonnull/verify_nonnull attributes. I'm curious what you think of
> > that proposal.
>
> I think it may have been worth considering at the time nonnull was
> being proposed but it's too late now. Users are familiar with it
> under its current name, it's embedded in lots of code, and it's
> supported by GCC-compatible compilers. Introducing a pair of new
> attributes with slightly different semantics would make it difficult
> to write code that's meant to be portable across these implementations
> (including prior versions of GCC). Not to mention that it wouldn't
> help users of nonnull.
True.
> But since the problem is limited to function definitions, we don't
> need a solution for declarations. What's missing is an intuitive
> way to tell GCC that an attribute on the declaration should not
> have an effect in its definition. A nicer name for Andrew's asm
> trick. Coming up with a generic enough name would make it usable
> for arguments with other attributes with similar effect (the only
> one that comes to mind at the moment is attribute access which
> also triggers warnings in function bodies, although it's not yet
> used for optimization; others might pop up in the future).
I think -fno-delete-null-pointer-checks covering exactly this case
would be OK. I think we just want to make sure we're not losing
the warnings when functions are called with explicit NULL in that
case.
Richard.
> Martin
>
> >
> >> A few comments on the documentation changes below.
> >>
> >>>
> >>> Mention -Wnonnull-compare.
> >>>
> >>> Mention workaround from PR100404 comment 7.
> >>>
> >>> The workaround can be used for this scenario. Say we have a test.c:
> >>> ...
> >>> #include <stdlib.h>
> >>>
> >>> extern int isnull (char *ptr) __attribute__ ((nonnull));
> >>> int isnull (char *ptr)
> >>> {
> >>> if (ptr == 0)
> >>> return 1;
> >>> return 0;
> >>> }
> >>>
> >>> int
> >>> main (void)
> >>> {
> >>> char *ptr = NULL;
> >>> if (isnull (ptr)) __builtin_abort ();
> >>> return 0;
> >>> }
> >>> ...
> >>>
> >>> The test-case contains a mistake: ptr == NULL, and we want to detect the
> >>> mistake using an abort:
> >>> ...
> >>> $ gcc test.c
> >>> $ ./a.out
> >>> Aborted (core dumped)
> >>> ...
> >>>
> >>> At -O2 however, the mistake is not detected:
> >>> ...
> >>> $ gcc test.c -O2
> >>> $ ./a.out
> >>> ...
> >>> which is what -Wnonnull-compare (not show here) warns about.
> >>>
> >>> The easiest way to fix this is by dropping the nonnull attribute. But
> >>> that
> >>> also disables -Wnonnull, which would detect something like:
> >>> ...
> >>> if (isnull (NULL)) __builtin_abort ();
> >>> ...
> >>> at compile time.
> >>>
> >>> Using this workaround:
> >>> ...
> >>> int isnull (char *ptr)
> >>> {
> >>> + asm ("" : "+r"(ptr));
> >>> if (ptr == 0)
> >>> return 1;
> >>> return 0;
> >>> }
> >>> ...
> >>> we still manage to detect the problem at runtime with -O2:
> >>> ...
> >>> $ ~/gcc_versions/devel/install/bin/gcc test.c -O2
> >>> $ ./a.out
> >>> Aborted (core dumped)
> >>> ...
> >>> while keeping the possibility to detect "isnull (NULL)" at compile time.
> >>>
> >>> OK for trunk?
> >>>
> >>> Thanks,
> >>> - Tom
> >>>
> >>> [gcc/doc] Improve nonnull attribute documentation
> >>>
> >>> gcc/ChangeLog:
> >>>
> >>> 2021-07-28 Tom de Vries <tdevr...@suse.de>
> >>>
> >>> * doc/extend.texi (nonnull attribute): Improve documentation.
> >>>
> >>> ---
> >>> gcc/doc/extend.texi | 51
> >>> ++++++++++++++++++++++++++++++++++++++++-----------
> >>> 1 file changed, 40 insertions(+), 11 deletions(-)
> >>>
> >>> diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi
> >>> index b83cd4919bb..3389effd70c 100644
> >>> --- a/gcc/doc/extend.texi
> >>> +++ b/gcc/doc/extend.texi
> >>> @@ -3488,17 +3488,46 @@ my_memcpy (void *dest, const void *src, size_t
> >>> len)
> >>> @end smallexample
> >>> @noindent
> >>> -causes the compiler to check that, in calls to @code{my_memcpy},
> >>> -arguments @var{dest} and @var{src} are non-null. If the compiler
> >>> -determines that a null pointer is passed in an argument slot marked
> >>> -as non-null, and the @option{-Wnonnull} option is enabled, a warning
> >>> -is issued. @xref{Warning Options}. Unless disabled by
> >>> -the @option{-fno-delete-null-pointer-checks} option the compiler may
> >>> -also perform optimizations based on the knowledge that certain function
> >>> -arguments cannot be null. In addition,
> >>> -the @option{-fisolate-erroneous-paths-attribute} option can be specified
> >>> -to have GCC transform calls with null arguments to non-null functions
> >>> -into traps. @xref{Optimize Options}.
> >>> +informs the compiler that, in calls to @code{my_memcpy}, arguments
> >>> +@var{dest} and @var{src} must be non-null.
> >>> +
> >>> +The attribute has effect both for functions calls and function
> >>> definitions.
> >>
> >> Missing article: has <ins>an </ins> effect. Also, an effect on
> >> (rather than for) might be more appropriate.
> >>
> >
> > Done.
> >
> >>> +
> >>> +For function calls:
> >>> +@itemize @bullet
> >>> +@item If the compiler determines that a null pointer is
> >>> +passed in an argument slot marked as non-null, and the
> >>> +@option{-Wnonnull} option is enabled, a warning is issued.
> >>> +@xref{Warning Options}.
> >>> +@item The @option{-fisolate-erroneous-paths-attribute} option can be
> >>> +specified to have GCC transform calls with null arguments to non-null
> >>> +functions into traps. @xref{Optimize Options}.
> >>> +@item The compiler may also perform optimizations based on the
> >>> +knowledge that certain function arguments cannot be null. These
> >>> +optimizations can be disabled by the
> >>> +@option{-fno-delete-null-pointer-checks} option. @xref{Optimize
> >>> Options}.
> >>> +@end itemize
> >>> +
> >>> +For function definitions:
> >>> +@itemize @bullet
> >>> +@item If the compiler determines that a function parameter that is
> >>> +marked with non-null
> >>
> >> Either no "with" or no hyphen in nonnull (when it names the attribute).
> >>
> >
> > Done.
> >
> >>> is compared with null, and
> >>> +@option{-Wnonnull-compare} option is enabled, a warning is issued.
> >>> +@xref{Warning Options}.
> >>> +@item The compiler may also perform optimizations based on the
> >>> +knowledge that certain function parameters cannot be null.
> >>
> >> "certain function parameters" makes me think it might include others
> >> besides those marked nonnull. Unless that's the case, to avoid any
> >> doubt I'd suggest to rephrase that: say "based on the knowledge that
> >> @code{nonnul} parameters cannot be null."
> >>
> >
> > Done.
> >
> >>> This can
> >>> +be disabled by hiding the nonnullness using an inline assembly
> >>> statement:
> >>
> >> This is a clever trick but it feels more like a workaround than
> >> a solution to recommend. I think I would find it preferable to
> >> accept and gracefully handle the combination of attribute nonnull
> >> and optimize ("-fno-delete-null-pointer-checks"). But that has
> >> the downside of disabling the removal of all such checks, not
> >> just those of the argument, and so doesn't seem like an optimal
> >> solution either. So it seems like some other mechanism might be
> >> needed here.
> >
> > Yeah, I hope the assume_nonnull / verify_nonnull is an acceptable way
> > forward.
> >
> > Anyway, dropped the workaround from this version.
> >
> > Thanks,
> > - Tom
> >
> >
>
>
--
Richard Biener <rguent...@suse.de>
SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg)
