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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits