rsmith added inline comments.
================ Comment at: clang/lib/Sema/SemaDecl.cpp:9508-9510 + // FIXME: We should really be doing this in SemaDeclAttr.cpp::handleNoBuiltin + // but there is a bug with FunctionDecl::isThisDeclarationADefinition() which + // always returns false before Sema::ActOnStartOfFunctionDef is called. ---------------- aaron.ballman wrote: > rsmith wrote: > > aaron.ballman wrote: > > > handleNoBuiltin -> handleNoBuiltinAttr > > I am not convinced that this is a bug -- the function declaration does not > > become a definition until the parser reaches the definition. > > > > In any case, I don't think the predicate you want is "is this declaration a > > definition?". Rather, I think what you want is, "does this declaration > > introduce an explicit function body?". In particular, we should not permit > > uses of this attribute on defaulted or deleted functions, nor on functions > > that have a definition by virtue of using `__attribute__((alias))`. So it > > probably should be a syntactic check on the form of the declarator (that > > is, the check you're perrforming here), and the check should probably be > > `D.getFunctionDefinitionKind() == FDK_Definition`. (A custom diagnostic for > > using the attribute on a defaulted or deleted function would be nice too, > > since the existing diagnostic text isn't really accurate in those cases.) > > In particular, we should not permit uses of this attribute on defaulted or > > deleted functions > > Deleted functions, sure. Defaulted functions... not so sure. I could sort of > imagine wanting to instruct a defaulted assignment operator that does > memberwise copy that it's not allowed to use a builtin memcpy, for instance. > Or is this a bad idea for some reason I'm not thinking of? `-fno-builtin` does not turn off using `llvm.memcpy` for copying memory, and it doesn't turn off `llvm.memcpy` being lowered to a call to `memcpy`. Allowing this for defaulted functions would only give a false sense of security, at least for now (though I could imagine we might find some way to change that in the future). Also, trivial defaulted functions (where we're most likely to end up with `memcpy` calls) are often not emitted at all, instead being directly inlined by the frontend, so there's nowhere to attach a `no-builtin-memcpy` LLVM attribute (we'd need to put the attribute on all callers of those functions) even if LLVM learned to not emit calls to `memcpy` to implement `llvm.memcpy` when operating under a `no-builtin-memcpy` constraint. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D68028/new/ https://reviews.llvm.org/D68028 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
