[Bug web/36166] New: Documentation for the 'nonnull' attribute is a bit misleading

[Bug web/36166] New: Documentation for the 'nonnull' attribute is a bit misleading

[pgut001 at cs dot auckland dot ac dot nz]

(I assume this goes into the category 'web', I couldn't find one for
'documentation')...

The documentation for the 'nonnull' attribute (section 5.27) currently says:

"The compiler may also choose to make optimizations based on the knowledge that
certain function arguments will not be null".

This is somewhat misleading in that it doesn't quite convey the effects that
may result from incorrect use of nonnull, and in particular the fact that while
analysis and issuing of warnings on nonnull is performed by the front-end, use
of nonnull for code-generation is done by the back-end.  As a result there can
be cases where no warning is issued because the analysis required to reveal
this would have to be done by the back-end, but the back-end optimiser still
changes the code that it generates under the assumption that the pointer is
never null.

Obviously this can be excused by saying that it's the programmer's fault for
applying the attribute incorrectly, but if its primary use is as a code-
diagnosis tool then the programmers may not be aware of the sometimes drastic
code-generation side-effects.

To make developers aware of this issue it'd be useful to amend the docs to
append to the above sentence the additional text:

"Note that the use of 'nonnull' to generate warnings and to generate code are
performed by different passes of the compiler.  The optimiser may completely
remove sections of code (for example checks for a pointer being null) if it
encounters a parameter with the 'nonnull' attribute set.  This attribute should
therefore be used with care".


-- 
           Summary: Documentation for the 'nonnull' attribute is a bit
                    misleading
           Product: gcc
           Version: 4.3.0
            Status: UNCONFIRMED
          Severity: minor
          Priority: P3
         Component: web
        AssignedTo: unassigned at gcc dot gnu dot org
        ReportedBy: pgut001 at cs dot auckland dot ac dot nz


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=36166

[Bug c/36166] Use of the 'nonnull' attribute breaks code

[pgut001 at cs dot auckland dot ac dot nz]

------- Comment #1 from pgut001 at cs dot auckland dot ac dot nz  2008-06-10 07:48 -------
This is actually a lot more serious than just a documentation issue (I've
changed it from a doc issue to a gcc issue) in that the use of this annotation
by the optimizer will break correctly-functioning code by silently removing any
checks for NULL pointers.  To summarise the original issue:

There are cases where no warning about use of a NULL pointer is issued because
the analysis required to reveal this would have to be done by the back-end, but
the back-end optimiser still changes the code that it generates under the
assumption that the pointer is never null.

In other words use of this annotation to warn about NULL pointers results in
code that segfaults if the pointer ever does happen to be NULL, even though the
compiler doesn't (reliably) warn about use of a NULL pointer in the first
place.  Because of this it's just too dangerous to use, if the compiler can't
reliably warn about use of NULL pointers then it shouldn't then use this
unreliable information to change the behaviour of generated code.

The problem is compounded by the way the attribute is used, for __unused__ the
attribute is placed in front of the variable that it affects but for
__nonnull__ it's necessary to tediously hand-count parameters and then provide
a list of index values for the attribute.  When annotating a function with
several pointers, some of which can be null and some of which can't, it's very
easy to lose track and supply an off-by-one index, which gcc uses to remove
NULL pointer checks resulting in segfaults in apparently-correct code.  Is
there any reason why __nonnull__ can't work the same way as __unused__?


-- 

pgut001 at cs dot auckland dot ac dot nz changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Severity|minor                       |normal
          Component|web                         |c
            Summary|Documentation for the       |Use of the 'nonnull'
                   |'nonnull' attribute is a bit|attribute breaks code
                   |misleading                  |


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=36166

[Bug middle-end/36166] Use of the 'nonnull' attribute breaks code

[pinskia at gcc dot gnu dot org]

------- Comment #2 from pinskia at gcc dot gnu dot org  2008-06-10 08:25 -------
No this is not a non documentation bug as non-null attribute says that the
argument has to be non null to work correctly. So if you pass a NULL argument
to the function, it will crash.  So optimizing out the check for NULLness is
correct really as the function is already expecting nonNULL.


-- 

pinskia at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
          Component|c                           |middle-end
           Keywords|                            |documentation


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=36166

