[PATCH] D68028: [clang] Add no_builtin attribute

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a subscriber: rsmith.
aaron.ballman added inline comments.


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:1072
+NoBuiltinAttr *
+Sema::mergeNoBuiltinAttr(Sema &S, Decl *D, const AttributeCommonInfo &CI,
+                         llvm::ArrayRef<StringRef> FunctionNames) {
----------------
gchatelet wrote:
> aaron.ballman wrote:
> > You're missing a call to this function within `mergeDeclAttribute()` in SemaDecl.cpp.
> Thx, I rearranged the signature a bit, do you happen to know how `mergeDeclAttribute` is tested?
Through redeclarations, e.g.,
```
[[some_attr]] void func();
[[some_other_attr]] void func();

[[a_third_attr, some_attr]] void func() {
}
```


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:1086-1089
+  if (FunctionNamesSet.count(Wildcard) > 0) {
+    FunctionNamesSet.clear();
+    FunctionNamesSet.insert(Wildcard);
+  }
----------------
gchatelet wrote:
> aaron.ballman wrote:
> > Rather than walking the entire set like this, would it make more sense to look for the wildcard in the above loop before inserting the name, and set a local variable if found, so that you can do the clear without having to rescan the entire list?
> This is is conflict with the `llvm::copy` suggestion above. Which one do you prefer?
Walking the list and not calling `llvm::copy`.


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:1098-1099
+
+  if (D->hasAttr<NoBuiltinAttr>())
+    D->dropAttr<NoBuiltinAttr>();
+
----------------
gchatelet wrote:
> gchatelet wrote:
> > aaron.ballman wrote:
> > > 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?
> > 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?
> 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::setWillHaveBody`in [[ https://github.com/llvm/llvm-project/blob/0577a0cedbc5be4cd4c20ba53d3dbdac6bff9a0a/clang/lib/Sema/SemaDecl.cpp#L8820 | this switch ]].
> 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68028