Page MenuHomePhabricator

[nonnull] Teach Clang to attach the nonnull LLVM attribute to declarations and calls instead of just definitions, and then teach it to *not* attach such attributes even if the source code contains them.
ClosedPublic

Authored by chandlerc on Mar 9 2017, 7:59 PM.

Details

Reviewers
hfinkel
rsmith
Summary

This follows the design direction discussed on cfe-dev here:
http://lists.llvm.org/pipermail/cfe-dev/2017-January/052066.html

The idea is that for C standard library builtins, even if the library
vendor chooses to annotate their routines with attribute((nonnull)),
we will ignore those attributes which pertain to pointer arguments that
have an associated size. This allows the widespread (and seemingly
reasonable) pattern of calling these routines with a null pointer and
a zero size. I have only done this for the library builtins currently
recognized by Clang, but we can now trivially add to this set. This will
be controllable with -fno-builtin if anyone should care to do so.

Note that this does *not* change the AST. As a consequence, warnings,
static analysis, and source code rewriting are not impacted.

This isn't even a regression on any platform as neither Clang nor LLVM
have ever put 'nonnull' onto these arguments for declarations. All this
patch does is enable it on other declarations while preventing us from
ever accidentally enabling it on these libc functions due to a library
vendor.

Event Timeline

chandlerc created this revision.Mar 9 2017, 7:59 PM

Are users allowed to call these routines with a null pointer and a non-zero size? Or can we assume that if the size is known to be non-zero at compile time, the pointers are not null?

I'm thinking we might miss optimization opportunities if it's not possible to mark the pointer parameters as nonnull.

Are users allowed to call these routines with a null pointer and a non-zero size? Or can we assume that if the size is known to be non-zero at compile time, the pointers are not null?

If the sizes are non-zero, all of these will access memory through the pointer.

But we don't currently have any way of easily expressing this in the IR. We could try something fancy like call-side attributes when the size is known, but it is pretty redundant -- we could just teach the same things that look at the attributes to know that these are reads or writes of a constant size...

I'm thinking we might miss optimization opportunities if it's not possible to mark the pointer parameters as nonnull.

Note that currently, *nothing marks these parameters as nonnull*. So in all the years that folks have used Clang and LLVM with libc headers that have an attribute((nonnull)), no one has found a performance problem with this as the root cause and fixed it.

To me, this indicates that this isn't that important of an optimization *for these libc routines*.

If someone ever comes up with a testcase where this is important thing for performance, I think we can cross the bridge of finding a fancier way to analyze things then.

majnemer added inline comments.
lib/AST/ASTContext.cpp
8791

1U to avoid overflow UB?

lib/CodeGen/CGCall.cpp
2082

Ditto.

chandlerc added inline comments.Mar 10 2017, 1:20 AM
lib/AST/ASTContext.cpp
8791

I'd mildly prefer an assert (or rely on UBSan) that it *doesn't* overflow.

Also note that this is the same pattern we use just a few lines down for the ICE argument bitmask -- we have a pretty baked in assumption that all the builtins here don't have more than 31 arguments.

Anyways, happy to make whatever change you would prefer here. I just copied the code below, I don't have any strong opinion here.

lib/CodeGen/CGCall.cpp
2082

See above, I'll keep these in sync.

However, this exposes a related bug: we shouldn't be even doing this bit test if the function isn't a builtin. I'll add a check for that. That seems cleaner than relying on the bittest itself failing.

chandlerc updated this revision to Diff 91286.Mar 10 2017, 1:35 AM

Update to fix subtle bugs in handling arguments. Thanks to David for the review
comments that caused me to see the issue here.

hfinkel added inline comments.
include/clang/AST/ASTContext.h
1865

Please add some description of the new parameters.

1868

It seems like you had to touch more code than necessary because you decided to add these parameters before IntegerConstantArgs. Why don't you add them afterward instead?

chandlerc added inline comments.Mar 16 2017, 7:09 PM
include/clang/AST/ASTContext.h
1868

It seemed more natural.... But I'm happy to change the order. I'm not worreid about how much code I have to touch, I'd rather just get the options in the right place. What order would you suggest?

hfinkel added inline comments.Mar 16 2017, 7:23 PM
include/clang/AST/ASTContext.h
1868

I suggest the other order because it is more common to want the IntegerConstantArgs than the nonnull args (which essentially only one caller wants).

chandlerc marked an inline comment as done.Mar 17 2017, 10:41 PM

Function argument order fixed.

chandlerc updated this revision to Diff 92245.Mar 17 2017, 10:42 PM

Update with fixes suggested in review.

hfinkel accepted this revision.Mar 18 2017, 6:40 PM

LGTM

This revision is now accepted and ready to land.Mar 18 2017, 6:40 PM

Coming from D50039. This patch was accepted but never merged?

efriedma closed this revision.Sep 27 2018, 2:26 PM
efriedma added a subscriber: efriedma.
spatel added a subscriber: spatel.Sep 27 2018, 4:09 PM

But it got reverted soon after due to miscompiles:
https://reviews.llvm.org/rL298695

And the functionality hasn't been reimplemented AFAICT.