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@-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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
