Created attachment 20630 [details]
C source code

Created attachment 20631 [details]
Generated assembly code, with annotation

That's a user bug.  You shouldn't pass NULL to arguments declared nonnull.
To quote gcc documentation:
"The compiler may also choose to make optimizations based
on the knowledge that certain function arguments will not be null."

(In reply to comment #3)
> That's a user bug.  You shouldn't pass NULL to arguments declared nonnull.
> To quote gcc documentation:
> "The compiler may also choose to make optimizations based
> on the knowledge that certain function arguments will not be null."
> 

Parameter 2 has been declared nonnull. It isn't being passed as NULL.

The generated code elides a test for parameter 3 being NULL. This parameter has not been declared nonnull.

Works for me with:
        .ident  "GCC: (GNU) 4.6.0 20100427 (experimental) [trunk revision 158795]"
on x86_64 with -m32 -O2 -fno-inline.

Part of the problem is a documentation bug: the nonnull attribute's parameter is named "arg-index", suggesting that the first parameter is 0, while the example strongly suggests that the first parameter is 1.

If parameters are numbered from 1, then please change the name to "arg-number"; if they are numbered from 0, then please change the example to match.

On the subject of nonnull's documentation (and only weakly related to the current bug report): To judge from the fact that this attribute is being incorrectly used in glibc on functions like memcmp,memcpy,memmove,qsort etc. where NULL is a valid value (so long as n==0), and also from comments such as at http://chtekk.longitekk.com/index.php?/archives/43-GCCs-__attribute__-nonnull-...-not-helpful-at-all.html, I suggest that the "The compiler may also choose to make optimizations based on the knowledge that certain function arguments will not be null" sentence be given more emphasis: I suggest putting it in a paragraph by itself, and adding a warning that a consequence would be that any checks for NULL in the function will be “optimized away”, and also warning that the attribute is inappropriate for functions such as memcmp,memcpy,memmove,qsort where NULL is only unusual (but valid when n==0) rather than unconditionally invalid.

I can't reproduce the problem either, neither in 4.6 nor in 4.4.  4.5.0 (rather than current 4.5 snapshots) had a bug with nonnull attribute if a function is IPA optimized and some arguments are optimized out, but I don't think that's the case here.

The nonnull arguments are 1 based, sorry for not reading the testcase carefully
when replying in #c3.

memcpy etc. with NULL argument is invalid, the standard say that those functions
copy from object pointed by the argument (or to object pointed by the argument).
NULL doesn't point to any object (the standard say that it compares unequal to pointer to any object).

OK, on careful reading, I agree re memcpy etc. (see below).

Consequently, I amend my suggested change to the documentation to:

  - Add a sentence about implicit `this' argument (copied & pasted from the format_arg entry).

  - Although largely taken care of by the above, I'd still be inclined to rename the attribute parameter from ‘arg-index’ to ‘arg-number’; and similarly to rename ‘string-index’ to ‘string-arg-number’ elsewhere in the texinfo node (in format and format_arg items).

  - Reword ‘argument index list’ to ‘argument number list’ (in the nonnull item).

  - Despite retracting the “wrongly used in glibc” grounds, I think the concerns raised on the page I linked to are sufficient to merit giving more emphasis to the "The compiler may also choose to make optimizations based on
the knowledge that certain function arguments will not be null" sentence: that it be put it in a paragraph by itself, and adding a warning that a consequence would be that any checks for NULL in the function may be optimized away.


Coming back to memcpy etc.: As this is such an edge case, I'll give more detail for my (revised) reasoning (partly for the benefit of certain others that I intend to refer to this discussion):

  - C99 §7.21.2.1 [#2]: “The memcpy function copies n [i.e. 0] characters from the object pointed to by s2 into the object pointed to by s1.”

    Text is similar for memcmp, memmove, memset, and their w- (wmemcmp etc.) counterparts, in each case referring to “the object pointed to by s/s1/s2”; while text for qsort says “sorts an array of nmemb objects, the initial element of which is pointed to by base”.

  - C99 §6.3.2.3 [#3]: “Such a pointer [as NULL], called a null pointer, is guaranteed to compare unequal to a pointer to any object or function.”

Thus, if s/s1/s2 is a null pointer, then “the object pointed to by s/s1/s2” isn't defined, so the sentence doesn't apply (despite copying/comparing only 0 bytes/characters from or to this undefined thing); and in each case there's no other text giving the behaviour when s/s1/s2 is a null pointer; so I conclude that the behaviour of these functions is indeed undefined when s/s1/s2 is a null pointer, so the “undefined behaviour” provisions of §3.18 apply, and optimizing out the test for NULL is conformant behaviour as an instance of “ignoring the situation [of s/s1/s2 == NULL] completely with unpredictable results” (§3.18 [#2]).

The direction of discussion has centred so far on the documentation, but as far as I can tell the only point at which the documentation confused someone was the triage at #3. Should there not be a separate bug opened for problems with the documentation?

Can somone with access to a stock 4.5.0 confirm that the originally reported testcase does indeed show a bug?

In #7, Jakub says:
> 4.5.0 (rather than current 4.5 snapshots) had a bug with nonnull attribute
> if a function is IPA optimized and some arguments are optimized out, but I
> don't think that's the case here.

Do you have a reference for this bug? What makes you think this isn't the same?

Note that in the process of cutting down the original code to a testcase, one constant that I found had to be preserved was that the test() function be static, so that gcc had freedom to fiddle with the calling conventions; another was that the first parameter be preserved even though unused; another was that the second parameter needed its nonnull attribute. It sounds very similar to me.

Subject: Re:  Incorrect nonnull assumed in code generation

On Wed, 12 May 2010, pmoulder at mail dot csse dot monash dot edu dot au wrote:

> Coming back to memcpy etc.: As this is such an edge case, I'll give more detail
> for my (revised) reasoning (partly for the benefit of certain others that I
> intend to refer to this discussion):

It's a case about which the standard is completely explicit.  7.21.1#2.  
"Where an argument declared as size_t n specifies the length of the array 
for a function, n can have the value zero on a call to that function. 
Unless explicitly stated otherwise in the description of a particular 
function in this subclause, pointer arguments on such a call shall still 
have valid values, as described in 7.1.4."

/*
nonnull_2.c

gcc -O0 nonnull_2.c
gcc -O1 nonnull_2.c
gcc -O3 nonnull_2.c

00000000  if (0 or 8)  <- Ok
00000004  else
00000008  if (0 or 8)

gcc -O2 nonnull_2.c
gcc -Os nonnull_2.c

00000000  else         <- Error
00000004  else
00000008  if (0 or 8)
*/

#include <stdio.h>

static void attribute_nonnull_2(int i1, int *i2, int *i3) __attribute__((__nonnull__(2)));
static void attribute_nonnull_2(int i1, int *i2, int *i3) {
	*i2 = i1;

	if (i3 == 0 || i3 == (int *) 8)
		printf("%p  if (0 or 8)\n", i3);
	else
		printf("%p  else       \n", i3);
}

int i2a;

int main(int argc, char *argv[]) {
	attribute_nonnull_2(1, &i2a, (int *) 0);
	attribute_nonnull_2(2, &i2a, (int *) 4);
	attribute_nonnull_2(3, &i2a, (int *) 8);
	return 0;
}

/*    attr_nn.c    */

#include <stdio.h>

static void attr_nn(int i1, int *i2, int *i3) __attribute__((__nonnull__(2)));
static void attr_nn(int i1, int *i2, int *i3) {
    *i2 = i1;

    if (i3 == 0 || i3 == (int *) 8)  printf("%p  if (0 or 8)\n", i3);
    else                             printf("%p  else       \n", i3);
}

int i2a;

int main(int argc, char *argv[]) {
    attr_nn(1, &i2a, (int *) 0);
    attr_nn(2, &i2a, (int *) 4);
    attr_nn(3, &i2a, (int *) 8);
    return 0;
}




/*
-----------------------------------------------------------------------------

Ubuntu GCC-4.4.5  -  Error


$ gcc -O0 attr_nn.c && ./a.out
$ gcc -O1 attr_nn.c && ./a.out
$ gcc -O3 attr_nn.c && ./a.out

(nil)  if (0 or 8)       <- Ok
0x4  else
0x8  if (0 or 8)

$ gcc -O2 attr_nn.c && ./a.out
$ gcc -Os attr_nn.c && ./a.out

(nil)  else           <- Error
0x4  else
0x8  if (0 or 8)


$ gcc -v
Using built-in specs.
Target: i686-linux-gnu
Configured with: ../src/configure -v --with-pkgversion='Ubuntu/Linaro 4.4.4-14ubuntu5' --with-bugurl=file:///usr/share/doc/gcc-4.4/README.Bugs --enable-languages=c,c++,fortran,objc,obj-c++ --prefix=/usr --program-suffix=-4.4 --enable-shared --enable-multiarch --enable-linker-build-id --with-system-zlib --libexecdir=/usr/lib --without-included-gettext --enable-threads=posix --with-gxx-include-dir=/usr/include/c++/4.4 --libdir=/usr/lib --enable-nls --with-sysroot=/ --enable-clocale=gnu --enable-libstdcxx-debug --enable-objc-gc --enable-targets=all --disable-werror --with-arch-32=i686 --with-tune=generic --enable-checking=release --build=i686-linux-gnu --host=i686-linux-gnu --target=i686-linux-gnu
Thread model: posix
gcc version 4.4.5 (Ubuntu/Linaro 4.4.4-14ubuntu5) 

-----------------------------------------------------------------------------

Cygwin GCC-4.3.4  - Ok


$ gcc -O0 attr_nn.c && ./a.exe
$ gcc -O1 attr_nn.c && ./a.exe
$ gcc -O2 attr_nn.c && ./a.exe
$ gcc -O3 attr_nn.c && ./a.exe
$ gcc -Os attr_nn.c && ./a.exe

0x0  if (0 or 8)         <- Ok
0x4  else
0x8  if (0 or 8)

-----------------------------------------------------------------------------

Cygwin GCC-4.5.0  - Error


$ gcc -O0 attr_nn.c && ./a.exe
$ gcc -O1 attr_nn.c && ./a.exe
$ gcc -O3 attr_nn.c && ./a.exe

0x0  if (0 or 8)         <- Ok
0x4  else
0x8  if (0 or 8)

$ gcc -O2 attr_nn.c && ./a.exe
$ gcc -Os attr_nn.c && ./a.exe

0x0  else             <- Error
0x4  else
0x8  if (0 or 8)


$ gcc -v
Using built-in specs.
COLLECT_GCC=gcc
COLLECT_LTO_WRAPPER=/usr/lib/gcc/i686-pc-cygwin/4.5.0/lto-wrapper.exe
Target: i686-pc-cygwin
Configured with: /gnu/gcc/releases/respins/4.5.0-1/gcc4-4.5.0-1/src/gcc-4.5.0/configure --srcdir=/gnu/gcc/releases/respins/4.5.0-1/gcc4-4.5.0-1/src/gcc-4.5.0 --prefix=/usr --exec-prefix=/usr --bindir=/usr/bin --sbindir=/usr/sbin --libexecdir=/usr/lib --datadir=/usr/share --localstatedir=/var --sysconfdir=/etc --datarootdir=/usr/share --docdir=/usr/share/doc/gcc4 --datadir=/usr/share --infodir=/usr/share/info --mandir=/usr/share/man -v --with-gmp=/usr --with-mpfr=/usr --enable-bootstrap --enable-version-specific-runtime-libs --libexecdir=/usr/lib --enable-static --enable-shared --enable-shared-libgcc --disable-__cxa_atexit --with-gnu-ld --with-gnu-as --with-dwarf2 --disable-sjlj-exceptions --enable-languages=ada,c,c++,fortran,java,lto,objc,obj-c++ --enable-graphite --enable-lto --enable-java-awt=gtk --disable-symvers --enable-libjava --program-suffix=-4 --enable-libgomp --enable-libssp --enable-libada --enable-threads=posix --with-arch=i686 --with-tune=generic --enable-libgcj-sublibs CC=gcc-4 CXX=g++-4 CC_FOR_TARGET=gcc-4 CXX_FOR_TARGET=g++-4 GNATMAKE_FOR_TARGET=gnatmake GNATBIND_FOR_TARGET=gnatbind --with-ecj-jar=/usr/share/java/ecj.jar
Thread model: posix
gcc version 4.5.0 (GCC)

-----------------------------------------------------------------------------

MinGW GCC-4.4.3 (Strawberry Perl)  -  Error


>gcc -O0 attr_nn.c && a.exe
>gcc -O1 attr_nn.c && a.exe
>gcc -O3 attr_nn.c && a.exe

00000000  if (0 or 8)    <- Ok
00000004  else
00000008  if (0 or 8)

>gcc -O2 attr_nn.c && a.exe
>gcc -Os attr_nn.c && a.exe

00000000  else           <- Error
00000004  else
00000008  if (0 or 8)


>gcc -v
Using built-in specs.
Target: i686-w64-mingw32
Configured with: ../gcc44-svn/configure --target=i686-w64-mingw32 --host=i686-w64-mingw32 --disable-multilib --disable-nls --disable-win32-registry --prefix=/mingw32 --with-gmp=/mingw32 -with-mpfr=/mingw32 --enable-languages=c,c++
Thread model: win32
gcc version 4.4.3 (GCC)

-----------------------------------------------------------------------------

MinGW GCC-4.5.0 (20101030)  -  Error


>gcc -O0 attr_nn.c && a.exe
>gcc -O1 attr_nn.c && a.exe
>gcc -O3 attr_nn.c && a.exe

00000000  if (0 or 8)    <- Ok
00000004  else
00000008  if (0 or 8)

>gcc -O2 attr_nn.c && a.exe
>gcc -Os attr_nn.c && a.exe

00000000  else           <- Error
00000004  else
00000008  if (0 or 8)


gcc -v
Using built-in specs.
COLLECT_GCC=gcc
COLLECT_LTO_WRAPPER=g:/mingw/bin/../libexec/gcc/mingw32/4.5.0/lto-wrapper.exe
Target: mingw32
Configured with: ../gcc-4.5.0/configure --enable-languages=c,c++,ada,fortran,objc,obj-c++ --disable-sjlj-exceptions --with-dwarf2 --enable-shared --enable-libgomp --disable-win32-registry --enable-libstdcxx-debug --enable-version-specific-runtime-libs --disable-werror --build=mingw32 --prefix=/mingw

Thread model: win32
gcc version 4.5.0 (GCC)

-----------------------------------------------------------------------------
*/

Should we close this?

(In reply to Manuel López-Ibáñez from comment #13)
> Should we close this?

With what status?

I think it should at least be updated to CONFIRMED, based on the comments from valeriy.

I should note also that I was discouraged by the quality of response, and in particular that I never got a reply to #9.

I'm unable to reproduce any unexpected/erroneous behavior with the test cases submitted in comment #11 and later.  Recent versions of GCC (5.x and beyond) all print the same output (marked <- Ok in the comments).  Thus resolving as fixed.