[clang] [clang][sema] Add nonnull attribute to builtin format functions (PR #160988)

[Nikolas Klauser via cfe-commits]

Radovan =?utf-8?q?Božić?= <radovan.bozic at htecgroup.com>,
Radovan =?utf-8?q?Božić?= <radovan.bozic at htecgroup.com>,
Radovan =?utf-8?q?Božić?= <radovan.bozic at htecgroup.com>
Message-ID:
In-Reply-To: <llvm.org/llvm/llvm-project/pull/160988 at github.com>


philnik777 wrote:

> > > 2. improving optimization behavior. So it's not really the implementation undefining the behavior, it's the implementation admitting the behavior was already undefined (maybe too find of a distinction?).
> > 
> > 
> > What I mean is that some libc implementations seem define their behaviour when a nullptr is passed.
> 
> [citation needed] -- do you have evidence that they intentionally define the behavior?

Is this intentional enough: https://github.com/bminor/glibc/blob/26e48102108284d2474f83f5afee56b994c86d54/stdio-common/vfprintf-internal.c#L1533 ?

The macro expands to
```c
  do
    {
      CHECK_FILE (s, -1);
      if (s->_flags & _IO_NO_WRITES)
	{
	  s->_flags |= 0x0020;
	  __set_errno (9);
	  return -1;
	}
      if (format == ((void *) 0))
	{
	  __set_errno (22);
	  return -1;
	}
    }
  while (0);
```

> > This is effectively undone by the compiler adding the `[[gnu::nonnull]]` and the actual libc has no say in this. For hardened implementations this is a very real issue, see e.g. https://gcc.gnu.org/bugzilla/show_bug.cgi?id=121052. If this were to happen to a function libc++ defines I'd fight tooth and nail.
> > > gcc seems to treat the parameters as being marked nonnull: https://godbolt.org/z/YTb1ejh8W
> 
> Again, this is matching GCC's behavior and I think that is reasonable given how bad our codegen is otherwise: https://godbolt.org/z/9zh7KTGvG

Does this really matter though? Sure, the CodeGen doesn't look great, but I'd expect by far most format strings are literals. If they're generated by some function that could be annotated with `[[gnu::returns_nonnull]]` as well to get the same optimization behaviour, but is much safer, since a function can usually actually guarantee that it returns a non-null pointer. I also want to note that this is a single null check after a relatively heavy function, so I doubt this will have much of an impact. libcs can add the attributes themselves if it's deemed important enough. If you think this isn't a problem I don't want to block this, especially since I don't have any stake in this. I mostly want to make sure that this is thought through and we don't set this kind of precedent for C++ functions.


https://github.com/llvm/llvm-project/pull/160988