[PATCH] D77491: [Sema] Introduce BuiltinAttr, per-declaration builtin-ness

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

In D77491#2246948 <https://reviews.llvm.org/D77491#2246948>, @rjmccall wrote:

> I'd still like @rsmith to sign off here as code owner.

Generally, I'm happy with this direction.

What happens for builtins with the "t" (custom typechecking) flag, for which the signature is intended to have no meaning? Do we always give them builtin semantics, or never, or ... something else? I think it might be reasonable to require them to always be declared as taking unspecified arguments -- `()` in C and `(...)` in C++, or to simply say that the user cannot declare such functions themselves.



================
Comment at: clang/lib/Headers/intrin.h:148
 void __writemsr(unsigned long, unsigned __int64);
-static __inline__
-void *_AddressOfReturnAddress(void);
-static __inline__
-unsigned char _BitScanForward(unsigned long *_Index, unsigned long _Mask);
-static __inline__
-unsigned char _BitScanReverse(unsigned long *_Index, unsigned long _Mask);
+__inline__ void *_AddressOfReturnAddress(void);
+__inline__ unsigned char _BitScanForward(unsigned long *_Index,
----------------
Does the `__inline__` here do anything for a builtin function? Can we remove it along with the `static`?


================
Comment at: clang/lib/Headers/intrin.h:200
 void __incgsword(unsigned long);
-static __inline__
-void __movsq(unsigned long long *, unsigned long long const *, size_t);
+static __inline__ void __movsq(unsigned long long *, unsigned long long const *,
+                               size_t);
----------------
Why is `static` being removed from some of the functions in this header but not others?


================
Comment at: clang/lib/Sema/SemaDecl.cpp:9668-9694
+  // In C builtins get merged with implicitly lazily created declarations.
+  // In C++ we need to check if it's a builtin and add the BuiltinAttr here.
+  if (getLangOpts().CPlusPlus) {
+    if (IdentifierInfo *II = Previous.getLookupName().getAsIdentifierInfo()) {
+      if (unsigned BuiltinID = II->getBuiltinID()) {
+        FunctionDecl *D = CreateBuiltin(II, BuiltinID, NewFD->getLocation());
+
----------------
I think this needs more refinement:

* You appear to be creating and throwing away a new builtin function declaration (plus parameter declarations etc) each time you see a declaration with a matching name, even if one was already created. Given that you don't actually use `D` for anything other than its type, creating the declaration seems redundant and using `ASTContext::GetBuiltinType` would be more appropriate.
* There are no checks of which scope the new function is declared in; this appears to apply in all scopes, but some builtin names are only reserved in the global scope (those beginning with an underscore followed by a lowercase letter such as `_bittest`), so that doesn't seem appropriate. The old code in `FunctionDecl::getBuiltinID` checked that the declaration is given C language linkage (except for `_GetExceptionInfo`, which was permitted to have C++ language linkage), and that check still seems appropriate to me.
* The special case permitting `_GetExceptionInfo` to be declared with *any* type seems suspect; the old code permitted it to have different language linkage, not the wrong type.
* Using `typesAreCompatible` in C++-specific code is weird, since C++ doesn't have a notion of "compatible types".


================
Comment at: clang/lib/Serialization/ASTReader.cpp:975
   bool HasRevertedTokenIDToIdentifier = readBit(Bits);
-  bool HasRevertedBuiltin = readBit(Bits);
+  readBit(Bits); // Previously used to indicate reverted builtin.
   bool Poisoned = readBit(Bits);
----------------
We don't have any stability guarantees for our AST bitcode format yet; you can just remove this bit rather than retaining a placeholder.


================
Comment at: clang/test/Analysis/bstring.cpp:106
   Derived d;
-  memset(&d.d_mem, 0, sizeof(Derived));
+  memset(&d.d_mem, 0, sizeof(Derived)); // expected-warning {{'memset' will always overflow; destination buffer has size 4, but size argument is 8}}
   clang_analyzer_eval(d.b_mem == 0); // expected-warning{{UNKNOWN}}
----------------
This should not be recognized as a builtin, because the `memset` function is not `extern "C"`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77491/new/

https://reviews.llvm.org/D77491