[Bug middle-end/36166] Use of the 'nonnull' attribute breaks code

[pgut001 at cs dot auckland dot ac dot nz]

------- Comment #3 from pgut001 at cs dot auckland dot ac dot nz  2008-06-10 09:02 -------
It's more than just a documentation bug, two different portions of gcc are
interpreting this attribute in completely different ways, and the interaction
between the two breaks otherwise perfectly valid code.  The intended use of the
nonnull attribute is to warn, at compile time, against the inadvertent use of
NULL pointers where they should be nonnull.  By overloading it to also change
the way code generation works gcc is making it far too dangerous to use and
more or less destroying its usefulness for its original application - a single
error in counting parameters or applying the annotation and your otherwise
fully correct code suddenly breaks.  In fact gcc seems to be doing the opposite
of what ISO WG 14 is proposing for this attribute, which was to add extra
checking to make sure the attribute isn't NULL.  gcc instead *removes* extra
checking to make sure the attribute isn't NULL.

As it stands, gcc's interpretation of nonnull is unsafe at any speed, it
doesn't reliably warn about NULL pointers, but it does reliably break your code
when used.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=36166

[Bug middle-end/36166] Use of the 'nonnull' attribute breaks code

[pinskia at gcc dot gnu dot org]

------- Comment #4 from pinskia at gcc dot gnu dot org  2008-06-10 09:07 -------
The GCC attribute is an extension.  What it is about is saying if it is NULL
then the program/function will crash and is not expecting a NULL argument.  So
if you mark a function argument as non NULL, then the optimizer can optimize it
based on that fact.  Even though the C standards committee is proposing
something different, that will be handled differently from the GCC attribute
extension anyways.  So again there is no changed needed really to the GCC
extension as it is defined as one that will crash if supplied with a NULL
argument.  Think printf with %s where it is supplied with a NULL argument or
puts with a NULL argument.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=36166

[Bug middle-end/36166] Use of the 'nonnull' attribute breaks code

[pgut001 at cs dot auckland dot ac dot nz]

------- Comment #5 from pgut001 at cs dot auckland dot ac dot nz  2008-06-10 09:15 -------
>Think printf with %s where it is supplied with a NULL argument or puts 
>with a NULL argument.

Sure, I understand how that part of it works, but gcc doesn't just use it for
that, it applies two often mutually exclusive interpretations to the same
attribute.  Warning about inadvertent use of NULL is useful for developers, so
there's a temptation to annotate code with it to warn at compile time of errors
(and that seems to be the intended use of stdc_nonnull).  However the opposite
interpretation of the attribute is that if you do inadvertently pass a NULL
pointer to annotated code, gcc may not warn about it but will cause your code
to crash.  The single attribute shouldn't be used to perform two mutually
exclusive (and in one case, dangerous) actions, it's like storing rat poison in
a fruit juice bottle...


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=36166

[Bug middle-end/36166] Use of the 'nonnull' attribute breaks code

[pinskia at gcc dot gnu dot org]

------- Comment #6 from pinskia at gcc dot gnu dot org  2008-06-10 09:19 -------
>gcc doesn't just use it for that, it applies two often mutually exclusive interpretations to the same
> attribute.

They are no mutually exclusive at all.  Think of it is this way.  The developer
of the API says that it must be non-NULL so when the developer of the API then
tests for NULLness he is either being stupid or really just thinking that the
user will not use it correctly but since the warning is there, there is no
reason for the function itself to test for NULLness which is why GCC optimizes
away the check.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=36166

[Bug middle-end/36166] Use of the 'nonnull' attribute breaks code

[pgut001 at cs dot auckland dot ac dot nz]

------- Comment #7 from pgut001 at cs dot auckland dot ac dot nz  2008-06-10 09:27 -------
>The developer of the API says that it must be non-NULL so when the developer 
>of the API then tests for NULLness he is either being stupid or really just
>thinking that the user will not use it correctly but since the warning is 
>there

But the warning *isnt't* there, gcc doesn't reliably warn about incorrect use
of a NULL pointer.  If gcc could reliably detect use of a NULL pointer and warn
about it, and *then* remove the NULL pointer checks that'd be fine.  At the
moment gcc doesn't warn about NULL pointer use but does remove code that would
catch inadvertent NULL pointer use, and that's my complaint with the way the
attribute works - it's what James Reason (who specialises in safety
engineering) would call a latent pathogen.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=36166

