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

Wed Sep 2 21:17:33 PDT 2020

tambre added a comment.

Thanks for the review!
I believe I've managed to address your comments.

In D77491#2248454 <https://reviews.llvm.org/D77491#2248454>, @rsmith wrote:

> 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.

The options I see are:

1. Disallow.
2. Run the custom typechecking.
3. Require as taking unspecified arguments.
4. Simply check the name.

Implementing running custom typechecking seems it'd be tricky and not worth it.
Disallowing isn't an option as a cursory search shows that this is used in the wild.
Requiring unspecified arguments isn't feasible either as the real-world usages seem to include arguments in their declarations. E.g. __va_start from MSVC's vadefs.h <https://github.com/ojdkbuild/tools_toolchain_vs2017bt/blob/ee20a12c95b6a8b5942bf66a48424f61d560e938/VC/Tools/MSVC/14.12.25827/include/vadefs.h#L64>, which is also tested by `clang/test/SemaCXX/microsoft-varargs.cpp`.

I've thus gone with simply marking as them builtins if the name matches and they're `extern "C"`. The C linkage requirement didn't exist before, but seems a safe improvement to make.

================
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,
----------------
rsmith wrote:
> Does the `__inline__` here do anything for a builtin function? Can we remove it along with the `static`?
It does not. Removed from all.

================
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);
----------------
rsmith wrote:
> Why is `static` being removed from some of the functions in this header but not others?
I removed `static` from builtins that showed up as problematic in tests. But like `inline`, there's really no effect. I've removed both from all builtins in this header.

================
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());
+
----------------
rsmith wrote:
> 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".
> * 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.
Fixed.

> * 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.
Fixed.

> * 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.
I've fixed the linkage part, but the old code didn't have any type checking at all so I've kept it this way.

> * Using `typesAreCompatible` in C++-specific code is weird, since C++ doesn't have a notion of "compatible types".
Agreed, however I was unable to come up with a better way to do this. Let me know if you have any ideas.
That said, since I've now fixed it to check for C linkage I think this is less of a problem.

================
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}}
----------------
rsmith wrote:
> This should not be recognized as a builtin, because the `memset` function is not `extern "C"`.
Fixed by proper `extern "C"` handling.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77491