[PATCH] D68028: [clang] Add no_builtin attribute

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a reviewer: aaron.ballman.
aaron.ballman added a comment.

Thank you for working on this!

It looks like you're missing all of the sema tests that check that the attribute only appertains to functions, accepts the proper kind of arguments, etc.



================
Comment at: clang/include/clang/Basic/Attr.td:3349
+  let Spellings = [Clang<"no_builtin">];
+  let Args = [VariadicStringArgument<"FunctionNames">];
+  let Subjects = SubjectList<[Function]>;
----------------
The documentation should explain what this is.


================
Comment at: clang/lib/CodeGen/CGCall.cpp:1853
+    if (const auto *Attr = TargetDecl->getAttr<NoBuiltinAttr>()) {
+      const auto HasWildcard = llvm::is_contained(Attr->functionNames(), "*");
+      assert(!HasWildcard ||
----------------
`const auto` -> `bool` (and drop the top-level `const`).


================
Comment at: clang/lib/CodeGen/CGCall.cpp:1859
+      else
+        for (const auto &FunctionName : Attr->functionNames()) {
+          SmallString<32> AttributeName;
----------------
`const auto &` -> `StringRef`?


================
Comment at: clang/lib/CodeGen/CGCall.cpp:1861-1862
+          SmallString<32> AttributeName;
+          // TODO: check that function names are valid for the
+          // TargetLibraryInfo.
+          AttributeName += "no-builtin-";
----------------
I think this checking should happen in Sema rather than CodeGen.


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:1075-1078
+  // Insert previous NoBuiltin attributes.
+  if (D->hasAttr<NoBuiltinAttr>())
+    for (StringRef FunctionName : D->getAttr<NoBuiltinAttr>()->functionNames())
+      FunctionNames.insert(FunctionName);
----------------
I think that this should be done in a `merge` function; there are plenty of examples of how this is typically done elsewhere in the file.


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:1080-1081
+
+  if (AL.getNumArgs() == 0)
+    FunctionNames.insert(Wildcard);
+  else
----------------
Given that the user has to provide the parens for the attribute anyway, I think this situation should be diagnosed rather than defaulting to the wildcard. It helps catch think-os where the user put in parens and forgot to add the parameter in the first place (the wildcard is not onerous to spell out).


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:1091
+
+  // Wildcard is a super set of all builtins, we keep only this one.
+  if (FunctionNames.count(Wildcard) > 0) {
----------------
super set -> superset


================
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();
----------------
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.


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:1097
+
+  auto UniqueFunctionNames = FunctionNames.takeVector();
+  llvm::sort(UniqueFunctionNames);
----------------
Please don't use `auto` here as the type is not spelled out in the initialization.


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:1100-1101
+
+  if (D->hasAttr<NoBuiltinAttr>())
+    D->dropAttr<NoBuiltinAttr>();
+
----------------
I think this needs to happen in a `merge` function.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68028