[PATCH] D30806: [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.

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc added inline comments.


================
Comment at: lib/AST/ASTContext.cpp:8786
+    if (OverrideNonnull && OverrideNonnullArgs)
+      *OverrideNonnullArgs |= 1 << ArgTypes.size();
+
----------------
majnemer wrote:
> `1U` to avoid overflow UB?
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.


================
Comment at: lib/CodeGen/CGCall.cpp:2082
+      if (getNonNullAttr(FD, PVD, ParamType, PVD->getFunctionScopeIndex()) &&
+          (OverrideNonnullArgs & (1 << ArgNo)) == 0)
+        Attrs.addAttribute(llvm::Attribute::NonNull);
----------------
majnemer wrote:
> Ditto.
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.


https://reviews.llvm.org/D30806