[PATCH] D68028: [clang] Add no_builtin attribute

[Richard Smith - zygoloid via Phabricator via cfe-commits]

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