[Bug middle-end/36166] Use of the 'nonnull' attribute breaks code

[rguenth at gcc dot gnu dot org]

------- Comment #8 from rguenth at gcc dot gnu dot org  2008-06-10 09:56 -------
This works as designed (and documented).  Whether the design is the best
possible
one is another question.


-- 

rguenth at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |RESOLVED
         Resolution|                            |INVALID


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=36166

[Bug middle-end/36166] Use of the 'nonnull' attribute breaks code

[joseph at codesourcery dot com]

------- Comment #9 from joseph at codesourcery dot com  2008-06-10 11:53 -------
Subject: Re:  Use of the 'nonnull' attribute breaks
 code

On Tue, 10 Jun 2008, pgut001 at cs dot auckland dot ac dot nz wrote:

> fully correct code suddenly breaks.  In fact gcc seems to be doing the opposite
> of what ISO WG 14 is proposing for this attribute, which was to add extra
> checking to make sure the attribute isn't NULL.  gcc instead *removes* extra
> checking to make sure the attribute isn't NULL.

If WG14 is proposing incompatible attribute semantics, that's ignoring the 
C1X Charter (N1250) - we agreed at the London meeting that one of the main 
problems with C99 and reasons for it not being widely implemented, that 
should be avoided for C1x, was invention and being incompatible with 
existing practice.  (Some subsequent minutes suggest that the idea has 
since been adopted that C++0x can invent something and then C1x can take 
C++0x as existing practice for C; not a good idea in my view.)

    13. Unlike for C9X, the consensus at the London meeting was that there 
    should be no invention, without exception. Only those features that 
    have a history and are in common use by a commercial implementation 
    should be considered. Also there must be care to standardize these 
    features in a way that would make the Standard and the commercial 
    implementation compatible.

Do you have a reference to the incompatible proposal?  I sent N1259 a 
while back warning about incompatibilities in syntax; perhaps a paper is 
needed on incompatible semantics as well.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=36166

[Bug middle-end/36166] Use of the 'nonnull' attribute breaks code

[pgut001 at cs dot auckland dot ac dot nz]

------- Comment #10 from pgut001 at cs dot auckland dot ac dot nz  2008-06-11 03:47 -------
>13. Unlike for C9X, the consensus at the London meeting was that there should
>be no invention, without exception. Only those features that have a history
>and are in common use by a commercial implementation should be considered.
>Also there must be care to standardize these features in a way that would
>make the Standard and the commercial implementation compatible.

Well they've done exactly that, it's perfectly compatible with VC++ (via
PREfast), which is a widely-used commercial implementation :-).

>Do you have a reference to the incompatible proposal?  I sent N1259 a while
>back warning about incompatibilities in syntax; perhaps a paper is needed on
>incompatible semantics as well.

I'm using N1273, which seems to be the only published statement on the use of
attributes.  For stdc_nonnull it describes the proposed semantics as:

  To be discussed[[the implementation may/it is implementation defined add
  assertions to ensure the runtime values of the variables are not null
  pointers]

OTOH the gcc interpretation of this seems to be:

  To be discussed[[the implementation may/it is implementation defined
  *remove* assertions that ensure the runtime values of the variables are not
  null pointers]

If the current gcc behaviour is regarded as correct and appropriate then maybe
the docs need to be updated (which is why I originally reported this as a doc.
bug, I wasn't sure which of the two was definitive).  Currently the docs say
something to the effect of:

  The compiler WILL issue warnings and MAY change its code generation when it
  sees this attribute.

when in fact what it really does is:

  The compiler WILL change its code generation and MAY issue warnings when it
  sees this attribute.

So the proposed change would be to substitute, in place of the current

 ... causes the compiler to check that, in calls to my_memcpy, arguments dest
 and src are non-null. 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. The compiler may also choose to make
 optimizations based on the knowledge that certain function arguments will not
 be null.

the new text:

 ... causes the compiler to make optimizations based on the knowledge that
 certain function arguments will not be null.  The compiler may also choose to
 check that, in calls to my_memcpy, arguments dest and src are non-null. 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.

This more accurately reflects the actual behaviour of the compiler.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=36166