[PATCH] D68028: [clang] Add no_builtin attribute

[Aaron Ballman via Phabricator via cfe-commits]

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:
> > 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?


================
Comment at: clang/test/Sema/no-builtin.c:7
+
+void no_builtin_no_argument() __attribute__((no_builtin)) {}
+// expected-error at -1 {{'no_builtin' attribute takes at least 1 argument}}
----------------
rsmith wrote:
> I find this surprising. I would expect this to work, and to mean the same as `-fno-builtin` for the function (that is, same as the `*` form).
> 
> I think might be a better design to remove the magical `"*"` form entirely, and use an empty argument list to mean "all builtins". Have you considered that? (That would better mirror how `-fno-builtin` vs `-fno-builtin-foo` works.)
> I think might be a better design to remove the magical "*" form entirely, and use an empty argument list to mean "all builtins".

I like this approach.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68028/new/

https://reviews.llvm.org/D68028