[PATCH] D68028: [clang] Add no_builtin attribute

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D68028#1707931 <https://reviews.llvm.org/D68028#1707931>, @gchatelet wrote:

> A gentle ping @aaron.ballman


Sorry for the delay, I was traveling for last week.



================
Comment at: clang/include/clang/Basic/AttrDocs.td:4402
+The ``__attribute__((no_builtin))`` is similar to the ``-fno-builtin`` flag
+except it is specific to the body of a function.
+
----------------
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.`


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:3600
   "attribute">;
+def err_attribute_no_builtin_invalid_builtin_name : Error<
+  "'%0' is not a valid builtin name for %1">;
----------------
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.


================
Comment at: clang/lib/Sema/SemaDecl.cpp:9508
+  // definition.
+  // FIXME: We should really be doing this in SemaDeclAttr.cpp::handleNoBuiltin
+  // but there is a bug with FunctionDecl::isThisDeclarationADefinition() which
----------------
handleNoBuiltin -> handleNoBuiltinAttr


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


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:1103
+
+static void handleNoBuiltin(Sema &S, Decl *D, const ParsedAttr &AL) {
+  if (!checkAttributeAtLeastNumArgs(S, AL, 1))
----------------
Can you rename this to `handleNoBuiltinAttr` for consistency?


================
Comment at: clang/test/CodeGen/no-builtin.c:20
+void separate_attrs_ordering() __attribute__((no_builtin("memcpy"))) __attribute__((no_builtin("memset"))) {}
+
+// CHECK: attributes #0 = {{{.*}}"no-builtin-memcpy"{{.*}}}
----------------
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`.


================
Comment at: clang/test/Sema/no-builtin.c:1
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -S -emit-llvm -o - -verify %s
+
----------------
aaron.ballman wrote:
> This should not be emitting LLVM -- it should be `-fsyntax-only`, however.
This still has `-S -emit-llvm -o -`; that should be removed, no?


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68028