Bug 17308

I think the issue here is that we issue the warning way before getting to the middle-end, in the front-
end.

*** Bug 22179 has been marked as a duplicate of this bug. ***

*** Bug 30043 has been marked as a duplicate of this bug. ***

/*
  I concur with Ulrich, but three years on, using gcc 4.1.2.

  Although a parameter which is marked with the 'nonnull' attribute
  is demonstrably nonnull, and although the compiler recognizes it is
  specifically NULL, no warning is emitted.

  And, in fact, the compiler can end up generating bad code.

  Using built-in specs.
  Target: x86_64-redhat-linux
  Configured with: ../configure --prefix=/usr --mandir=/usr/share/man  \
    --infodir=/usr/share/info --enable-shared --enable-threads=posix   \
    --enable-checking=release --with-system-zlib --enable-__cxa_atexit \
    --disable-libunwind-exceptions --enable-libgcj-multifile           \
    --enable-languages=c,c++,objc,obj-c++,java,fortran,ada             \
    --enable-java-awt=gtk --disable-dssi --enable-plugin               \
    --with-java-home=/usr/lib/jvm/java-1.4.2-gcj-1.4.2.0/jre           \
    --with-cpu=generic --host=x86_64-redhat-linux
  Thread model: posix
  gcc version 4.1.2 20071124 (Red Hat 4.1.2-42)



        gcc -c  -o nonnull.o -O3 -Wall nonnull.c
        nonnull.c: In function ‘f1’:
        nonnull.c:52: warning: null argument where non-null required (argument 1)
        nonnull.c:52: warning: null argument where non-null required (argument 2)

        objdump --disassemble --reloc nonnull.o

        nonnull.o:     file format elf64-x86-64

        Disassembly of section .text:

        0000000000000000 <f1>:
           0:	f3 c3                	repz retq
           2:	0f 1f 80 00 00 00 00 	nopl   0x0(%rax)
           9:	0f 1f 80 00 00 00 00 	nopl   0x0(%rax)

  If '-Wall' is not provided, the compiler does not even warn about
  the NULL arguments.  It hardly matters if the compiler warns or not,
  the code that is generated for 'f1()' is simply bad code.  But, when
  no warning is provided by the compiler, the code is egregiously bad
  -- anyone wondering why their program doesn't work may disassemble
  the object module and see that nothing was emitted.

        0000000000000010 <f0>:
          10:	ba 00 04 00 00       	mov    $0x400,%edx     ; length = 1024
          15:	31 f6                	xor    %esi,%esi       ; second arg = NULL
          17:	bf 00 00 00 00       	mov    $0x0,%edi       ; first arg = buf
          1c:	48 c7 05 00 00 00 00 	movq   $0x0,0(%rip)    ; a = buf
          23:	00 00 00 00
          27:	48 c7 05 00 00 00 00 	movq   $0x0,0(%rip)    ; b = NULL
          2e:	00 00 00 00
          32:	e9 00 00 00 00       	jmpq   memcpy

  In this function, the compiler has automatically inlined
  'copy_block()', and obviously determined that the second parameter
  is NULL, as can be seen from addresses 15 & 27 -- but still no
  warning is presented.
 */

#include <stdlib.h>
#include <string.h>

extern void *a;
extern void *b;

static void __attribute__((nonnull(1, 2)))
copy_block(void *dest, void *src)
{
   memcpy(dest, src, 1024);
}


void f0(void)
{
   static char buf[100];
   a = buf;
   b = NULL;
   copy_block(a, b);
}

void f1(void)
{
   copy_block(NULL, NULL);
}

I second Ulrich's request.

Besides nonnull, this enhancement would be useful in attribute printf
as well. For example, in the program below, both calls to printf() have
undefined behavior in C99 and should be diagnosed:

$ cat t.c && gcc -Wformat -pedantic -std=c99 -O3 t.c
int printf(const char*, ...)
  __attribute__((__nonnull__((1))))
  __attribute__ ((__format__ (__printf__, 1, 2)));

