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

[Raul Tambre via Phabricator via cfe-commits]

tambre marked 4 inline comments as done.
tambre added a comment.

Thanks for the reviews and design pointers, John!

There are still a few tests failing, but I'd rather not dive in before the general approach is considered acceptable.



================
Comment at: clang/lib/Sema/SemaDecl.cpp:8880
+      }
+    }
+
----------------
rjmccall wrote:
> Hmm.  I'm concerned about not doing any sort of semantic compatibility check here before we assign the function special semantics.  Have we really never done those in C++?
> 
> If not, I wonder if we can reasonably make an implicit declaration but just make it hidden from lookup.
Currently there's no semantic compatibility checking for builtin redeclarations. There is for initial declarations though.

I've added this checking by splitting the actual builtin declaration creation off from `LazilyCreateBuiltin` into `CreateBuiltin` and checking if the current declaration is compatible with what the builtin's would be.
This results in stronger typechecking than before for builtin declarations, so some incompatible declarations are no longer marked as builtin. See `cxx1z-noexcept-function-type.cpp` for an example.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:6047
   OverloadDecl->setParams(Params);
+  Sema->mergeDeclAttributes(OverloadDecl, FDecl);
   return OverloadDecl;
----------------
`rewriteBuiltinFunctionDecl()` creates a new function declaration and happened to disregard the builtin's attributes. This caused `BuiltinAttr` to go missing in a bunch of OpenCL tests, resulting in failures.


================
Comment at: clang/test/Sema/warn-fortify-source.c:20
 void *memcpy(void *dst, const void *src, size_t c);
 static void *memcpy(void *dst __attribute__((pass_object_size(1))), const void *src, size_t c) __attribute__((overloadable)) __asm__("merp");
 static void *memcpy(void *const dst __attribute__((pass_object_size(1))), const void *src, size_t c) __attribute__((overloadable)) {
----------------
Not quite sure what to do here. These were previously recognized as builtins due to their name despite being incompatible and thus had fortify checking similar to the real `memcpy`.

Maybe:
1. Introduce a generic version of `ArmBuiltinAliasAttr`.
2. Something like `FormatAttr`.


================
Comment at: clang/test/SemaCXX/cxx11-compat.cpp:35
+#define PRIuS "zu"
 int printf(const char *, ...);
 typedef __typeof(sizeof(int)) size_t;
----------------
For some reason this `printf` didn't get format checking before. Same for `SemaCXX/warn-unused-local-typedef.cpp`.


================
Comment at: clang/test/SemaCXX/cxx1z-noexcept-function-type.cpp:120
   extern "C" int strcmp(const char *, const char *);
-  extern "C" int strncmp(const char *, const char *, decltype(sizeof(0))) noexcept;
+  extern "C" int strncmp(const char *, const char *, decltype(sizeof(0)));
 
----------------
This doesn't work anymore since we now properly check builtin declaration compatibility and since C++17 noexcept is part of the function type but builtins aren't noexcept.
Thoughts?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77491