Bug 44081 - Incorrect nonnull assumed in code generation
Incorrect nonnull assumed in code generation
Status: UNCONFIRMED
Product: gcc
Classification: Unclassified
Component: middle-end
4.5.0
: P3 normal
: ---
Assigned To: Not yet assigned to anyone
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-05-11 14:40 UTC by Hugo van der Sanden
Modified: 2011-01-20 18:23 UTC (History)
4 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed:


Attachments
C source code (341 bytes, text/plain)
2010-05-11 14:41 UTC, Hugo van der Sanden
Details
Generated assembly code, with annotation (590 bytes, text/plain)
2010-05-11 14:41 UTC, Hugo van der Sanden
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Hugo van der Sanden 2010-05-11 14:40:14 UTC
zen% /opt/gcc-4.5.0/bin/gcc -v
Using built-in specs.
COLLECT_GCC=/opt/gcc-4.5.0/bin/gcc
COLLECT_LTO_WRAPPER=/opt/gcc-4.5.0/libexec/gcc/i686-pc-linux-gnu/4.5.0/lto-wrapper
Target: i686-pc-linux-gnu
Configured with: /src/package/lang/other/gcc-4.5.0/configure
--prefix=/opt/gcc-4.5.0 --with-gmp=/opt/gmp-4.2.2 --with-mpfr=/opt/mpfr-2.4.1
--with-mpc=/opt/mpc-0.8.1
Thread model: posix
gcc version 4.5.0 (GCC) 
zen% 

test.S is generated from 'gcc -S -O2 -fno-inline -Wall gcctest2.c -o gcctest2.S' using gcc-4.5.0. The assembler shows that the function test() does not check if its third parameter o2 is NULL, but one path in the calling function peep() does not guarantee that it is nonnull.

I was unable to reproduce this with gcc-4.4.3; I was also unable to reproduce it without the '__attribute__((nonnull(2)))' declaration - it seems possible that in some circumstances the nonnull may be getting applied to the wrong parameter, but that may be a red herring.

This is cut down from a problem found by Alex Hunsaker compiling perl-5.12.1, where the erroneous generated code caused a segfault <http://www.xray.mpe.mpg.de/mailing-lists/perl5-porters/2010-05/msg00316.html>.
Comment 1 Hugo van der Sanden 2010-05-11 14:41:27 UTC
Created attachment 20630 [details]
C source code
Comment 2 Hugo van der Sanden 2010-05-11 14:41:59 UTC
Created attachment 20631 [details]
Generated assembly code, with annotation
Comment 3 Jakub Jelinek 2010-05-11 14:51:36 UTC
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."
Comment 4 Hugo van der Sanden 2010-05-11 15:38:32 UTC
(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.
Comment 5 Andrew Pinski 2010-05-11 22:59:08 UTC
Works for me with:
        .ident  "GCC: (GNU) 4.6.0 20100427 (experimental) [trunk revision 158795]"
on x86_64 with -m32 -O2 -fno-inline.
Comment 6 Peter Moulder 2010-05-12 05:29:02 UTC
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.
Comment 7 Jakub Jelinek 2010-05-12 06:25:22 UTC
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).
Comment 8 Peter Moulder 2010-05-12 09:25:05 UTC
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]).
Comment 9 Hugo van der Sanden 2010-05-12 10:54:48 UTC
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.
Comment 10 joseph@codesourcery.com 2010-05-12 11:52:27 UTC
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."

Comment 11 Valeriy 2010-12-26 22:57:48 UTC
/*
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;
}
Comment 12 Valeriy 2011-01-20 18:23:29 UTC
/*    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)

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