int main() {
  char *s = 0;
  printf(s, "");
  printf("%s", s);
}
$

It's been several years, and this attribute is still missing functionality about warning for deduced NULL values.  Can this be looked at?

(In reply to comment #6)
> It's been several years, and this attribute is still missing functionality
> about warning for deduced NULL values.  Can this be looked at?

GCC has as a unwritten policy to not emit warnings from the middle-end, except for a few exceptions. The front-ends do not have the infrastructure to do this kind of analysis, and GCC maintainers have repeatedly said that there is no intention to make gcc front-ends serve as a source code analysis tool like the clang analyzer. So, unless new contributors step forward with a convincing implementation of this, and for what I mean by "convincing" please read point 3 in the notes to beginners[*], the answer is: No, I wouldn't expect this to be implemented in the near or medium future. Sorry for the bad news, but I think it is better to be honest and save you the frustration. 

[*] http://gcc.gnu.org/wiki/GCC_Research

I hit this again today, and I'm still upset that gcc is doing such a poor job with (not) using this attribute as a way to improve code quality via decent warnings.

Basically, libvirt had an issue where we accidentally misused the nonnull attribute marker; we had added the marker in a header file, then later changed the C file to work with a NULL argument but without updating the header file to match: https://bugzilla.redhat.com/show_bug.cgi?id=815270

The calling code managed to pass in a NULL pointer to a parameter based on the semantics we saw in the .c file, but the NULL check in our function in question was "helpfully" elided by gcc since the attribute was still present, all without ever warning us that we were losing the NULL check.  As a result, the code misbehaved when it dereferenced NULL.  If gcc is smart enough to elide code based on the attribute, it should be smart enough to issue a warning that we have dead code, and the only reason the code is assumed to be dead is because of a suspicious attribute.  Had we had the warning, we would have known to either remove our dead NULL check, or to fix the header to no longer have the (now-inappropriate) attribute.

In other words, with a header like:

void foo(void *bar) __attribute__((nonnull(1)));

and a C file like:

void foo(void *bar) { if (!bar) abort(); }

Even if you decide that you are unable to warn about a call to foo(var) because the only way to analyze that var might be NULL is in the middle end but the warning is only emitted by the front end, AT LEAST you could have warned that the 'if (!bar)' conditional is assumed to be dead code based on the attribute placed on var.

(In reply to comment #8)
> Even if you decide that you are unable to warn about a call to foo(var) because
> the only way to analyze that var might be NULL is in the middle end but the
> warning is only emitted by the front end, AT LEAST you could have warned that
> the 'if (!bar)' conditional is assumed to be dead code based on the attribute
> placed on var.

GCC does not warn about unreachable code anymore:

http://gcc.gnu.org/ml/gcc-help/2011-05/msg00360.html

Someone would have to contribute a new -Wunreachable-code implementation. I honestly don't know what kind of implementation would be acceptable by GCC maintainers. Maybe one implemented in the FE? If so, it would require exactly the same infrastructure necessary to implement what Ulrich asks for above.

Sorry, comment #7 still applies.

*** Bug 46936 has been marked as a duplicate of this bug. ***

(In reply to Eric Blake from comment #8)
> In other words, with a header like:
> 
> void foo(void *bar) __attribute__((nonnull(1)));
> 
> and a C file like:
> 
> void foo(void *bar) { if (!bar) abort(); }
> 
> Even if you decide that you are unable to warn about a call to foo(var)
> because the only way to analyze that var might be NULL is in the middle end
> but the warning is only emitted by the front end, AT LEAST you could have
> warned that the 'if (!bar)' conditional is assumed to be dead code based on
> the attribute placed on var.

This part has now been implemented:
https://gcc.gnu.org/ml/gcc-patches/2015-09/msg00626.html

(In reply to Mark Wielaard from comment #11)
> This part has now been implemented:
> https://gcc.gnu.org/ml/gcc-patches/2015-09/msg00626.html

And updated in
https://gcc.gnu.org/ml/gcc-patches/2016-02/msg01103.html

It should be noted that current implementation is conflict with assertions in some cases, as in https://lists.fedoraproject.org/archives/list/devel@lists.fedoraproject.org/thread/VA2QCXJGXVLH43327TRR5UM2BP52DWIC/. The purpose of such an attribute is not emphasized in the documentation, which leads to the confusion.

I'm actually not against the semantics it implies, but I find the warning actually reduce its availability for other reasons.

Note `assert` is required to be supported by the standard, so it will work across different implementations, including those do not support `__attribute__` at all. To get the code portable, the attributes will always be wrapped in macros whose names are not reserved identifiers. It should be natural that by conditionally defining such macros as nothing, each configuration of every implementation would be happy eventually, because modifying assertions everywhere in current code (instead modifying the macros) is obviously infeasible in general.

A question is, when to disable the attribute?

For debug build configurations (with `NDEBUG` undefined), it has almost no side effect by conditional definition based on some conditions (like `NDEBUG`), except one: the conditions may have to be duplicated in #ifdef/#if in different places of the code. It is still not a big trouble for `assert` normally, but it can be a disaster for custom assertion macros when the conditions are complicated enough. And though rarely, it can be a nightmare after the condition macros are defined (after necessary #undef) more than once, by different authors of the code.

For release configurations (such as builds with properly defined `NDEBUG`), the problem is more serious. Since the attribute is designed as a hint of optimization, it should not be defined as nothing besides debugging. But to modify all occurrence of assertions is still even more absurd, whether they will generate code or not.

So there is a dilemma: which one to keep, the assertions or the attribute? Removing the assertions is infeasible; unconditionally disabling the attribute is illogical (and totally nonsense for old versions of GCC which I need to adapt to). And clearly, I don't want to maintain multiple copies of the code just for different build configurations; I definitely need both of them in such cases. Thus either choice is essentially bad.

The final workaround (*not the fix*) is to disable the warning itself, which is configuration-neutral and it should does work at least as old versions of GCC. However, old compiler drivers do not recognize such options. To maintain the divergence in build scripts is dirty. So it should better be... pragmas? The last problem: #pragma or _Pragma to disable such diagnostics does not work. Is it a bug, or unsupported at all?

Testing a patch.

Patch posted for review:
https://gcc.gnu.org/ml/gcc-patches/2016-12/msg00399.html

Author: msebor
Date: Wed Dec 14 17:23:16 2016
New Revision: 243661

URL: https://gcc.gnu.org/viewcvs?rev=243661&root=gcc&view=rev
Log:
PR c/78673 - sprintf missing attribute nonnull on destination argument
PR c/17308 - nonnull attribute not as useful as it could be

gcc/ChangeLog:

	PR c/17308
	* builtin-attrs.def (ATTR_NONNULL_1_1, ATTR_NONNULL_1_2): Defined.
	(ATTR_NONNULL_1_3, ATTR_NONNULL_1_4, ATTR_NONNULL_1_5): Same.
	(ATTR_NOTHROW_NONNULL_1_1, ATTR_NOTHROW_NONNULL_1_2): Same.
	(ATTR_NOTHROW_NONNULL_1_3, ATTR_NOTHROW_NONNULL_1_4): Same.
	(ATTR_NOTHROW_NONNULL_1_5): Same.
	(ATTR_NONNULL_1_FORMAT_PRINTF_1_2): Same.
	(ATTR_NONNULL_1_FORMAT_PRINTF_2_0): Same.
	(ATTR_NONNULL_1_FORMAT_PRINTF_2_3): Same.
	(ATTR_NONNULL_1_FORMAT_PRINTF_3_0): Same.
	(ATTR_NONNULL_1_FORMAT_PRINTF_3_4): Same.
	(ATTR_NONNULL_1_FORMAT_PRINTF_4_0): Same.
	(ATTR_NONNULL_1_FORMAT_PRINTF_4_5): Same.
	* builtins.c (validate_arg): Add argument.  Treat null pointers
	passed to nonnull arguments as invalid.
	(validate_arglist): Same.
	* builtins.def (fprintf, fprintf_unlocked): Add nonnull attribute.
	(printf, printf_unlocked, sprintf. vfprintf, vsprintf): Same.
	(__sprintf_chk, __vsprintf_chk, __fprintf_chk, __vfprintf_chk): Same.
	* calls.c (get_nonnull_ags, maybe_warn_null_arg): New functions.
	(initialize_argument_information): Diagnose null pointers passed to
	arguments declared nonnull.
	* calls.h (get_nonnull_args): Declared.

gcc/c-family/ChangeLog:

	PR c/17308
	* c-common.c (check_nonnull_arg): Disable when optimization
	is enabled.

gcc/testsuite/ChangeLog:

	PR c/17308
	* gcc.dg/builtins-nonnull.c: New test.
	* gcc.dg/nonnull-4.c: New test.


Added:
    trunk/gcc/testsuite/gcc.dg/builtins-nonnull.c
    trunk/gcc/testsuite/gcc.dg/nonnull-4.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/builtin-attrs.def
    trunk/gcc/builtins.c
    trunk/gcc/builtins.def
    trunk/gcc/c-family/ChangeLog
    trunk/gcc/c-family/c-common.c
    trunk/gcc/calls.c
    trunk/gcc/calls.h
    trunk/gcc/testsuite/ChangeLog

Implemented in 7.0 in r243661.

This gives a warning in powerpc-linuc (which breaks bootstrap), when
compiling tree-inline.c:

/home/segher/src/gcc/gcc/vec.h:1613:5: error: argument 1 null where non-null expected [-Werror=nonnull]
   memset (&(address ()[oldlen]), 0, sz);
   ^~~~~~

Configuring with --disable-werror ends with a bootstrap comparison failure:

Bootstrap comparison failure!
gcc/gcc.o differs
gcc/plugin.o differs

(which could of course be something else).

powerpc64-linux, even.

I've raised bug 78817 for the powerpc64le bootstrap failure.

The patch was reverted, see PR 78817 on the reasons why.

*** Bug 95515 has been marked as a duplicate of this bug. ***

The patch which would have "fixed" this was reverted as there was too many false positives and that happens when you do optimization based warnings ...
I don't know if we want to keep this open or close this as won't fix though. 
It has been 6 years since this was tried too and we still most likely have just as many false positives as before even.
I also suspect many of these new warnings we are doing in recent years really should not be part of -Wall because of how many false positives we have. GCC has been getting a recent attention because of the false positives warnings too.

(In reply to Andrew Pinski from comment #23)
> I also suspect many of these new warnings we are doing in recent years
> really should not be part of -Wall because of how many false positives we
> have. GCC has been getting a recent attention because of the false positives
> warnings too.

Current documentation says

'-Wall'
     This enables all the warnings about constructions that some users
     consider questionable, and that are easy to avoid (or modify to
     prevent the warning), even in conjunction with macros.

and

'-Wextra'
     This enables some extra warning flags that are not enabled by
     '-Wall'.

We don't document at all what options should be enabled by -Wall, what
options should be enabled by -W, and which should be done by neither.  The
current documentation for -Wall is very noncommittal.

It all is a tradeoff of course.  IMO our documentation should make that clear
as well.

In my view, -Wall should enable all warnings that have few false positives
(less than 5% or 10%, say), and when they do this is easy to avoid, or perhaps
the warning points out very important (security) problems.

-W is the same but with higher tolerance for false positives.  And the
warnings that are not so useful, or are hard to avoid, and have a high false
positive rate, should not be enabled by either.

(Oh, and please note that -Werror is not part of these considerations at all.
When -Werror makes things "break" this is just a learning opportunity for
whoever asked for it.  Maybe we should document -Werror as an alias of
-Wself-flagellation).