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