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

[Aaron Ballman 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>


AaronBallman wrote:

> > > @AaronBallman I don't think [#158626 (review)](https://github.com/llvm/llvm-project/pull/158626#pullrequestreview-3229443611) has really been addressed yet.
> > 
> > 
> > Thanks for raising the concern!
> > > If you think it's fine to basically undefine the behaviour of implementations that's fine with me, but I think we should acknowledge that we do that.
> > 
> > 
> > The behavior is undefined according to the standard, so this is 1) ensuring we get diagnostics for misuse,
> 
> I'm 100% on board with this. It's most likely user error which should be diagnosed. However, this can be achieved by adding `_Nonnull` instead. That doesn't have any optimization implications as opposed to `[[gnu::nonnull]]`.

True.

> > 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? When we did an audit of implementations for deciding whether you can pass null pointer and a zero count, we saw tons of evidence that null with a zero count was intentionally supported. I don't recall seeing anything about situations where there's no count provided, and passing a null pointer in those cases has been undefined behavior for 40+ years.

llvm-libc doesn't define the behavior: https://github.com/llvm/llvm-project/blob/438a18c1e105ca04e624239644195e48b28b5099/libc/src/stdio/printf_core/vfprintf_internal.h#L40
musl doesn't define the behavior: https://git.musl-libc.org/cgit/musl/tree/src/stdio/vfprintf.c#n681
BSD CRT doesn't define the behavior: https://github.com/freebsd/freebsd-src/blob/e878ba8eea7206b3a435338c6eed0e4264e0ce14/lib/libc/stdio/vfprintf.c#L277
I couldn't figure out where the actual implementation lives in glibc, but I would be surprised if they defined the behavior either. I stopped looking at libc implementations after that.

> 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

CC @michaelrj-google for additional input

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