[clang] Add no_builtin attribute

[gchatelet]

Summary

This is a follow up on https://reviews.llvm.org/D61634
This patch is simpler and only adds the no_builtin attribute.

[aaron.ballman]

clang/lib/Sema/SemaDeclAttr.cpp

1073

You're missing a call to this function within mergeDeclAttribute() in SemaDecl.cpp.

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.

[gchatelet]

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.
Indeed I think it only makes sense to have the attribute on the function definition.

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.
All calls to hasBody() return false, if I repeat the operation in CGCall then hasBody returns true and I can see the CompoundStatement.

Do you have any recommendations on where to perform the check?

[gchatelet]

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.
I believe it's because the CompoundStatement is not parsed at this point.

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.

[aaron.ballman]

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

So after some investigations it turns out that FunctionDecl::isThisDeclarationADefinition is buggy (returns always false) when called from ProcessDeclAttribute.

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

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)

You got four of the six. What about the use in handleObjCSuppresProtocolAttr() and the second use in handleAliasAttr()?

I'm unsure of how to fix this, it may be possible of using FunctionDecl::setWillHaveBodyin this switch.

I'm also unsure of a good way forward. @rsmith may have ideas too.

[gchatelet]

clang/lib/Sema/SemaDeclAttr.cpp

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()?

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.

[gchatelet]

clang/lib/Sema/SemaDeclAttr.cpp

1099–1100

I've tried to fix it in D68701. I'm pretty sure this is not the right way to do it though,
@rsmith any idea on how to proceed?

In the meantime I'll add a FIXME and move on with this patch if you don't mind.

[gchatelet]

• Address aaron ballman comments
• Checks function name validity and errors when passed 0 argument.
• Update documentation and rebase
• Rewrote mergeNoBuiltinAttr
• Address comments

[gchatelet]

• Remove leftovers

[gchatelet]

• rebase

[gchatelet]

It should be ready to submit, @aaron.ballman @courbet can you take a final look?

[courbet]

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.

[gchatelet]

• Address comments

[gchatelet]

• Fix missing rename

[gchatelet]

A gentle ping @aaron.ballman

[aaron.ballman]

In D68028#1707931, @gchatelet wrote:

A gentle ping @aaron.ballman

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?

[rsmith]

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.)

[aaron.ballman]

clang/lib/Sema/SemaDecl.cpp

9534–9536

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?

clang/test/Sema/no-builtin.c

7 ↗

(On Diff #224286)

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.

[rsmith]

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.

[aaron.ballman]

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.

[gchatelet]

• Address comments
• Fix missing rename
• Address comments
• Add warning category

[gchatelet]

clang/include/clang/Basic/DiagnosticSemaKinds.td

3627

Makes perfect sense, thx for pointing this out.

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.

@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.

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?

[aaron.ballman]

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)

What do you want me to test for this one?

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
};

[gchatelet]

• Change NoBuiltin from InheritableAttr to Attr
• Improve tests

[gchatelet]

• Improve tests

[gchatelet]

• Reverting whole file formatting

[gchatelet]

• Call sites to virtual functions are not annotated

[aaron.ballman]

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.)

[gchatelet]

• Fix documentation and Warning category

[gchatelet]

clang/include/clang/Basic/DiagnosticSemaKinds.td

3629

"invalid-no-builtin-names" LGTM

[gchatelet]

LGTY @aaron.ballman ?

[aaron.ballman]

LGTM!

[gchatelet]

In D68028#1723568, @aaron.ballman wrote:

LGTM!

Thx a lot for bearing with me and for your valuable comments !
Really appreciate it!