[PATCH] D68028: [clang] Add no_builtin attribute

[Guillaume Chatelet via Phabricator via cfe-commits]

gchatelet added inline comments.


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:3600
   "attribute">;
+def err_attribute_no_builtin_invalid_builtin_name : Error<
+  "'%0' is not a valid builtin name for %1">;
----------------
aaron.ballman wrote:
> I think this should be a warning rather than an error.
> 
> Imagine the situation where the user uses Clang 11 to write their code and they disable a Clang 11-specific builtin from being used, but then they try to compile the code in Clang 10 which doesn't know about the builtin.
> 
> Rather than err, it seems reasonable to warn about the unknown builtin name (under a new warning flag group under `-Wattributes`) and then just ignore the attribute. Because the builtin is unknown anyway, we won't transform any constructs to use it.
Makes perfect sense, thx for pointing this out.


================
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.
> I am not convinced that this is a bug -- the function declaration does not become a definition until the parser reaches the definition.

@rsmith It may not be a bug but it is currently used in a buggy manner (See D68379).
An assert to prevent misuse would be welcome IMHO, I've added a note on the function meanwhile.


================
Comment at: clang/test/Sema/no-builtin.c:26
+int __attribute__((no_builtin("*"))) variable;
+// expected-warning at -1 {{'no_builtin' attribute only applies to functions}}
----------------
aaron.ballman wrote:
> You should probably add another test case for this situation:
> ```
> void whatever() __attribute__((no_builtin("*", "*"))) {}
> ```
> and for member functions in C++ as well:
> ```
> struct S {
>   void whatever() __attribute__((no_builtin("memcpy"))) {}
>   virtual void intentional() __attribute__((no_builtin("memcpy"))) {}
> };
> ```
```
void whatever() __attribute__((no_builtin("*", "*"))) {}
```

I've added a few ones.

```
struct S {
  void whatever() __attribute__((no_builtin("memcpy"))) {}
  virtual void intentional() __attribute__((no_builtin("memcpy"))) {}
};
```

What do you want me to test for this one?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68028