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

[Raul Tambre via Phabricator via cfe-commits]

tambre added inline comments.


================
Comment at: clang/lib/Sema/SemaDecl.cpp:8880
+      }
+    }
+
----------------
rjmccall wrote:
> 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?
I agree having `noexcept` weakened is reasonable.
I've changed it to create an identical type to the NewFD with the exception spec removed for the comparison. This fixes it.


================
Comment at: clang/test/CodeGen/ms-intrinsics.c:23
 void test__stosb(unsigned char *Dest, unsigned char Data, size_t Count) {
   return __stosb(Dest, Data, Count);
 }
----------------
`__stosb` and friends aren't marked as builtin because they're declared as `static`.
I don't think there's a good reason to have builtins as `static` and we should simply remove the `static` specifier from those intrinsics in headers.
Alternatively, we could weaken compatibility checking similar to `noexcept`.
Thoughts?


================
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)) {
----------------
rjmccall wrote:
> 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.
Agreed.
I've removed this test, as there doesn't seem to be an easy way to replicate this behaviour.


================
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)));
 
----------------
tambre wrote:
> 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?
Fixed by removing `noexcept` for the declaration compatibility comparison.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77491