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

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.


================
Comment at: clang/lib/Sema/SemaDecl.cpp:8880
+      }
+    }
+
----------------
tambre wrote:
> 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.
That makes sense to me in principle.  I'm definitely concerned about `noexcept` differences causing C library functions to not be treated as builtins, though; that seems stricter than we want.  How reasonable is it to weaken this?


================
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)) {
----------------
tambre wrote:
> 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`.
That's interesting.  It definitely seems wrong to apply builtin logic to a function that doesn't have a compatible low-level signature.  My inclination is to disable builtin checking here, but to notify the contributors so that they can figure out an appropriate response.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77491