[PATCH] D68028: [clang] Add no_builtin attribute

[Guillaume Chatelet via Phabricator via cfe-commits]

gchatelet marked 7 inline comments as done.
gchatelet added a comment.

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)



================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:1072
+NoBuiltinAttr *
+Sema::mergeNoBuiltinAttr(Sema &S, Decl *D, const AttributeCommonInfo &CI,
+                         llvm::ArrayRef<StringRef> FunctionNames) {
----------------
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?


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:1078-1079
+  // Insert previous NoBuiltin attributes.
+  if (D->hasAttr<NoBuiltinAttr>())
+    for (StringRef FunctionName : D->getAttr<NoBuiltinAttr>()->functionNames())
+      FunctionNamesSet.insert(FunctionName);
----------------
aaron.ballman wrote:
> Instead of doing `hasAttr<>` followed by `getAttr<>`, this should be:
> ```
> if (const auto *NBA = D->getAttr<NoBuiltinAttr>()) {
>   for (StringRef FunctionName : NBA->functionNames())
>     ...
> }
> ```
> But are you able to use `llvm::copy()` instead of a manual loop?
I had to use a vector instead of a set and //uniquify// by hand.


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:1086-1089
+  if (FunctionNamesSet.count(Wildcard) > 0) {
+    FunctionNamesSet.clear();
+    FunctionNamesSet.insert(Wildcard);
+  }
----------------
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?


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:1098-1099
+
+  if (D->hasAttr<NoBuiltinAttr>())
+    D->dropAttr<NoBuiltinAttr>();
+
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68028