[PATCH] D68028: [clang] Add no_builtin attribute

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman marked an inline comment as done.
aaron.ballman added inline comments.


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:1092
+  // Wildcard is a super set of all builtins, we keep only this one.
+  if (FunctionNames.count(Wildcard) > 0) {
+    FunctionNames.clear();
----------------
gchatelet wrote:
> aaron.ballman wrote:
> > This silently changes the behavior from what the user might expect, which seems bad. For instance, it would be very reasonable for a user to write `[[clang::no_builtin("__builtin_mem*")]]` expecting that to behave as a regex pattern, but instead it silently turns into a "disable all builtins".
> > 
> > I think it makes sense to diagnose unexpected input like that rather than silently accept it under perhaps unexpected semantics. It may also make sense to support regex patterns in the strings or at least keep the design such that we can add that functionality later without breaking users.
> The code looks for a function name that is exactly `*` and not "a function name that contains `*`", it would not turn `[[clang::no_builtin("__builtin_mem*")]]` into `[[clang::no_builtin("*")]]`.
> That said, I fully agree that an error should be returned if the function name is not valid.
> I'll work on this.
Ah, I missed that -- great!


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:1072
+NoBuiltinAttr *
+Sema::mergeNoBuiltinAttr(Sema &S, Decl *D, const AttributeCommonInfo &CI,
+                         llvm::ArrayRef<StringRef> FunctionNames) {
----------------
You're missing a call to this function within `mergeDeclAttribute()` in SemaDecl.cpp.


================
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);
----------------
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?


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:1082-1083
+  // Insert new NoBuiltin attributes.
+  for (StringRef FunctionName : FunctionNames)
+    FunctionNamesSet.insert(FunctionName);
+
----------------
Same question here about using `llvm::copy()`.


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


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


================
Comment at: clang/test/Sema/no-builtin.c:1
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -S -emit-llvm -o - -verify %s
+
----------------
This should not be emitting LLVM -- it should be `-fsyntax-only`, however.


================
Comment at: clang/test/Sema/no-builtin.c:9
+
+int __attribute__((no_builtin("*"))) variable; // expected-warning {{'no_builtin' attribute only applies to functions}}
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68028