This is a follow up on https://reviews.llvm.org/D61634
This patch is simpler and only adds the no_builtin attribute.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
clang/lib/Sema/SemaDeclAttr.cpp | ||
---|---|---|
1073 | You're missing a call to this function within mergeDeclAttribute() in SemaDecl.cpp. | |
1099–1100 | Just making sure I understand the semantics you want: redeclarations do not have to have matching lists of builtins, but instead the definition will use a merged list? e.g., [[clang::no_builtin("memset")]] void whatever(); [[clang::no_builtin("memcpy")]] void whatever(); [[clang::no_builtin("memmove")]] void whatever() { // Will not use memset, memcpy, or memmove builtins. } That seems a bit strange, to me. In fact, being able to apply this attribute to a declaration seems a bit like a mistake -- this only impacts the definition of the function, and I can't imagine good things coming from hypothetical code like: [[clang::no_builtin("memset")]] void whatever(); #include "whatever.h" // Provides a library declaration of whatever() with no restrictions on it WDYT about restricting this attribute to only appear on definitions? | |
clang/test/Sema/no-builtin.c | ||
1 ↗ | (On Diff #222163) | This should not be emitting LLVM -- it should be -fsyntax-only, however. |
9 ↗ | (On Diff #222163) | We'll need more tests for redeclarations or declaration vs definition once we decide how we want to handle that. You should also add some positive tests that show the attribute applying as expected. |
thx @aaron.ballman , I'm waiting for your reply before uploading the new patch (some of my comments won't have the accompanying code update sorry)
clang/lib/Sema/SemaDeclAttr.cpp | ||
---|---|---|
1073 | Thx, I rearranged the signature a bit, do you happen to know how mergeDeclAttribute is tested? | |
1079–1080 | I had to use a vector instead of a set and uniquify by hand. | |
1087–1090 | This is is conflict with the llvm::copy suggestion above. Which one do you prefer? | |
1099–1100 | That's a very good point. Thx for noticing. I've tried to to use FunctionDecl->hasBody() during attribute handling in the Sema phase but it seems like the FunctionDecl is not complete at this point. Do you have any recommendations on where to perform the check? |
clang/lib/Sema/SemaDeclAttr.cpp | ||
---|---|---|
1099–1100 | So after some investigations it turns out that FunctionDecl::isThisDeclarationADefinition is buggy (returns always false) when called from ProcessDeclAttribute. As a matter of fact all code using this function in ProcessDeclAttribute is dead code (see D68379 which disables the dead code, tests are still passing) I'm unsure of how to fix this, it may be possible of using FunctionDecl::setWillHaveBodyin this switch. |
clang/lib/Sema/SemaDeclAttr.cpp | ||
---|---|---|
1073 | Through redeclarations, e.g., [[some_attr]] void func(); [[some_other_attr]] void func(); [[a_third_attr, some_attr]] void func() { } | |
1087–1090 | Walking the list and not calling llvm::copy. | |
1099–1100 |
That is a bit odd to me; we call it in a half dozen places in SemaDeclAttr.cpp, all of which get called from ProcessDeclAttribute. Are ALL of those uses broken currently?!
You got four of the six. What about the use in handleObjCSuppresProtocolAttr() and the second use in handleAliasAttr()?
I'm also unsure of a good way forward. @rsmith may have ideas too. |
clang/lib/Sema/SemaDeclAttr.cpp | ||
---|---|---|
1099–1100 |
It really is FunctionDecl::isThisDeclarationADefinition that is buggy, the other places where we call isThisDeclarationADefinition in SemaDeclAttr.cpp are ok, i.e. VarDecl::isThisDeclarationADefinition and ObjCProtocolDecl::isThisDeclarationADefinition I tried to use FunctionDecl::setWillHaveBodybut it broke many tests so it seems inappropriate. |
- Address aaron ballman comments
- Checks function name validity and errors when passed 0 argument.
- Update documentation and rebase
- Rewrote mergeNoBuiltinAttr
- Address comments
clang/include/clang/Basic/Attr.td | ||
---|---|---|
3433 | [nit] Should this be BuiltinNames ? builtins are not necessarily functions. | |
clang/include/clang/Basic/AttrDocs.td | ||
4409 | I would phrase it as The compiler is not allowed to add any builtin to foo's body. This covers inserting as well as replacing code with builtins. |
Sorry for the delay, I was traveling for last week.
clang/include/clang/Basic/AttrDocs.td | ||
---|---|---|
4402 | I'd appreciate a blurb in here to the effect: The attribute may also be applied to a virtual function but has no effect on the behavior of overriding functions in a derived class. | |
clang/include/clang/Basic/DiagnosticSemaKinds.td | ||
3627 | 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. | |
clang/lib/Sema/SemaDecl.cpp | ||
9534 | handleNoBuiltin -> handleNoBuiltinAttr | |
clang/lib/Sema/SemaDeclAttr.cpp | ||
1073 | Sorry for the code churn, but we realized we wanted this only on definitions after we started down the declaration merging path. I think we do not need mergeNoBuiltinAttr() any longer and can remove most of this logic entirely -- you cannot re-define functions, so we don't have any attributes to merge together. e.g., we no longer have this situation: void whatever() __attribute__((no_builtin("memcpy"))); void whatever() __attribute__((no_builtin("memmove"))) {} // Should ignore both memcpy and memmove I think the only part that needs to move over to handleNoBuiltinAttr() is the part diagnosing a combination of wildcard and non-wildcard arguments. As a thought for a future patch (not requesting this be done now, unless you want to), should we fix-it (or possibly support) this situation? void whatever() __attribute__((no_builtin("memcpy, memmove"))) {} // Note that it's only one string literal | |
1104 | Can you rename this to handleNoBuiltinAttr for consistency? | |
clang/test/CodeGen/no-builtin.c | ||
20 ↗ | (On Diff #224286) | I'd like to see a codegen case that ensures we don't do bad things with virtual function overrides: struct S { virtual void foo() __attribute__((no_builtin("memcpy"))) {} }; struct D : S { void foo() __attribute__((no_builtin("memmove"))) {} }; We should ensure that S::foo() only prohibits memcpy and D::foo() only prohibits memmove. |
clang/test/Sema/no-builtin.c | ||
26 ↗ | (On Diff #224286) | 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"))) {} }; |
1 ↗ | (On Diff #222163) | This still has -S -emit-llvm -o -; that should be removed, no? |
clang/lib/Sema/SemaDecl.cpp | ||
---|---|---|
9534–9536 | 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.) | |
clang/test/Sema/no-builtin.c | ||
7 ↗ | (On Diff #224286) | 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.) |
clang/lib/Sema/SemaDecl.cpp | ||
---|---|---|
9534–9536 |
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? | |
clang/test/Sema/no-builtin.c | ||
7 ↗ | (On Diff #224286) |
I like this approach. |
clang/lib/Sema/SemaDecl.cpp | ||
---|---|---|
9534–9536 | -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. |
clang/lib/Sema/SemaDecl.cpp | ||
---|---|---|
9534–9536 | Ah, good to know! We're in agreement on how this should proceed, thank you for the insights. |
clang/include/clang/Basic/DiagnosticSemaKinds.td | ||
---|---|---|
3627 | Makes perfect sense, thx for pointing this out. | |
clang/lib/Sema/SemaDecl.cpp | ||
9534–9536 |
@rsmith It may not be a bug but it is currently used in a buggy manner (See D68379). | |
clang/test/Sema/no-builtin.c | ||
26 ↗ | (On Diff #224286) | 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? |
clang/include/clang/Basic/Attr.td | ||
---|---|---|
3431 | I think we do not want this to be an inheritable attribute, but just Attr instead, because this can only be written on definitions (so there's no way to inherit the attribute from previous declarations). | |
clang/test/Sema/no-builtin.c | ||
26 ↗ | (On Diff #224286) |
Ensuring that applying the attribute to member function definitions, including virtual ones, is supported and doesn't cause diagnostics. The codegen side of things will test that overridden virtual methods do not inherit the attribute. Actually, there's another interesting test hiding in there for when we do want a diagnostic (I think): struct S { void whatever() __attribute__((no_builtin("memcpy"))); // Should diagnose as not a definition }; |
clang/include/clang/Basic/AttrDocs.td | ||
---|---|---|
4407 | This mention of * is now out of date. | |
4412 | This one too. | |
clang/include/clang/Basic/DiagnosticSemaKinds.td | ||
3629 | Hmm, I thought there was a way you could do: InGroup<DiagGroup<"invalid-no-builtin-names", IgnoredAttributes>>; but it seems I've got the order reversed and you'd have to add a new diagnostic group that IgnoredAttributes could subsume. We don't do that for other attributes, so I think you should just do: InGroup<DiagGroup<"invalid-no-builtin-names">>; (Feel free to pick a better diagnostic group name if you have a better idea, I'm not tied to my suggestion.) |
clang/include/clang/Basic/DiagnosticSemaKinds.td | ||
---|---|---|
3629 | "invalid-no-builtin-names" LGTM |