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
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
