aaron.ballman 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. ---------------- rsmith wrote: > 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. Ah, good to know! We're in agreement on how this should proceed, thank you for the insights. 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
