[PATCH] D68028: [clang] Add no_builtin attribute

[Guillaume Chatelet via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGbd8791610948: [clang] Add no_builtin attribute (authored by 
gchatelet).

Changed prior to commit:
  https://reviews.llvm.org/D68028?vs=225618=226687#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68028

Files:
  clang/include/clang/AST/Decl.h
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/CodeGen/no-builtin.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/Sema/no-builtin.cpp

Index: clang/test/Sema/no-builtin.cpp
===
--- /dev/null
+++ clang/test/Sema/no-builtin.cpp
@@ -0,0 +1,51 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -fsyntax-only -verify %s
+
+/// Prevent use of all builtins.
+void valid_attribute_all_1() __attribute__((no_builtin)) {}
+void valid_attribute_all_2() __attribute__((no_builtin())) {}
+
+/// Prevent use of specific builtins.
+void valid_attribute_function() __attribute__((no_builtin("memcpy"))) {}
+void valid_attribute_functions() __attribute__((no_builtin("memcpy"))) __attribute__((no_builtin("memcmp"))) {}
+
+/// Many times the same builtin is fine.
+void many_attribute_function_1() __attribute__((no_builtin)) __attribute__((no_builtin)) {}
+void many_attribute_function_2() __attribute__((no_builtin("memcpy"))) __attribute__((no_builtin("memcpy"))) {}
+void many_attribute_function_3() __attribute__((no_builtin("memcpy", "memcpy"))) {}
+void many_attribute_function_4() __attribute__((no_builtin("memcpy", "memcpy"))) __attribute__((no_builtin("memcpy"))) {}
+
+/// Invalid builtin name.
+void invalid_builtin() __attribute__((no_builtin("not_a_builtin"))) {}
+// expected-warning@-1 {{'not_a_builtin' is not a valid builtin name for no_builtin}}
+
+/// Can't use bare no_builtin with a named one.
+void wildcard_and_functionname() __attribute__((no_builtin)) __attribute__((no_builtin("memcpy"))) {}
+// expected-error@-1 {{empty no_builtin cannot be composed with named ones}}
+
+/// Can't attach attribute to a variable.
+int __attribute__((no_builtin)) variable;
+// expected-warning@-1 {{'no_builtin' attribute only applies to functions}}
+
+/// Can't attach attribute to a declaration.
+void nobuiltin_on_declaration() __attribute__((no_builtin));
+// expected-error@-1 {{no_builtin attribute is permitted on definitions only}}
+
+struct S {
+  /// Can't attach attribute to a defaulted function,
+  S()
+  __attribute__((no_builtin)) = default;
+  // expected-error@-1 {{no_builtin attribute has no effect on defaulted or deleted functions}}
+
+  /// Can't attach attribute to a deleted function,
+  S(const S &)
+  __attribute__((no_builtin)) = delete;
+  // expected-error@-1 {{no_builtin attribute has no effect on defaulted or deleted functions}}
+
+  void whatever() __attribute__((no_builtin("memcpy")));
+  // expected-error@-1 {{no_builtin attribute is permitted on definitions only}}
+};
+
+/// Can't attach attribute to an aliased function.
+void alised_function() {}
+void aliasing_function() __attribute__((no_builtin)) __attribute__((alias("alised_function")));
+// expected-error@-1 {{no_builtin attribute is permitted on definitions only}}
Index: clang/test/Misc/pragma-attribute-supported-attributes-list.test
===
--- clang/test/Misc/pragma-attribute-supported-attributes-list.test
+++ clang/test/Misc/pragma-attribute-supported-attributes-list.test
@@ -75,6 +75,7 @@
 // CHECK-NEXT: NSConsumed (SubjectMatchRule_variable_is_parameter)
 // CHECK-NEXT: NSConsumesSelf (SubjectMatchRule_objc_method)
 // CHECK-NEXT: Naked (SubjectMatchRule_function)
+// CHECK-NEXT: NoBuiltin (SubjectMatchRule_function)
 // CHECK-NEXT: NoCommon (SubjectMatchRule_variable)
 // CHECK-NEXT: NoDebug (SubjectMatchRule_type_alias, SubjectMatchRule_hasType_functionType, SubjectMatchRule_objc_method, SubjectMatchRule_variable_not_is_parameter)
 // CHECK-NEXT: NoDestroy (SubjectMatchRule_variable)
Index: clang/test/CodeGen/no-builtin.cpp
===
--- /dev/null
+++ clang/test/CodeGen/no-builtin.cpp
@@ -0,0 +1,64 @@
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -S -emit-llvm -o - %s | FileCheck %s
+
+// CHECK-LABEL: define void @foo_no_mempcy() #0
+extern "C" void foo_no_mempcy() __attribute__((no_builtin("memcpy"))) {}
+
+// CHECK-LABEL: define void @foo_no_mempcy_twice() #0
+extern "C" void foo_no_mempcy_twice() __attribute__((no_builtin("memcpy"))) __attribute__((no_builtin("memcpy"))) {}
+
+// CHECK-LABEL: define void @foo_no_builtins() #1
+extern "C" void foo_no_builtins() __attribute__((no_builtin)) {}
+
+// CHECK-LABEL: define void 

[PATCH] D68028: [clang] Add no_builtin attribute

[Guillaume Chatelet via Phabricator via cfe-commits]

gchatelet added a comment.

In D68028#1723568 , @aaron.ballman 
wrote:

> LGTM!


Thx a lot for bearing with me and for your valuable comments !
Really appreciate it!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68028



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68028: [clang] Add no_builtin attribute

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

LGTM!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68028



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68028: [clang] Add no_builtin attribute

[Guillaume Chatelet via Phabricator via cfe-commits]

gchatelet marked an inline comment as done.
gchatelet added a comment.

LGTY @aaron.ballman ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68028



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68028: [clang] Add no_builtin attribute

[Guillaume Chatelet via Phabricator via cfe-commits]

gchatelet updated this revision to Diff 225618.
gchatelet added a comment.

- Fix documentation and Warning category


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68028

Files:
  clang/include/clang/AST/Decl.h
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/CodeGen/no-builtin.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/Sema/no-builtin.cpp

Index: clang/test/Sema/no-builtin.cpp
===
--- /dev/null
+++ clang/test/Sema/no-builtin.cpp
@@ -0,0 +1,51 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -fsyntax-only -verify %s
+
+/// Prevent use of all builtins.
+void valid_attribute_all_1() __attribute__((no_builtin)) {}
+void valid_attribute_all_2() __attribute__((no_builtin())) {}
+
+/// Prevent use of specific builtins.
+void valid_attribute_function() __attribute__((no_builtin("memcpy"))) {}
+void valid_attribute_functions() __attribute__((no_builtin("memcpy"))) __attribute__((no_builtin("memcmp"))) {}
+
+/// Many times the same builtin is fine.
+void many_attribute_function_1() __attribute__((no_builtin)) __attribute__((no_builtin)) {}
+void many_attribute_function_2() __attribute__((no_builtin("memcpy"))) __attribute__((no_builtin("memcpy"))) {}
+void many_attribute_function_3() __attribute__((no_builtin("memcpy", "memcpy"))) {}
+void many_attribute_function_4() __attribute__((no_builtin("memcpy", "memcpy"))) __attribute__((no_builtin("memcpy"))) {}
+
+/// Invalid builtin name.
+void invalid_builtin() __attribute__((no_builtin("not_a_builtin"))) {}
+// expected-warning@-1 {{'not_a_builtin' is not a valid builtin name for no_builtin}}
+
+/// Can't use bare no_builtin with a named one.
+void wildcard_and_functionname() __attribute__((no_builtin)) __attribute__((no_builtin("memcpy"))) {}
+// expected-error@-1 {{empty no_builtin cannot be composed with named ones}}
+
+/// Can't attach attribute to a variable.
+int __attribute__((no_builtin)) variable;
+// expected-warning@-1 {{'no_builtin' attribute only applies to functions}}
+
+/// Can't attach attribute to a declaration.
+void nobuiltin_on_declaration() __attribute__((no_builtin));
+// expected-error@-1 {{no_builtin attribute is permitted on definitions only}}
+
+struct S {
+  /// Can't attach attribute to a defaulted function,
+  S()
+  __attribute__((no_builtin)) = default;
+  // expected-error@-1 {{no_builtin attribute has no effect on defaulted or deleted functions}}
+
+  /// Can't attach attribute to a deleted function,
+  S(const S &)
+  __attribute__((no_builtin)) = delete;
+  // expected-error@-1 {{no_builtin attribute has no effect on defaulted or deleted functions}}
+
+  void whatever() __attribute__((no_builtin("memcpy")));
+  // expected-error@-1 {{no_builtin attribute is permitted on definitions only}}
+};
+
+/// Can't attach attribute to an aliased function.
+void alised_function() {}
+void aliasing_function() __attribute__((no_builtin)) __attribute__((alias("alised_function")));
+// expected-error@-1 {{no_builtin attribute is permitted on definitions only}}
Index: clang/test/Misc/pragma-attribute-supported-attributes-list.test
===
--- clang/test/Misc/pragma-attribute-supported-attributes-list.test
+++ clang/test/Misc/pragma-attribute-supported-attributes-list.test
@@ -74,6 +74,7 @@
 // CHECK-NEXT: NSConsumed (SubjectMatchRule_variable_is_parameter)
 // CHECK-NEXT: NSConsumesSelf (SubjectMatchRule_objc_method)
 // CHECK-NEXT: Naked (SubjectMatchRule_function)
+// CHECK-NEXT: NoBuiltin (SubjectMatchRule_function)
 // CHECK-NEXT: NoCommon (SubjectMatchRule_variable)
 // CHECK-NEXT: NoDebug (SubjectMatchRule_type_alias, SubjectMatchRule_hasType_functionType, SubjectMatchRule_objc_method, SubjectMatchRule_variable_not_is_parameter)
 // CHECK-NEXT: NoDestroy (SubjectMatchRule_variable)
Index: clang/test/CodeGen/no-builtin.cpp
===
--- /dev/null
+++ clang/test/CodeGen/no-builtin.cpp
@@ -0,0 +1,64 @@
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -S -emit-llvm -o - %s | FileCheck %s
+
+// CHECK-LABEL: define void @foo_no_mempcy() #0
+extern "C" void foo_no_mempcy() __attribute__((no_builtin("memcpy"))) {}
+
+// CHECK-LABEL: define void @foo_no_mempcy_twice() #0
+extern "C" void foo_no_mempcy_twice() __attribute__((no_builtin("memcpy"))) __attribute__((no_builtin("memcpy"))) {}
+
+// CHECK-LABEL: define void @foo_no_builtins() #1
+extern "C" void foo_no_builtins() __attribute__((no_builtin)) {}
+
+// CHECK-LABEL: define void @foo_no_mempcy_memset() #2
+extern "C" void foo_no_mempcy_memset() __attribute__((no_builtin("memset", "memcpy"))) {}
+
+// 

[PATCH] D68028: [clang] Add no_builtin attribute

[Guillaume Chatelet via Phabricator via cfe-commits]

gchatelet marked 4 inline comments as done.
gchatelet added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:3609
+  "'%0' is not a valid builtin name for %1">,
+  InGroup; // Which group should it be in?
+def err_attribute_no_builtin_wildcard_or_builtin_name : Error<

aaron.ballman wrote:
> Hmm, I thought there was a way you could do:
> ```
> InGroup>;
> ```
> but it seems I've got the order reversed and you'd have to add a new 
> diagnostic group that `IgnoredAttributes` could subsume. We don't do that for 
> other attributes, so I think you should just do:
> ```
> InGroup>;
> ```
> (Feel free to pick a better diagnostic group name if you have a better idea, 
> I'm not tied to my suggestion.)
`"invalid-no-builtin-names"` LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68028



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68028: [clang] Add no_builtin attribute

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/include/clang/Basic/AttrDocs.td:4407
+It accepts one or more strings corresponding to the name of the builtin
+(e.g. "memcpy", "memset") or "*" which disables all builtins at once.
+

This mention of `*` is now out of date.



Comment at: clang/include/clang/Basic/AttrDocs.td:4412
+  // The compiler is not allowed to add any builtin to foo's body.
+  void foo(char* data, size_t count) __attribute__((no_builtin("*"))) {
+// The compiler is not allowed to convert the loop into

This one too.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:3609
+  "'%0' is not a valid builtin name for %1">,
+  InGroup; // Which group should it be in?
+def err_attribute_no_builtin_wildcard_or_builtin_name : Error<

Hmm, I thought there was a way you could do:
```
InGroup>;
```
but it seems I've got the order reversed and you'd have to add a new diagnostic 
group that `IgnoredAttributes` could subsume. We don't do that for other 
attributes, so I think you should just do:
```
InGroup>;
```
(Feel free to pick a better diagnostic group name if you have a better idea, 
I'm not tied to my suggestion.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68028



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68028: [clang] Add no_builtin attribute

[Guillaume Chatelet via Phabricator via cfe-commits]

gchatelet updated this revision to Diff 225593.
gchatelet added a comment.

- Call sites to virtual functions are not annotated


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68028

Files:
  clang/include/clang/AST/Decl.h
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/CodeGen/no-builtin.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/Sema/no-builtin.cpp

Index: clang/test/Sema/no-builtin.cpp
===
--- /dev/null
+++ clang/test/Sema/no-builtin.cpp
@@ -0,0 +1,51 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -fsyntax-only -verify %s
+
+/// Prevent use of all builtins.
+void valid_attribute_all_1() __attribute__((no_builtin)) {}
+void valid_attribute_all_2() __attribute__((no_builtin())) {}
+
+/// Prevent use of specific builtins.
+void valid_attribute_function() __attribute__((no_builtin("memcpy"))) {}
+void valid_attribute_functions() __attribute__((no_builtin("memcpy"))) __attribute__((no_builtin("memcmp"))) {}
+
+/// Many times the same builtin is fine.
+void many_attribute_function_1() __attribute__((no_builtin)) __attribute__((no_builtin)) {}
+void many_attribute_function_2() __attribute__((no_builtin("memcpy"))) __attribute__((no_builtin("memcpy"))) {}
+void many_attribute_function_3() __attribute__((no_builtin("memcpy", "memcpy"))) {}
+void many_attribute_function_4() __attribute__((no_builtin("memcpy", "memcpy"))) __attribute__((no_builtin("memcpy"))) {}
+
+/// Invalid builtin name.
+void invalid_builtin() __attribute__((no_builtin("not_a_builtin"))) {}
+// expected-warning@-1 {{'not_a_builtin' is not a valid builtin name for no_builtin}}
+
+/// Can't use bare no_builtin with a named one.
+void wildcard_and_functionname() __attribute__((no_builtin)) __attribute__((no_builtin("memcpy"))) {}
+// expected-error@-1 {{empty no_builtin cannot be composed with named ones}}
+
+/// Can't attach attribute to a variable.
+int __attribute__((no_builtin)) variable;
+// expected-warning@-1 {{'no_builtin' attribute only applies to functions}}
+
+/// Can't attach attribute to a declaration.
+void nobuiltin_on_declaration() __attribute__((no_builtin));
+// expected-error@-1 {{no_builtin attribute is permitted on definitions only}}
+
+struct S {
+  /// Can't attach attribute to a defaulted function,
+  S()
+  __attribute__((no_builtin)) = default;
+  // expected-error@-1 {{no_builtin attribute has no effect on defaulted or deleted functions}}
+
+  /// Can't attach attribute to a deleted function,
+  S(const S &)
+  __attribute__((no_builtin)) = delete;
+  // expected-error@-1 {{no_builtin attribute has no effect on defaulted or deleted functions}}
+
+  void whatever() __attribute__((no_builtin("memcpy")));
+  // expected-error@-1 {{no_builtin attribute is permitted on definitions only}}
+};
+
+/// Can't attach attribute to an aliased function.
+void alised_function() {}
+void aliasing_function() __attribute__((no_builtin)) __attribute__((alias("alised_function")));
+// expected-error@-1 {{no_builtin attribute is permitted on definitions only}}
Index: clang/test/Misc/pragma-attribute-supported-attributes-list.test
===
--- clang/test/Misc/pragma-attribute-supported-attributes-list.test
+++ clang/test/Misc/pragma-attribute-supported-attributes-list.test
@@ -74,6 +74,7 @@
 // CHECK-NEXT: NSConsumed (SubjectMatchRule_variable_is_parameter)
 // CHECK-NEXT: NSConsumesSelf (SubjectMatchRule_objc_method)
 // CHECK-NEXT: Naked (SubjectMatchRule_function)
+// CHECK-NEXT: NoBuiltin (SubjectMatchRule_function)
 // CHECK-NEXT: NoCommon (SubjectMatchRule_variable)
 // CHECK-NEXT: NoDebug (SubjectMatchRule_type_alias, SubjectMatchRule_hasType_functionType, SubjectMatchRule_objc_method, SubjectMatchRule_variable_not_is_parameter)
 // CHECK-NEXT: NoDestroy (SubjectMatchRule_variable)
Index: clang/test/CodeGen/no-builtin.cpp
===
--- /dev/null
+++ clang/test/CodeGen/no-builtin.cpp
@@ -0,0 +1,64 @@
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -S -emit-llvm -o - %s | FileCheck %s
+
+// CHECK-LABEL: define void @foo_no_mempcy() #0
+extern "C" void foo_no_mempcy() __attribute__((no_builtin("memcpy"))) {}
+
+// CHECK-LABEL: define void @foo_no_mempcy_twice() #0
+extern "C" void foo_no_mempcy_twice() __attribute__((no_builtin("memcpy"))) __attribute__((no_builtin("memcpy"))) {}
+
+// CHECK-LABEL: define void @foo_no_builtins() #1
+extern "C" void foo_no_builtins() __attribute__((no_builtin)) {}
+
+// CHECK-LABEL: define void @foo_no_mempcy_memset() #2
+extern "C" void foo_no_mempcy_memset() __attribute__((no_builtin("memset", "memcpy"))) {}
+
+// 

[PATCH] D68028: [clang] Add no_builtin attribute

[Guillaume Chatelet via Phabricator via cfe-commits]

gchatelet updated this revision to Diff 225578.
gchatelet added a comment.

- Reverting whole file formatting


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68028

Files:
  clang/include/clang/AST/Decl.h
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/CodeGen/no-builtin.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/Sema/no-builtin.cpp

Index: clang/test/Sema/no-builtin.cpp
===
--- /dev/null
+++ clang/test/Sema/no-builtin.cpp
@@ -0,0 +1,51 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -fsyntax-only -verify %s
+
+/// Prevent use of all builtins.
+void valid_attribute_all_1() __attribute__((no_builtin)) {}
+void valid_attribute_all_2() __attribute__((no_builtin())) {}
+
+/// Prevent use of specific builtins.
+void valid_attribute_function() __attribute__((no_builtin("memcpy"))) {}
+void valid_attribute_functions() __attribute__((no_builtin("memcpy"))) __attribute__((no_builtin("memcmp"))) {}
+
+/// Many times the same builtin is fine.
+void many_attribute_function_1() __attribute__((no_builtin)) __attribute__((no_builtin)) {}
+void many_attribute_function_2() __attribute__((no_builtin("memcpy"))) __attribute__((no_builtin("memcpy"))) {}
+void many_attribute_function_3() __attribute__((no_builtin("memcpy", "memcpy"))) {}
+void many_attribute_function_4() __attribute__((no_builtin("memcpy", "memcpy"))) __attribute__((no_builtin("memcpy"))) {}
+
+/// Invalid builtin name.
+void invalid_builtin() __attribute__((no_builtin("not_a_builtin"))) {}
+// expected-warning@-1 {{'not_a_builtin' is not a valid builtin name for no_builtin}}
+
+/// Can't use bare no_builtin with a named one.
+void wildcard_and_functionname() __attribute__((no_builtin)) __attribute__((no_builtin("memcpy"))) {}
+// expected-error@-1 {{empty no_builtin cannot be composed with named ones}}
+
+/// Can't attach attribute to a variable.
+int __attribute__((no_builtin)) variable;
+// expected-warning@-1 {{'no_builtin' attribute only applies to functions}}
+
+/// Can't attach attribute to a declaration.
+void nobuiltin_on_declaration() __attribute__((no_builtin));
+// expected-error@-1 {{no_builtin attribute is permitted on definitions only}}
+
+struct S {
+  /// Can't attach attribute to a defaulted function,
+  S()
+  __attribute__((no_builtin)) = default;
+  // expected-error@-1 {{no_builtin attribute has no effect on defaulted or deleted functions}}
+
+  /// Can't attach attribute to a deleted function,
+  S(const S &)
+  __attribute__((no_builtin)) = delete;
+  // expected-error@-1 {{no_builtin attribute has no effect on defaulted or deleted functions}}
+
+  void whatever() __attribute__((no_builtin("memcpy")));
+  // expected-error@-1 {{no_builtin attribute is permitted on definitions only}}
+};
+
+/// Can't attach attribute to an aliased function.
+void alised_function() {}
+void aliasing_function() __attribute__((no_builtin)) __attribute__((alias("alised_function")));
+// expected-error@-1 {{no_builtin attribute is permitted on definitions only}}
Index: clang/test/Misc/pragma-attribute-supported-attributes-list.test
===
--- clang/test/Misc/pragma-attribute-supported-attributes-list.test
+++ clang/test/Misc/pragma-attribute-supported-attributes-list.test
@@ -74,6 +74,7 @@
 // CHECK-NEXT: NSConsumed (SubjectMatchRule_variable_is_parameter)
 // CHECK-NEXT: NSConsumesSelf (SubjectMatchRule_objc_method)
 // CHECK-NEXT: Naked (SubjectMatchRule_function)
+// CHECK-NEXT: NoBuiltin (SubjectMatchRule_function)
 // CHECK-NEXT: NoCommon (SubjectMatchRule_variable)
 // CHECK-NEXT: NoDebug (SubjectMatchRule_type_alias, SubjectMatchRule_hasType_functionType, SubjectMatchRule_objc_method, SubjectMatchRule_variable_not_is_parameter)
 // CHECK-NEXT: NoDestroy (SubjectMatchRule_variable)
Index: clang/test/CodeGen/no-builtin.cpp
===
--- /dev/null
+++ clang/test/CodeGen/no-builtin.cpp
@@ -0,0 +1,59 @@
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -S -emit-llvm -o - %s | FileCheck %s
+
+// CHECK-LABEL: define void @foo_no_mempcy() #0
+extern "C" void foo_no_mempcy() __attribute__((no_builtin("memcpy"))) {}
+
+// CHECK-LABEL: define void @foo_no_mempcy_twice() #0
+extern "C" void foo_no_mempcy_twice() __attribute__((no_builtin("memcpy"))) __attribute__((no_builtin("memcpy"))) {}
+
+// CHECK-LABEL: define void @foo_no_builtins() #1
+extern "C" void foo_no_builtins() __attribute__((no_builtin)) {}
+
+// CHECK-LABEL: define void @foo_no_mempcy_memset() #2
+extern "C" void foo_no_mempcy_memset() __attribute__((no_builtin("memset", "memcpy"))) {}
+
+// CHECK-LABEL: 

[PATCH] D68028: [clang] Add no_builtin attribute

[Guillaume Chatelet via Phabricator via cfe-commits]

gchatelet updated this revision to Diff 225576.
gchatelet marked an inline comment as done.
gchatelet added a comment.

- Improve tests


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68028

Files:
  clang/include/clang/AST/Decl.h
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/CodeGen/no-builtin.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/Sema/no-builtin.cpp

Index: clang/test/Sema/no-builtin.cpp
===
--- /dev/null
+++ clang/test/Sema/no-builtin.cpp
@@ -0,0 +1,51 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -fsyntax-only -verify %s
+
+/// Prevent use of all builtins.
+void valid_attribute_all_1() __attribute__((no_builtin)) {}
+void valid_attribute_all_2() __attribute__((no_builtin())) {}
+
+/// Prevent use of specific builtins.
+void valid_attribute_function() __attribute__((no_builtin("memcpy"))) {}
+void valid_attribute_functions() __attribute__((no_builtin("memcpy"))) __attribute__((no_builtin("memcmp"))) {}
+
+/// Many times the same builtin is fine.
+void many_attribute_function_1() __attribute__((no_builtin)) __attribute__((no_builtin)) {}
+void many_attribute_function_2() __attribute__((no_builtin("memcpy"))) __attribute__((no_builtin("memcpy"))) {}
+void many_attribute_function_3() __attribute__((no_builtin("memcpy", "memcpy"))) {}
+void many_attribute_function_4() __attribute__((no_builtin("memcpy", "memcpy"))) __attribute__((no_builtin("memcpy"))) {}
+
+/// Invalid builtin name.
+void invalid_builtin() __attribute__((no_builtin("not_a_builtin"))) {}
+// expected-warning@-1 {{'not_a_builtin' is not a valid builtin name for no_builtin}}
+
+/// Can't use bare no_builtin with a named one.
+void wildcard_and_functionname() __attribute__((no_builtin)) __attribute__((no_builtin("memcpy"))) {}
+// expected-error@-1 {{empty no_builtin cannot be composed with named ones}}
+
+/// Can't attach attribute to a variable.
+int __attribute__((no_builtin)) variable;
+// expected-warning@-1 {{'no_builtin' attribute only applies to functions}}
+
+/// Can't attach attribute to a declaration.
+void nobuiltin_on_declaration() __attribute__((no_builtin));
+// expected-error@-1 {{no_builtin attribute is permitted on definitions only}}
+
+struct S {
+  /// Can't attach attribute to a defaulted function,
+  S()
+  __attribute__((no_builtin)) = default;
+  // expected-error@-1 {{no_builtin attribute has no effect on defaulted or deleted functions}}
+
+  /// Can't attach attribute to a deleted function,
+  S(const S &)
+  __attribute__((no_builtin)) = delete;
+  // expected-error@-1 {{no_builtin attribute has no effect on defaulted or deleted functions}}
+
+  void whatever() __attribute__((no_builtin("memcpy")));
+  // expected-error@-1 {{no_builtin attribute is permitted on definitions only}}
+};
+
+/// Can't attach attribute to an aliased function.
+void alised_function() {}
+void aliasing_function() __attribute__((no_builtin)) __attribute__((alias("alised_function")));
+// expected-error@-1 {{no_builtin attribute is permitted on definitions only}}
Index: clang/test/Misc/pragma-attribute-supported-attributes-list.test
===
--- clang/test/Misc/pragma-attribute-supported-attributes-list.test
+++ clang/test/Misc/pragma-attribute-supported-attributes-list.test
@@ -74,6 +74,7 @@
 // CHECK-NEXT: NSConsumed (SubjectMatchRule_variable_is_parameter)
 // CHECK-NEXT: NSConsumesSelf (SubjectMatchRule_objc_method)
 // CHECK-NEXT: Naked (SubjectMatchRule_function)
+// CHECK-NEXT: NoBuiltin (SubjectMatchRule_function)
 // CHECK-NEXT: NoCommon (SubjectMatchRule_variable)
 // CHECK-NEXT: NoDebug (SubjectMatchRule_type_alias, SubjectMatchRule_hasType_functionType, SubjectMatchRule_objc_method, SubjectMatchRule_variable_not_is_parameter)
 // CHECK-NEXT: NoDestroy (SubjectMatchRule_variable)
Index: clang/test/CodeGen/no-builtin.cpp
===
--- /dev/null
+++ clang/test/CodeGen/no-builtin.cpp
@@ -0,0 +1,59 @@
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -S -emit-llvm -o - %s | FileCheck %s
+
+// CHECK-LABEL: define void @foo_no_mempcy() #0
+extern "C" void foo_no_mempcy() __attribute__((no_builtin("memcpy"))) {}
+
+// CHECK-LABEL: define void @foo_no_mempcy_twice() #0
+extern "C" void foo_no_mempcy_twice() __attribute__((no_builtin("memcpy"))) __attribute__((no_builtin("memcpy"))) {}
+
+// CHECK-LABEL: define void @foo_no_builtins() #1
+extern "C" void foo_no_builtins() __attribute__((no_builtin)) {}
+
+// CHECK-LABEL: define void @foo_no_mempcy_memset() #2
+extern "C" void foo_no_mempcy_memset() __attribute__((no_builtin("memset", "memcpy"))) 

[PATCH] D68028: [clang] Add no_builtin attribute

[Guillaume Chatelet via Phabricator via cfe-commits]

gchatelet updated this revision to Diff 225575.
gchatelet marked 3 inline comments as done.
gchatelet added a comment.

- Change NoBuiltin from InheritableAttr to Attr
- Improve tests


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68028

Files:
  clang/include/clang/AST/Decl.h
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/CodeGen/no-builtin.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/Sema/no-builtin.cpp

Index: clang/test/Sema/no-builtin.cpp
===
--- /dev/null
+++ clang/test/Sema/no-builtin.cpp
@@ -0,0 +1,48 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -fsyntax-only -verify %s
+
+/// Prevent use of all builtins.
+void valid_attribute_all_1() __attribute__((no_builtin)) {}
+void valid_attribute_all_2() __attribute__((no_builtin())) {}
+
+/// Prevent use of specific builtins.
+void valid_attribute_function() __attribute__((no_builtin("memcpy"))) {}
+void valid_attribute_functions() __attribute__((no_builtin("memcpy"))) __attribute__((no_builtin("memcmp"))) {}
+
+/// Many times the same builtin is fine.
+void many_attribute_function_1() __attribute__((no_builtin)) __attribute__((no_builtin)) {}
+void many_attribute_function_2() __attribute__((no_builtin("memcpy"))) __attribute__((no_builtin("memcpy"))) {}
+void many_attribute_function_3() __attribute__((no_builtin("memcpy", "memcpy"))) {}
+void many_attribute_function_4() __attribute__((no_builtin("memcpy", "memcpy"))) __attribute__((no_builtin("memcpy"))) {}
+
+/// Invalid builtin name.
+void invalid_builtin() __attribute__((no_builtin("not_a_builtin"))) {}
+// expected-warning@-1 {{'not_a_builtin' is not a valid builtin name for no_builtin}}
+
+/// Can't use bare no_builtin with a named one.
+void wildcard_and_functionname() __attribute__((no_builtin)) __attribute__((no_builtin("memcpy"))) {}
+// expected-error@-1 {{empty no_builtin cannot be composed with named ones}}
+
+/// Can't attach attribute to a variable.
+int __attribute__((no_builtin)) variable;
+// expected-warning@-1 {{'no_builtin' attribute only applies to functions}}
+
+/// Can't attach attribute to a declaration.
+void nobuiltin_on_declaration() __attribute__((no_builtin));
+// expected-error@-1 {{no_builtin attribute is permitted on definitions only}}
+
+struct S {
+  /// Can't attach attribute to a defaulted function,
+  S()
+  __attribute__((no_builtin)) = default;
+  // expected-error@-1 {{no_builtin attribute has no effect on defaulted or deleted functions}}
+
+  /// Can't attach attribute to a deleted function,
+  S(const S &)
+  __attribute__((no_builtin)) = delete;
+  // expected-error@-1 {{no_builtin attribute has no effect on defaulted or deleted functions}}
+};
+
+/// Can't attach attribute to an aliased function.
+void alised_function() {}
+void aliasing_function() __attribute__((no_builtin)) __attribute__((alias("alised_function")));
+// expected-error@-1 {{no_builtin attribute is permitted on definitions only}}
\ No newline at end of file
Index: clang/test/Misc/pragma-attribute-supported-attributes-list.test
===
--- clang/test/Misc/pragma-attribute-supported-attributes-list.test
+++ clang/test/Misc/pragma-attribute-supported-attributes-list.test
@@ -74,6 +74,7 @@
 // CHECK-NEXT: NSConsumed (SubjectMatchRule_variable_is_parameter)
 // CHECK-NEXT: NSConsumesSelf (SubjectMatchRule_objc_method)
 // CHECK-NEXT: Naked (SubjectMatchRule_function)
+// CHECK-NEXT: NoBuiltin (SubjectMatchRule_function)
 // CHECK-NEXT: NoCommon (SubjectMatchRule_variable)
 // CHECK-NEXT: NoDebug (SubjectMatchRule_type_alias, SubjectMatchRule_hasType_functionType, SubjectMatchRule_objc_method, SubjectMatchRule_variable_not_is_parameter)
 // CHECK-NEXT: NoDestroy (SubjectMatchRule_variable)
Index: clang/test/CodeGen/no-builtin.cpp
===
--- /dev/null
+++ clang/test/CodeGen/no-builtin.cpp
@@ -0,0 +1,54 @@
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -S -emit-llvm -o - %s | FileCheck %s
+
+// CHECK-LABEL: define void @foo_no_mempcy() #0
+extern "C" void foo_no_mempcy() __attribute__((no_builtin("memcpy"))) {}
+
+// CHECK-LABEL: define void @foo_no_mempcy_twice() #0
+extern "C" void foo_no_mempcy_twice() __attribute__((no_builtin("memcpy"))) __attribute__((no_builtin("memcpy"))) {}
+
+// CHECK-LABEL: define void @foo_no_builtins() #1
+extern "C" void foo_no_builtins() __attribute__((no_builtin)) {}
+
+// CHECK-LABEL: define void @foo_no_mempcy_memset() #2
+extern "C" void foo_no_mempcy_memset() __attribute__((no_builtin("memset", "memcpy"))) {}
+
+// CHECK-LABEL: define void @separate_attrs() #2
+extern "C" 

[PATCH] D68028: [clang] Add no_builtin attribute

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/include/clang/Basic/Attr.td:3416
+
+def NoBuiltin : InheritableAttr {
+  let Spellings = [Clang<"no_builtin">];

I think we do not want this to be an inheritable attribute, but just `Attr` 
instead, because this can only be written on definitions (so there's no way to 
inherit the attribute from previous declarations).



Comment at: clang/test/Sema/no-builtin.c:26
+int __attribute__((no_builtin("*"))) variable;
+// expected-warning@-1 {{'no_builtin' attribute only applies to functions}}

gchatelet wrote:
> aaron.ballman wrote:
> > 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"))) {}
> > };
> > ```
> ```
> void whatever() __attribute__((no_builtin("*", "*"))) {}
> ```
> 
> I've added a few ones.
> 
> ```
> struct S {
>   void whatever() __attribute__((no_builtin("memcpy"))) {}
>   virtual void intentional() __attribute__((no_builtin("memcpy"))) {}
> };
> ```
> 
> What do you want me to test for this one?
> What do you want me to test for this one?

Ensuring that applying the attribute to member function definitions, including 
virtual ones, is supported and doesn't cause diagnostics. The codegen side of 
things will test that overridden virtual methods do not inherit the attribute.

Actually, there's another interesting test hiding in there for when we do want 
a diagnostic (I think):
```
struct S {
  void whatever() __attribute__((no_builtin("memcpy"))); // Should diagnose as 
not a definition
};
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68028



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68028: [clang] Add no_builtin attribute

[Guillaume Chatelet via Phabricator via cfe-commits]

gchatelet updated this revision to Diff 225435.
gchatelet marked 13 inline comments as done.
gchatelet added a comment.
Herald added subscribers: s.egerton, simoncook, aheejin, dschuff.

- Address comments
- Fix missing rename
- Address comments
- Add warning category


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68028

Files:
  clang/include/clang/AST/Decl.h
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/CodeGen/no-builtin.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/Sema/no-builtin.cpp

Index: clang/test/Sema/no-builtin.cpp
===
--- /dev/null
+++ clang/test/Sema/no-builtin.cpp
@@ -0,0 +1,48 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -fsyntax-only -verify %s
+
+/// Prevent use of all builtins.
+void valid_attribute_all_1() __attribute__((no_builtin)) {}
+void valid_attribute_all_2() __attribute__((no_builtin())) {}
+
+/// Prevent use of specific builtins.
+void valid_attribute_function() __attribute__((no_builtin("memcpy"))) {}
+void valid_attribute_functions() __attribute__((no_builtin("memcpy"))) __attribute__((no_builtin("memcmp"))) {}
+
+/// Many times the same builtin is fine.
+void many_attribute_function_1() __attribute__((no_builtin)) __attribute__((no_builtin)) {}
+void many_attribute_function_2() __attribute__((no_builtin("memcpy"))) __attribute__((no_builtin("memcpy"))) {}
+void many_attribute_function_3() __attribute__((no_builtin("memcpy", "memcpy"))) {}
+void many_attribute_function_4() __attribute__((no_builtin("memcpy", "memcpy"))) __attribute__((no_builtin("memcpy"))) {}
+
+/// Invalid builtin name.
+void invalid_builtin() __attribute__((no_builtin("not_a_builtin"))) {}
+// expected-warning@-1 {{'not_a_builtin' is not a valid builtin name for no_builtin}}
+
+/// Can't use bare no_builtin with a named one.
+void wildcard_and_functionname() __attribute__((no_builtin)) __attribute__((no_builtin("memcpy"))) {}
+// expected-error@-1 {{empty no_builtin cannot be composed with named ones}}
+
+/// Can't attach attribute to a variable.
+int __attribute__((no_builtin)) variable;
+// expected-warning@-1 {{'no_builtin' attribute only applies to functions}}
+
+/// Can't attach attribute to a declaration.
+void nobuiltin_on_declaration() __attribute__((no_builtin));
+// expected-error@-1 {{no_builtin attribute is permitted on definitions only}}
+
+struct S {
+  /// Can't attach attribute to a defaulted function,
+  S()
+  __attribute__((no_builtin)) = default;
+  // expected-error@-1 {{no_builtin attribute has no effect on defaulted or deleted functions}}
+
+  /// Can't attach attribute to a deleted function,
+  S(const S &)
+  __attribute__((no_builtin)) = delete;
+  // expected-error@-1 {{no_builtin attribute has no effect on defaulted or deleted functions}}
+};
+
+/// Can't attach attribute to an aliased function.
+void alised_function() {}
+void aliasing_function() __attribute__((no_builtin)) __attribute__((alias("alised_function")));
+// expected-error@-1 {{no_builtin attribute is permitted on definitions only}}
\ No newline at end of file
Index: clang/test/Misc/pragma-attribute-supported-attributes-list.test
===
--- clang/test/Misc/pragma-attribute-supported-attributes-list.test
+++ clang/test/Misc/pragma-attribute-supported-attributes-list.test
@@ -74,6 +74,7 @@
 // CHECK-NEXT: NSConsumed (SubjectMatchRule_variable_is_parameter)
 // CHECK-NEXT: NSConsumesSelf (SubjectMatchRule_objc_method)
 // CHECK-NEXT: Naked (SubjectMatchRule_function)
+// CHECK-NEXT: NoBuiltin (SubjectMatchRule_function)
 // CHECK-NEXT: NoCommon (SubjectMatchRule_variable)
 // CHECK-NEXT: NoDebug (SubjectMatchRule_type_alias, SubjectMatchRule_hasType_functionType, SubjectMatchRule_objc_method, SubjectMatchRule_variable_not_is_parameter)
 // CHECK-NEXT: NoDestroy (SubjectMatchRule_variable)
Index: clang/test/CodeGen/no-builtin.cpp
===
--- /dev/null
+++ clang/test/CodeGen/no-builtin.cpp
@@ -0,0 +1,45 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -S -emit-llvm -o - %s | FileCheck %s
+
+// CHECK-LABEL: define void @foo_no_mempcy() #0
+extern "C" void foo_no_mempcy() __attribute__((no_builtin("memcpy"))) {}
+
+// CHECK-LABEL: define void @foo_no_mempcy_twice() #0
+extern "C" void foo_no_mempcy_twice() __attribute__((no_builtin("memcpy"))) __attribute__((no_builtin("memcpy"))) {}
+
+// CHECK-LABEL: define void @foo_no_builtins() #1
+extern "C" void foo_no_builtins() __attribute__((no_builtin)) {}
+
+// CHECK-LABEL: define void @foo_no_mempcy_memset() #2
+extern "C" void foo_no_mempcy_memset() 

[PATCH] D68028: [clang] Add no_builtin attribute

[Guillaume Chatelet via Phabricator via cfe-commits]

gchatelet added inline comments.



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">;

aaron.ballman wrote:
> 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.
Makes perfect sense, thx for pointing this out.



Comment at: clang/lib/Sema/SemaDecl.cpp:9508-9510
+  // FIXME: We should really be doing this in SemaDeclAttr.cpp::handleNoBuiltin
+  // but there is a bug with FunctionDecl::isThisDeclarationADefinition() which
+  // always returns false before Sema::ActOnStartOfFunctionDef is called.

rsmith wrote:
> aaron.ballman wrote:
> > rsmith wrote:
> > > aaron.ballman wrote:
> > > > handleNoBuiltin -> handleNoBuiltinAttr
> > > I am not convinced that this is a bug -- the function declaration does 
> > > not become a definition until the parser reaches the definition.
> > > 
> > > In any case, I don't think the predicate you want is "is this declaration 
> > > a definition?". Rather, I think what you want is, "does this declaration 
> > > introduce an explicit function body?". In particular, we should not 
> > > permit uses of this attribute on defaulted or deleted functions, nor on 
> > > functions that have a definition by virtue of using 
> > > `__attribute__((alias))`. So it probably should be a syntactic check on 
> > > the form of the declarator (that is, the check you're perrforming here), 
> > > and the check should probably be `D.getFunctionDefinitionKind() == 
> > > FDK_Definition`. (A custom diagnostic for using the attribute on a 
> > > defaulted or deleted function would be nice too, since the existing 
> > > diagnostic text isn't really accurate in those cases.)
> > > In particular, we should not permit uses of this attribute on defaulted 
> > > or deleted functions
> > 
> > Deleted functions, sure. Defaulted functions... not so sure. I could sort 
> > of imagine wanting to instruct a defaulted assignment operator that does 
> > memberwise copy that it's not allowed to use a builtin memcpy, for 
> > instance. Or is this a bad idea for some reason I'm not thinking of?
> `-fno-builtin` does not turn off using `llvm.memcpy` for copying memory, and 
> it doesn't turn off `llvm.memcpy` being lowered to a call to `memcpy`. 
> Allowing this for defaulted functions would only give a false sense of 
> security, at least for now (though I could imagine we might find some way to 
> change that in the future).
> 
> Also, trivial defaulted functions (where we're most likely to end up with 
> `memcpy` calls) are often not emitted at all, instead being directly inlined 
> by the frontend, so there's nowhere to attach a `no-builtin-memcpy` LLVM 
> attribute (we'd need to put the attribute on all callers of those functions) 
> even if LLVM learned to not emit calls to `memcpy` to implement `llvm.memcpy` 
> when operating under a `no-builtin-memcpy` constraint.
> I am not convinced that this is a bug -- the function declaration does not 
> become a definition until the parser reaches the definition.

@rsmith It may not be a bug but it is currently used in a buggy manner (See 
D68379).
An assert to prevent misuse would be welcome IMHO, I've added a note on the 
function meanwhile.



Comment at: clang/test/Sema/no-builtin.c:26
+int __attribute__((no_builtin("*"))) variable;
+// expected-warning@-1 {{'no_builtin' attribute only applies to functions}}

aaron.ballman wrote:
> 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"))) {}
> };
> ```
```
void whatever() __attribute__((no_builtin("*", "*"))) {}
```

I've added a few ones.

```
struct S {
  void whatever() __attribute__((no_builtin("memcpy"))) {}
  virtual void intentional() __attribute__((no_builtin("memcpy"))) {}
};
```

What do you want me to test for this one?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68028



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68028: [clang] Add no_builtin attribute

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/lib/Sema/SemaDecl.cpp:9508-9510
+  // FIXME: We should really be doing this in SemaDeclAttr.cpp::handleNoBuiltin
+  // but there is a bug with FunctionDecl::isThisDeclarationADefinition() which
+  // always returns false before Sema::ActOnStartOfFunctionDef is called.

rsmith wrote:
> aaron.ballman wrote:
> > rsmith wrote:
> > > aaron.ballman wrote:
> > > > handleNoBuiltin -> handleNoBuiltinAttr
> > > I am not convinced that this is a bug -- the function declaration does 
> > > not become a definition until the parser reaches the definition.
> > > 
> > > In any case, I don't think the predicate you want is "is this declaration 
> > > a definition?". Rather, I think what you want is, "does this declaration 
> > > introduce an explicit function body?". In particular, we should not 
> > > permit uses of this attribute on defaulted or deleted functions, nor on 
> > > functions that have a definition by virtue of using 
> > > `__attribute__((alias))`. So it probably should be a syntactic check on 
> > > the form of the declarator (that is, the check you're perrforming here), 
> > > and the check should probably be `D.getFunctionDefinitionKind() == 
> > > FDK_Definition`. (A custom diagnostic for using the attribute on a 
> > > defaulted or deleted function would be nice too, since the existing 
> > > diagnostic text isn't really accurate in those cases.)
> > > In particular, we should not permit uses of this attribute on defaulted 
> > > or deleted functions
> > 
> > Deleted functions, sure. Defaulted functions... not so sure. I could sort 
> > of imagine wanting to instruct a defaulted assignment operator that does 
> > memberwise copy that it's not allowed to use a builtin memcpy, for 
> > instance. Or is this a bad idea for some reason I'm not thinking of?
> `-fno-builtin` does not turn off using `llvm.memcpy` for copying memory, and 
> it doesn't turn off `llvm.memcpy` being lowered to a call to `memcpy`. 
> Allowing this for defaulted functions would only give a false sense of 
> security, at least for now (though I could imagine we might find some way to 
> change that in the future).
> 
> Also, trivial defaulted functions (where we're most likely to end up with 
> `memcpy` calls) are often not emitted at all, instead being directly inlined 
> by the frontend, so there's nowhere to attach a `no-builtin-memcpy` LLVM 
> attribute (we'd need to put the attribute on all callers of those functions) 
> even if LLVM learned to not emit calls to `memcpy` to implement `llvm.memcpy` 
> when operating under a `no-builtin-memcpy` constraint.
Ah, good to know! We're in agreement on how this should proceed, thank you for 
the insights.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68028



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68028: [clang] Add no_builtin attribute

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: clang/lib/Sema/SemaDecl.cpp:9508-9510
+  // FIXME: We should really be doing this in SemaDeclAttr.cpp::handleNoBuiltin
+  // but there is a bug with FunctionDecl::isThisDeclarationADefinition() which
+  // always returns false before Sema::ActOnStartOfFunctionDef is called.

aaron.ballman wrote:
> rsmith wrote:
> > aaron.ballman wrote:
> > > handleNoBuiltin -> handleNoBuiltinAttr
> > I am not convinced that this is a bug -- the function declaration does not 
> > become a definition until the parser reaches the definition.
> > 
> > In any case, I don't think the predicate you want is "is this declaration a 
> > definition?". Rather, I think what you want is, "does this declaration 
> > introduce an explicit function body?". In particular, we should not permit 
> > uses of this attribute on defaulted or deleted functions, nor on functions 
> > that have a definition by virtue of using `__attribute__((alias))`. So it 
> > probably should be a syntactic check on the form of the declarator (that 
> > is, the check you're perrforming here), and the check should probably be 
> > `D.getFunctionDefinitionKind() == FDK_Definition`. (A custom diagnostic for 
> > using the attribute on a defaulted or deleted function would be nice too, 
> > since the existing diagnostic text isn't really accurate in those cases.)
> > In particular, we should not permit uses of this attribute on defaulted or 
> > deleted functions
> 
> Deleted functions, sure. Defaulted functions... not so sure. I could sort of 
> imagine wanting to instruct a defaulted assignment operator that does 
> memberwise copy that it's not allowed to use a builtin memcpy, for instance. 
> Or is this a bad idea for some reason I'm not thinking of?
`-fno-builtin` does not turn off using `llvm.memcpy` for copying memory, and it 
doesn't turn off `llvm.memcpy` being lowered to a call to `memcpy`. Allowing 
this for defaulted functions would only give a false sense of security, at 
least for now (though I could imagine we might find some way to change that in 
the future).

Also, trivial defaulted functions (where we're most likely to end up with 
`memcpy` calls) are often not emitted at all, instead being directly inlined by 
the frontend, so there's nowhere to attach a `no-builtin-memcpy` LLVM attribute 
(we'd need to put the attribute on all callers of those functions) even if LLVM 
learned to not emit calls to `memcpy` to implement `llvm.memcpy` when operating 
under a `no-builtin-memcpy` constraint.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68028



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68028: [clang] Add no_builtin attribute

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/lib/Sema/SemaDecl.cpp:9508-9510
+  // FIXME: We should really be doing this in SemaDeclAttr.cpp::handleNoBuiltin
+  // but there is a bug with FunctionDecl::isThisDeclarationADefinition() which
+  // always returns false before Sema::ActOnStartOfFunctionDef is called.

rsmith wrote:
> aaron.ballman wrote:
> > handleNoBuiltin -> handleNoBuiltinAttr
> I am not convinced that this is a bug -- the function declaration does not 
> become a definition until the parser reaches the definition.
> 
> In any case, I don't think the predicate you want is "is this declaration a 
> definition?". Rather, I think what you want is, "does this declaration 
> introduce an explicit function body?". In particular, we should not permit 
> uses of this attribute on defaulted or deleted functions, nor on functions 
> that have a definition by virtue of using `__attribute__((alias))`. So it 
> probably should be a syntactic check on the form of the declarator (that is, 
> the check you're perrforming here), and the check should probably be 
> `D.getFunctionDefinitionKind() == FDK_Definition`. (A custom diagnostic for 
> using the attribute on a defaulted or deleted function would be nice too, 
> since the existing diagnostic text isn't really accurate in those cases.)
> In particular, we should not permit uses of this attribute on defaulted or 
> deleted functions

Deleted functions, sure. Defaulted functions... not so sure. I could sort of 
imagine wanting to instruct a defaulted assignment operator that does 
memberwise copy that it's not allowed to use a builtin memcpy, for instance. Or 
is this a bad idea for some reason I'm not thinking of?



Comment at: clang/test/Sema/no-builtin.c:7
+
+void no_builtin_no_argument() __attribute__((no_builtin)) {}
+// expected-error@-1 {{'no_builtin' attribute takes at least 1 argument}}

rsmith wrote:
> I find this surprising. I would expect this to work, and to mean the same as 
> `-fno-builtin` for the function (that is, same as the `*` form).
> 
> I think might be a better design to remove the magical `"*"` form entirely, 
> and use an empty argument list to mean "all builtins". Have you considered 
> that? (That would better mirror how `-fno-builtin` vs `-fno-builtin-foo` 
> works.)
> I think might be a better design to remove the magical "*" form entirely, and 
> use an empty argument list to mean "all builtins".

I like this approach.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68028



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68028: [clang] Add no_builtin attribute

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: clang/lib/Sema/SemaDecl.cpp:9508-9510
+  // FIXME: We should really be doing this in SemaDeclAttr.cpp::handleNoBuiltin
+  // but there is a bug with FunctionDecl::isThisDeclarationADefinition() which
+  // always returns false before Sema::ActOnStartOfFunctionDef is called.

aaron.ballman wrote:
> handleNoBuiltin -> handleNoBuiltinAttr
I am not convinced that this is a bug -- the function declaration does not 
become a definition until the parser reaches the definition.

In any case, I don't think the predicate you want is "is this declaration a 
definition?". Rather, I think what you want is, "does this declaration 
introduce an explicit function body?". In particular, we should not permit uses 
of this attribute on defaulted or deleted functions, nor on functions that have 
a definition by virtue of using `__attribute__((alias))`. So it probably should 
be a syntactic check on the form of the declarator (that is, the check you're 
perrforming here), and the check should probably be 
`D.getFunctionDefinitionKind() == FDK_Definition`. (A custom diagnostic for 
using the attribute on a defaulted or deleted function would be nice too, since 
the existing diagnostic text isn't really accurate in those cases.)



Comment at: clang/test/Sema/no-builtin.c:7
+
+void no_builtin_no_argument() __attribute__((no_builtin)) {}
+// expected-error@-1 {{'no_builtin' attribute takes at least 1 argument}}

I find this surprising. I would expect this to work, and to mean the same as 
`-fno-builtin` for the function (that is, same as the `*` form).

I think might be a better design to remove the magical `"*"` form entirely, and 
use an empty argument list to mean "all builtins". Have you considered that? 
(That would better mirror how `-fno-builtin` vs `-fno-builtin-foo` works.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68028



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68028: [clang] Add no_builtin attribute

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In 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 ,
+ llvm::ArrayRef 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 , Decl *D, const ParsedAttr ) {
+  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@-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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68028: [clang] Add no_builtin attribute

[Guillaume Chatelet via Phabricator via cfe-commits]

gchatelet added a comment.

A gentle ping @aaron.ballman


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68028



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68028: [clang] Add no_builtin attribute

[Guillaume Chatelet via Phabricator via cfe-commits]

gchatelet updated this revision to Diff 224286.
gchatelet added a comment.

- Fix missing rename


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68028

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/Sema.h
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/CodeGen/no-builtin.c
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/Sema/no-builtin.c

Index: clang/test/Sema/no-builtin.c
===
--- /dev/null
+++ clang/test/Sema/no-builtin.c
@@ -0,0 +1,26 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -S -emit-llvm -o - -fsyntax-only -verify %s
+
+void valid_attribute_wildcard() __attribute__((no_builtin("*"))) {}
+void valid_attribute_function() __attribute__((no_builtin("memcpy"))) {}
+void valid_attribute_functions() __attribute__((no_builtin("memcpy"))) __attribute__((no_builtin("memcmp"))) {}
+
+void no_builtin_no_argument() __attribute__((no_builtin)) {}
+// expected-error@-1 {{'no_builtin' attribute takes at least 1 argument}}
+
+void no_builtin_no_argument2() __attribute__((no_builtin())) {}
+// expected-error@-1 {{'no_builtin' attribute takes at least 1 argument}}
+
+void invalid_builtin() __attribute__((no_builtin("not_a_builtin"))) {}
+// expected-error@-1 {{'not_a_builtin' is not a valid builtin name for no_builtin}}
+
+void wildcard_and_functionname() __attribute__((no_builtin("*", "memcpy"))) {}
+// expected-error@-1 {{no_builtin wildcard (*) cannot be composed with other builtin names}}
+
+void wildcard_and_functionname2() __attribute__((no_builtin("*"))) __attribute__((no_builtin("memcpy"))) {}
+// expected-error@-1 {{no_builtin wildcard (*) cannot be composed with other builtin names}}
+
+void nobuiltin_on_declaration() __attribute__((no_builtin("memcpy")));
+// expected-error@-1 {{no_builtin attribute is permitted on definitions only}}
+
+int __attribute__((no_builtin("*"))) variable;
+// expected-warning@-1 {{'no_builtin' attribute only applies to functions}}
Index: clang/test/Misc/pragma-attribute-supported-attributes-list.test
===
--- clang/test/Misc/pragma-attribute-supported-attributes-list.test
+++ clang/test/Misc/pragma-attribute-supported-attributes-list.test
@@ -74,6 +74,7 @@
 // CHECK-NEXT: NSConsumed (SubjectMatchRule_variable_is_parameter)
 // CHECK-NEXT: NSConsumesSelf (SubjectMatchRule_objc_method)
 // CHECK-NEXT: Naked (SubjectMatchRule_function)
+// CHECK-NEXT: NoBuiltin (SubjectMatchRule_function)
 // CHECK-NEXT: NoCommon (SubjectMatchRule_variable)
 // CHECK-NEXT: NoDebug (SubjectMatchRule_type_alias, SubjectMatchRule_hasType_functionType, SubjectMatchRule_objc_method, SubjectMatchRule_variable_not_is_parameter)
 // CHECK-NEXT: NoDestroy (SubjectMatchRule_variable)
Index: clang/test/CodeGen/no-builtin.c
===
--- /dev/null
+++ clang/test/CodeGen/no-builtin.c
@@ -0,0 +1,23 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -S -emit-llvm -o - %s | FileCheck %s
+
+// CHECK-LABEL: define void @foo_no_mempcy() #0
+void foo_no_mempcy() __attribute__((no_builtin("memcpy"))) {}
+
+// CHECK-LABEL: define void @foo_no_mempcy_twice() #0
+void foo_no_mempcy_twice() __attribute__((no_builtin("memcpy"))) __attribute__((no_builtin("memcpy"))) {}
+
+// CHECK-LABEL: define void @foo_no_builtins() #1
+void foo_no_builtins() __attribute__((no_builtin("*"))) {}
+
+// CHECK-LABEL: define void @foo_no_mempcy_memset() #2
+void foo_no_mempcy_memset() __attribute__((no_builtin("memset", "memcpy"))) {}
+
+// CHECK-LABEL: define void @separate_attrs() #2
+void separate_attrs() __attribute__((no_builtin("memset"))) __attribute__((no_builtin("memcpy"))) {}
+
+// CHECK-LABEL: define void @separate_attrs_ordering() #2
+void separate_attrs_ordering() __attribute__((no_builtin("memcpy"))) __attribute__((no_builtin("memset"))) {}
+
+// CHECK: attributes #0 = {{{.*}}"no-builtin-memcpy"{{.*}}}
+// CHECK: attributes #1 = {{{.*}}"no-builtins"{{.*}}}
+// CHECK: attributes #2 = {{{.*}}"no-builtin-memcpy"{{.*}}"no-builtin-memset"{{.*}}}
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -1068,6 +1068,60 @@
   S.Context, AL, Cond, Msg, DiagType, ArgDependent, cast(D)));
 }
 
+NoBuiltinAttr *
+Sema::mergeNoBuiltinAttr(Decl *D, const AttributeCommonInfo ,
+ llvm::ArrayRef BuiltinNames) {
+  llvm::SmallVector Names;
+  bool HasWildcard = false;
+
+  const auto TestAndPushBack = [, ](StringRef Name) {
+HasWildcard |= (Name == "*");
+Names.push_back(Name);
+  };
+
+  if (const auto *NBA = 

[PATCH] D68028: [clang] Add no_builtin attribute

[Guillaume Chatelet via Phabricator via cfe-commits]

gchatelet updated this revision to Diff 224285.
gchatelet marked 2 inline comments as done.
gchatelet added a comment.

- Address comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68028

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/Sema.h
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/CodeGen/no-builtin.c
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/Sema/no-builtin.c

Index: clang/test/Sema/no-builtin.c
===
--- /dev/null
+++ clang/test/Sema/no-builtin.c
@@ -0,0 +1,26 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -S -emit-llvm -o - -fsyntax-only -verify %s
+
+void valid_attribute_wildcard() __attribute__((no_builtin("*"))) {}
+void valid_attribute_function() __attribute__((no_builtin("memcpy"))) {}
+void valid_attribute_functions() __attribute__((no_builtin("memcpy"))) __attribute__((no_builtin("memcmp"))) {}
+
+void no_builtin_no_argument() __attribute__((no_builtin)) {}
+// expected-error@-1 {{'no_builtin' attribute takes at least 1 argument}}
+
+void no_builtin_no_argument2() __attribute__((no_builtin())) {}
+// expected-error@-1 {{'no_builtin' attribute takes at least 1 argument}}
+
+void invalid_builtin() __attribute__((no_builtin("not_a_builtin"))) {}
+// expected-error@-1 {{'not_a_builtin' is not a valid builtin name for no_builtin}}
+
+void wildcard_and_functionname() __attribute__((no_builtin("*", "memcpy"))) {}
+// expected-error@-1 {{no_builtin wildcard (*) cannot be composed with other builtin names}}
+
+void wildcard_and_functionname2() __attribute__((no_builtin("*"))) __attribute__((no_builtin("memcpy"))) {}
+// expected-error@-1 {{no_builtin wildcard (*) cannot be composed with other builtin names}}
+
+void nobuiltin_on_declaration() __attribute__((no_builtin("memcpy")));
+// expected-error@-1 {{no_builtin attribute is permitted on definitions only}}
+
+int __attribute__((no_builtin("*"))) variable;
+// expected-warning@-1 {{'no_builtin' attribute only applies to functions}}
Index: clang/test/Misc/pragma-attribute-supported-attributes-list.test
===
--- clang/test/Misc/pragma-attribute-supported-attributes-list.test
+++ clang/test/Misc/pragma-attribute-supported-attributes-list.test
@@ -74,6 +74,7 @@
 // CHECK-NEXT: NSConsumed (SubjectMatchRule_variable_is_parameter)
 // CHECK-NEXT: NSConsumesSelf (SubjectMatchRule_objc_method)
 // CHECK-NEXT: Naked (SubjectMatchRule_function)
+// CHECK-NEXT: NoBuiltin (SubjectMatchRule_function)
 // CHECK-NEXT: NoCommon (SubjectMatchRule_variable)
 // CHECK-NEXT: NoDebug (SubjectMatchRule_type_alias, SubjectMatchRule_hasType_functionType, SubjectMatchRule_objc_method, SubjectMatchRule_variable_not_is_parameter)
 // CHECK-NEXT: NoDestroy (SubjectMatchRule_variable)
Index: clang/test/CodeGen/no-builtin.c
===
--- /dev/null
+++ clang/test/CodeGen/no-builtin.c
@@ -0,0 +1,23 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -S -emit-llvm -o - %s | FileCheck %s
+
+// CHECK-LABEL: define void @foo_no_mempcy() #0
+void foo_no_mempcy() __attribute__((no_builtin("memcpy"))) {}
+
+// CHECK-LABEL: define void @foo_no_mempcy_twice() #0
+void foo_no_mempcy_twice() __attribute__((no_builtin("memcpy"))) __attribute__((no_builtin("memcpy"))) {}
+
+// CHECK-LABEL: define void @foo_no_builtins() #1
+void foo_no_builtins() __attribute__((no_builtin("*"))) {}
+
+// CHECK-LABEL: define void @foo_no_mempcy_memset() #2
+void foo_no_mempcy_memset() __attribute__((no_builtin("memset", "memcpy"))) {}
+
+// CHECK-LABEL: define void @separate_attrs() #2
+void separate_attrs() __attribute__((no_builtin("memset"))) __attribute__((no_builtin("memcpy"))) {}
+
+// CHECK-LABEL: define void @separate_attrs_ordering() #2
+void separate_attrs_ordering() __attribute__((no_builtin("memcpy"))) __attribute__((no_builtin("memset"))) {}
+
+// CHECK: attributes #0 = {{{.*}}"no-builtin-memcpy"{{.*}}}
+// CHECK: attributes #1 = {{{.*}}"no-builtins"{{.*}}}
+// CHECK: attributes #2 = {{{.*}}"no-builtin-memcpy"{{.*}}"no-builtin-memset"{{.*}}}
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -1068,6 +1068,60 @@
   S.Context, AL, Cond, Msg, DiagType, ArgDependent, cast(D)));
 }
 
+NoBuiltinAttr *
+Sema::mergeNoBuiltinAttr(Decl *D, const AttributeCommonInfo ,
+ llvm::ArrayRef BuiltinNames) {
+  llvm::SmallVector Names;
+  bool HasWildcard = false;
+
+  const auto TestAndPushBack = [, ](StringRef Name) {
+HasWildcard |= (Name == "*");
+

[PATCH] D68028: [clang] Add no_builtin attribute

[Clement Courbet via Phabricator via cfe-commits]

courbet added inline comments.



Comment at: clang/include/clang/Basic/Attr.td:3418
+  let Spellings = [Clang<"no_builtin">];
+  let Args = [VariadicStringArgument<"FunctionNames">];
+  let Subjects = SubjectList<[Function]>;

[nit] Should this be `BuiltinNames` ? builtins are not necessarily functions.



Comment at: clang/include/clang/Basic/AttrDocs.td:4409
+
+  // The compiler is not allowed to replace parts of foo's body with builtins.
+  void foo(char* data, size_t count) __attribute__((no_builtin("*"))) {

I would phrase it as `The compiler is not allowed to add any builtin to foo's 
body.` This covers //inserting// as well as //replacing// code with builtins.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68028



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68028: [clang] Add no_builtin attribute

[Guillaume Chatelet via Phabricator via cfe-commits]

gchatelet added a comment.

It should be ready to submit, @aaron.ballman @courbet can you take a final look?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68028



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68028: [clang] Add no_builtin attribute

[Guillaume Chatelet via Phabricator via cfe-commits]

gchatelet updated this revision to Diff 224271.
gchatelet added a comment.

- rebase


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68028

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/Sema.h
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/CodeGen/no-builtin.c
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/Sema/no-builtin.c

Index: clang/test/Sema/no-builtin.c
===
--- /dev/null
+++ clang/test/Sema/no-builtin.c
@@ -0,0 +1,26 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -S -emit-llvm -o - -fsyntax-only -verify %s
+
+void valid_attribute_wildcard() __attribute__((no_builtin("*"))) {}
+void valid_attribute_function() __attribute__((no_builtin("memcpy"))) {}
+void valid_attribute_functions() __attribute__((no_builtin("memcpy"))) __attribute__((no_builtin("memcmp"))) {}
+
+void no_builtin_no_argument() __attribute__((no_builtin)) {}
+// expected-error@-1 {{'no_builtin' attribute takes at least 1 argument}}
+
+void no_builtin_no_argument2() __attribute__((no_builtin())) {}
+// expected-error@-1 {{'no_builtin' attribute takes at least 1 argument}}
+
+void invalid_builtin() __attribute__((no_builtin("not_a_builtin"))) {}
+// expected-error@-1 {{'not_a_builtin' is not a valid function name for no_builtin}}
+
+void wildcard_and_functionname() __attribute__((no_builtin("*", "memcpy"))) {}
+// expected-error@-1 {{no_builtin wildcard (*) cannot be composed with other function names}}
+
+void wildcard_and_functionname2() __attribute__((no_builtin("*"))) __attribute__((no_builtin("memcpy"))) {}
+// expected-error@-1 {{no_builtin wildcard (*) cannot be composed with other function names}}
+
+void nobuiltin_on_declaration() __attribute__((no_builtin("memcpy")));
+// expected-error@-1 {{no_builtin attribute is permitted on definitions only}}
+
+int __attribute__((no_builtin("*"))) variable;
+// expected-warning@-1 {{'no_builtin' attribute only applies to functions}}
Index: clang/test/Misc/pragma-attribute-supported-attributes-list.test
===
--- clang/test/Misc/pragma-attribute-supported-attributes-list.test
+++ clang/test/Misc/pragma-attribute-supported-attributes-list.test
@@ -74,6 +74,7 @@
 // CHECK-NEXT: NSConsumed (SubjectMatchRule_variable_is_parameter)
 // CHECK-NEXT: NSConsumesSelf (SubjectMatchRule_objc_method)
 // CHECK-NEXT: Naked (SubjectMatchRule_function)
+// CHECK-NEXT: NoBuiltin (SubjectMatchRule_function)
 // CHECK-NEXT: NoCommon (SubjectMatchRule_variable)
 // CHECK-NEXT: NoDebug (SubjectMatchRule_type_alias, SubjectMatchRule_hasType_functionType, SubjectMatchRule_objc_method, SubjectMatchRule_variable_not_is_parameter)
 // CHECK-NEXT: NoDestroy (SubjectMatchRule_variable)
Index: clang/test/CodeGen/no-builtin.c
===
--- /dev/null
+++ clang/test/CodeGen/no-builtin.c
@@ -0,0 +1,23 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -S -emit-llvm -o - %s | FileCheck %s
+
+// CHECK-LABEL: define void @foo_no_mempcy() #0
+void foo_no_mempcy() __attribute__((no_builtin("memcpy"))) {}
+
+// CHECK-LABEL: define void @foo_no_mempcy_twice() #0
+void foo_no_mempcy_twice() __attribute__((no_builtin("memcpy"))) __attribute__((no_builtin("memcpy"))) {}
+
+// CHECK-LABEL: define void @foo_no_builtins() #1
+void foo_no_builtins() __attribute__((no_builtin("*"))) {}
+
+// CHECK-LABEL: define void @foo_no_mempcy_memset() #2
+void foo_no_mempcy_memset() __attribute__((no_builtin("memset", "memcpy"))) {}
+
+// CHECK-LABEL: define void @separate_attrs() #2
+void separate_attrs() __attribute__((no_builtin("memset"))) __attribute__((no_builtin("memcpy"))) {}
+
+// CHECK-LABEL: define void @separate_attrs_ordering() #2
+void separate_attrs_ordering() __attribute__((no_builtin("memcpy"))) __attribute__((no_builtin("memset"))) {}
+
+// CHECK: attributes #0 = {{{.*}}"no-builtin-memcpy"{{.*}}}
+// CHECK: attributes #1 = {{{.*}}"no-builtins"{{.*}}}
+// CHECK: attributes #2 = {{{.*}}"no-builtin-memcpy"{{.*}}"no-builtin-memset"{{.*}}}
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -1068,6 +1068,60 @@
   S.Context, AL, Cond, Msg, DiagType, ArgDependent, cast(D)));
 }
 
+NoBuiltinAttr *
+Sema::mergeNoBuiltinAttr(Decl *D, const AttributeCommonInfo ,
+ llvm::ArrayRef FunctionNames) {
+  llvm::SmallVector Names;
+  bool HasWildcard = false;
+
+  const auto TestAndPushBack = [, ](StringRef Name) {
+HasWildcard |= (Name == "*");
+Names.push_back(Name);
+  };
+
+  if (const auto *NBA = 

[PATCH] D68028: [clang] Add no_builtin attribute

[Guillaume Chatelet via Phabricator via cfe-commits]

gchatelet updated this revision to Diff 224085.
gchatelet added a comment.

- Remove leftovers


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68028

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/Sema.h
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/CodeGen/no-builtin.c
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/Sema/no-builtin.c

Index: clang/test/Sema/no-builtin.c
===
--- /dev/null
+++ clang/test/Sema/no-builtin.c
@@ -0,0 +1,26 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -S -emit-llvm -o - -fsyntax-only -verify %s
+
+void valid_attribute_wildcard() __attribute__((no_builtin("*"))) {}
+void valid_attribute_function() __attribute__((no_builtin("memcpy"))) {}
+void valid_attribute_functions() __attribute__((no_builtin("memcpy"))) __attribute__((no_builtin("memcmp"))) {}
+
+void no_builtin_no_argument() __attribute__((no_builtin)) {}
+// expected-error@-1 {{'no_builtin' attribute takes at least 1 argument}}
+
+void no_builtin_no_argument2() __attribute__((no_builtin())) {}
+// expected-error@-1 {{'no_builtin' attribute takes at least 1 argument}}
+
+void invalid_builtin() __attribute__((no_builtin("not_a_builtin"))) {}
+// expected-error@-1 {{'not_a_builtin' is not a valid function name for no_builtin}}
+
+void wildcard_and_functionname() __attribute__((no_builtin("*", "memcpy"))) {}
+// expected-error@-1 {{no_builtin wildcard (*) cannot be composed with other function names}}
+
+void wildcard_and_functionname2() __attribute__((no_builtin("*"))) __attribute__((no_builtin("memcpy"))) {}
+// expected-error@-1 {{no_builtin wildcard (*) cannot be composed with other function names}}
+
+void nobuiltin_on_declaration() __attribute__((no_builtin("memcpy")));
+// expected-error@-1 {{no_builtin attribute is permitted on definitions only}}
+
+int __attribute__((no_builtin("*"))) variable;
+// expected-warning@-1 {{'no_builtin' attribute only applies to functions}}
Index: clang/test/Misc/pragma-attribute-supported-attributes-list.test
===
--- clang/test/Misc/pragma-attribute-supported-attributes-list.test
+++ clang/test/Misc/pragma-attribute-supported-attributes-list.test
@@ -74,6 +74,7 @@
 // CHECK-NEXT: NSConsumed (SubjectMatchRule_variable_is_parameter)
 // CHECK-NEXT: NSConsumesSelf (SubjectMatchRule_objc_method)
 // CHECK-NEXT: Naked (SubjectMatchRule_function)
+// CHECK-NEXT: NoBuiltin (SubjectMatchRule_function)
 // CHECK-NEXT: NoCommon (SubjectMatchRule_variable)
 // CHECK-NEXT: NoDebug (SubjectMatchRule_type_alias, SubjectMatchRule_hasType_functionType, SubjectMatchRule_objc_method, SubjectMatchRule_variable_not_is_parameter)
 // CHECK-NEXT: NoDestroy (SubjectMatchRule_variable)
Index: clang/test/CodeGen/no-builtin.c
===
--- /dev/null
+++ clang/test/CodeGen/no-builtin.c
@@ -0,0 +1,17 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -S -emit-llvm -o - %s | FileCheck %s
+
+// CHECK-LABEL: define void @foo_no_mempcy() #0
+void foo_no_mempcy() __attribute__((no_builtin("memcpy"))) {}
+
+// CHECK-LABEL: define void @foo_no_builtins() #1
+void foo_no_builtins() __attribute__((no_builtin("*"))) {}
+
+// CHECK-LABEL: define void @foo_no_mempcy_memset() #2
+void foo_no_mempcy_memset() __attribute__((no_builtin("memset", "memcpy"))) {}
+
+// CHECK-LABEL: define void @separate_attrs() #2
+void separate_attrs() __attribute__((no_builtin("memset"))) __attribute__((no_builtin("memcpy"))) {}
+
+// CHECK: attributes #0 = {{{.*}}"no-builtin-memcpy"{{.*}}}
+// CHECK: attributes #1 = {{{.*}}"no-builtins"{{.*}}}
+// CHECK: attributes #2 = {{{.*}}"no-builtin-memcpy"{{.*}}"no-builtin-memset"{{.*}}}
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -1068,6 +1068,56 @@
   S.Context, AL, Cond, Msg, DiagType, ArgDependent, cast(D)));
 }
 
+NoBuiltinAttr *
+Sema::mergeNoBuiltinAttr(Decl *D, const AttributeCommonInfo ,
+ llvm::ArrayRef FunctionNames) {
+  llvm::SmallVector Names;
+  bool HasWildcard = false;
+
+  const auto TestAndPushBack = [, ](StringRef Name) {
+HasWildcard |= (Name == "*");
+Names.push_back(Name);
+  };
+
+  if (const auto *NBA = D->getAttr())
+for (StringRef FunctionName : NBA->functionNames())
+  TestAndPushBack(FunctionName);
+
+  for (StringRef FunctionName : FunctionNames)
+TestAndPushBack(FunctionName);
+
+  if (HasWildcard && Names.size() > 1)
+Diag(D->getLocation(),
+ diag::err_attribute_no_builtin_wildcard_or_functionname)
+ 

[PATCH] D68028: [clang] Add no_builtin attribute

[Guillaume Chatelet via Phabricator via cfe-commits]

gchatelet updated this revision to Diff 224082.
gchatelet added a comment.

- Address aaron ballman comments
- Checks function name validity and errors when passed 0 argument.
- Update documentation and rebase
- Rewrote mergeNoBuiltinAttr
- Address comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68028

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/Sema.h
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/CodeGen/no-builtin.c
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/Sema/no-builtin.c

Index: clang/test/Sema/no-builtin.c
===
--- /dev/null
+++ clang/test/Sema/no-builtin.c
@@ -0,0 +1,26 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -S -emit-llvm -o - -fsyntax-only -verify %s
+
+void valid_attribute_wildcard() __attribute__((no_builtin("*"))) {}
+void valid_attribute_function() __attribute__((no_builtin("memcpy"))) {}
+void valid_attribute_functions() __attribute__((no_builtin("memcpy"))) __attribute__((no_builtin("memcmp"))) {}
+
+void no_builtin_no_argument() __attribute__((no_builtin)) {}
+// expected-error@-1 {{'no_builtin' attribute takes at least 1 argument}}
+
+void no_builtin_no_argument2() __attribute__((no_builtin())) {}
+// expected-error@-1 {{'no_builtin' attribute takes at least 1 argument}}
+
+void invalid_builtin() __attribute__((no_builtin("not_a_builtin"))) {}
+// expected-error@-1 {{'not_a_builtin' is not a valid function name for no_builtin}}
+
+void wildcard_and_functionname() __attribute__((no_builtin("*", "memcpy"))) {}
+// expected-error@-1 {{no_builtin wildcard (*) cannot be composed with other function names}}
+
+void wildcard_and_functionname2() __attribute__((no_builtin("*"))) __attribute__((no_builtin("memcpy"))) {}
+// expected-error@-1 {{no_builtin wildcard (*) cannot be composed with other function names}}
+
+void nobuiltin_on_declaration() __attribute__((no_builtin("memcpy")));
+// expected-error@-1 {{no_builtin attribute is permitted on definitions only}}
+
+int __attribute__((no_builtin("*"))) variable;
+// expected-warning@-1 {{'no_builtin' attribute only applies to functions}}
Index: clang/test/Misc/pragma-attribute-supported-attributes-list.test
===
--- clang/test/Misc/pragma-attribute-supported-attributes-list.test
+++ clang/test/Misc/pragma-attribute-supported-attributes-list.test
@@ -74,6 +74,7 @@
 // CHECK-NEXT: NSConsumed (SubjectMatchRule_variable_is_parameter)
 // CHECK-NEXT: NSConsumesSelf (SubjectMatchRule_objc_method)
 // CHECK-NEXT: Naked (SubjectMatchRule_function)
+// CHECK-NEXT: NoBuiltin (SubjectMatchRule_function)
 // CHECK-NEXT: NoCommon (SubjectMatchRule_variable)
 // CHECK-NEXT: NoDebug (SubjectMatchRule_type_alias, SubjectMatchRule_hasType_functionType, SubjectMatchRule_objc_method, SubjectMatchRule_variable_not_is_parameter)
 // CHECK-NEXT: NoDestroy (SubjectMatchRule_variable)
Index: clang/test/CodeGen/no-builtin.c
===
--- /dev/null
+++ clang/test/CodeGen/no-builtin.c
@@ -0,0 +1,20 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -S -emit-llvm -o - %s | FileCheck %s
+
+// CHECK-LABEL: define void @foo_no_mempcy() #0
+void foo_no_mempcy() __attribute__((no_builtin("memcpy"))) {}
+
+// CHECK-LABEL: define void @foo_no_builtins() #1
+void foo_no_builtins() __attribute__((no_builtin("*"))) {}
+
+// CHECK-LABEL: define void @foo_no_mempcy_memset() #2
+void foo_no_mempcy_memset() __attribute__((no_builtin("memset", "memcpy"))) {}
+
+// CHECK-LABEL: define void @separate_attrs() #2
+void separate_attrs() __attribute__((no_builtin("memset"))) __attribute__((no_builtin("memcpy"))) {}
+
+// CHECK-LABEL: define void @wildcard_wins() #1
+void wildcard_wins() __attribute__((no_builtin("memset"))) __attribute__((no_builtin("*"))) __attribute__((no_builtin("memcpy"))) {}
+
+// CHECK: attributes #0 = {{{.*}}"no-builtin-memcpy"{{.*}}}
+// CHECK: attributes #1 = {{{.*}}"no-builtins"{{.*}}}
+// CHECK: attributes #2 = {{{.*}}"no-builtin-memcpy"{{.*}}"no-builtin-memset"{{.*}}}
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -1068,6 +1068,56 @@
   S.Context, AL, Cond, Msg, DiagType, ArgDependent, cast(D)));
 }
 
+NoBuiltinAttr *
+Sema::mergeNoBuiltinAttr(Decl *D, const AttributeCommonInfo ,
+ llvm::ArrayRef FunctionNames) {
+  llvm::SmallVector Names;
+  bool HasWildcard = false;
+
+  const auto TestAndPushBack = [, ](StringRef Name) {
+HasWildcard |= (Name == "*");
+Names.push_back(Name);
+  };
+
+  if 

[PATCH] D68028: [clang] Add no_builtin attribute

[Guillaume Chatelet via Phabricator via cfe-commits]

gchatelet marked an inline comment as done.
gchatelet added inline comments.



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:1098-1099
+
+  if (D->hasAttr())
+D->dropAttr();
+

gchatelet wrote:
> aaron.ballman wrote:
> > 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.
> > 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?!
> > You got four of the six. What about the use in 
> > handleObjCSuppresProtocolAttr() and the second use in handleAliasAttr()?
> 
> It really is `FunctionDecl::isThisDeclarationADefinition` that is buggy, the 
> other places where we call `isThisDeclarationADefinition` in 
> `SemaDeclAttr.cpp` are ok, i.e. `VarDecl::isThisDeclarationADefinition` and 
> `ObjCProtocolDecl::isThisDeclarationADefinition`
> 
> I tried to use `FunctionDecl::setWillHaveBody`but it broke many tests so it 
> seems inappropriate.
> 
I've tried to fix it in D68701. I'm pretty sure this is not the right way to do 
it though,
@rsmith any idea on how to proceed?

In the meantime I'll add a FIXME and move on with this patch if you don't mind.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68028



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68028: [clang] Add no_builtin attribute

[Guillaume Chatelet via Phabricator via cfe-commits]

gchatelet marked 2 inline comments as done.
gchatelet added inline comments.



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:1098-1099
+
+  if (D->hasAttr())
+D->dropAttr();
+

aaron.ballman wrote:
> 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.
> 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?!
> You got four of the six. What about the use in 
> handleObjCSuppresProtocolAttr() and the second use in handleAliasAttr()?

It really is `FunctionDecl::isThisDeclarationADefinition` that is buggy, the 
other places where we call `isThisDeclarationADefinition` in `SemaDeclAttr.cpp` 
are ok, i.e. `VarDecl::isThisDeclarationADefinition` and 
`ObjCProtocolDecl::isThisDeclarationADefinition`

I tried to use `FunctionDecl::setWillHaveBody`but it broke many tests so it 
seems inappropriate.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68028



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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 , Decl *D, const AttributeCommonInfo ,
+ llvm::ArrayRef 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())
+D->dropAttr();
+

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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68028: [clang] Add no_builtin attribute

[Guillaume Chatelet via Phabricator via cfe-commits]

gchatelet marked 3 inline comments as done.
gchatelet added inline comments.



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:1098-1099
+
+  if (D->hasAttr())
+D->dropAttr();
+

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 ]].


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68028



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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 , Decl *D, const AttributeCommonInfo ,
+ llvm::ArrayRef 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())
+for (StringRef FunctionName : D->getAttr()->functionNames())
+  FunctionNamesSet.insert(FunctionName);

aaron.ballman wrote:
> Instead of doing `hasAttr<>` followed by `getAttr<>`, this should be:
> ```
> if (const auto *NBA = D->getAttr()) {
>   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())
+D->dropAttr();
+

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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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 , Decl *D, const AttributeCommonInfo ,
+ llvm::ArrayRef 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())
+for (StringRef FunctionName : D->getAttr()->functionNames())
+  FunctionNamesSet.insert(FunctionName);

Instead of doing `hasAttr<>` followed by `getAttr<>`, this should be:
```
if (const auto *NBA = D->getAttr()) {
  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())
+D->dropAttr();
+

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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68028: [clang] Add no_builtin attribute

[Teresa Johnson via Phabricator via cfe-commits]

tejohnson added a comment.

In D68028#1685912 , @gchatelet wrote:

> @tejohnson I believe this is the missing part for D67923 
> .


Thanks, yep I will take a closer look at the patch today.

> I'm unsure if we still need the `BitVector` at all in the `TLI` since it 
> could be a simple attribute lookup on the function.

If we didn't save the info on the TLI we would instead need to save the 
Function object in the TLI and query the attribute info off the Function on 
every lookup, which seems heavier weight. I think caching the info in that 
object for fast lookup is going to be better. And as noted in the comments 
there, we can replace it with the existing AvailableArray moved from the base 
Impl object into the TLI and remove the override bitvector once this goes in, 
we use these attributes to set the TLI info on construction, and we remove the 
clang code that sets the unavailability from the CodeGenOpts which will no 
longer be needed. If this patch goes in first I can just modify my TLI patch to 
do that all in one go. Maybe that is best...

> Do you see any problematic interactions with the inlining phase?

The inliner will need to be modified to respect the function attributes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68028



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68028: [clang] Add no_builtin attribute

[Guillaume Chatelet via Phabricator via cfe-commits]

gchatelet updated this revision to Diff 222163.
gchatelet added a comment.

- Update documentation and rebase


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68028

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Sema/Sema.h
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/CodeGen/no-builtin.c
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/Sema/no-builtin.c

Index: clang/test/Sema/no-builtin.c
===
--- /dev/null
+++ clang/test/Sema/no-builtin.c
@@ -0,0 +1,9 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -S -emit-llvm -o - -verify %s
+
+void foo() __attribute__((no_builtin)) {} // expected-error {{'no_builtin' attribute takes at least 1 argument}}
+
+void bar() __attribute__((no_builtin())) {} // expected-error {{'no_builtin' attribute takes at least 1 argument}}
+
+void invalid_builtin() __attribute__((no_builtin("not_a_builtin"))) {} // expected-error {{use of unknown builtin not_a_builtin}}
+
+int __attribute__((no_builtin("*"))) variable; // expected-warning {{'no_builtin' attribute only applies to functions}}
Index: clang/test/Misc/pragma-attribute-supported-attributes-list.test
===
--- clang/test/Misc/pragma-attribute-supported-attributes-list.test
+++ clang/test/Misc/pragma-attribute-supported-attributes-list.test
@@ -74,6 +74,7 @@
 // CHECK-NEXT: NSConsumed (SubjectMatchRule_variable_is_parameter)
 // CHECK-NEXT: NSConsumesSelf (SubjectMatchRule_objc_method)
 // CHECK-NEXT: Naked (SubjectMatchRule_function)
+// CHECK-NEXT: NoBuiltin (SubjectMatchRule_function)
 // CHECK-NEXT: NoCommon (SubjectMatchRule_variable)
 // CHECK-NEXT: NoDebug (SubjectMatchRule_type_alias, SubjectMatchRule_hasType_functionType, SubjectMatchRule_objc_method, SubjectMatchRule_variable_not_is_parameter)
 // CHECK-NEXT: NoDestroy (SubjectMatchRule_variable)
Index: clang/test/CodeGen/no-builtin.c
===
--- /dev/null
+++ clang/test/CodeGen/no-builtin.c
@@ -0,0 +1,20 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -S -emit-llvm -o - %s | FileCheck %s
+
+// CHECK-LABEL: define void @foo_no_mempcy() #0
+void foo_no_mempcy() __attribute__((no_builtin("memcpy"))) {}
+
+// CHECK-LABEL: define void @foo_no_builtins() #1
+void foo_no_builtins() __attribute__((no_builtin("*"))) {}
+
+// CHECK-LABEL: define void @foo_no_mempcy_memset() #2
+void foo_no_mempcy_memset() __attribute__((no_builtin("memset", "memcpy"))) {}
+
+// CHECK-LABEL: define void @separate_attrs() #2
+void separate_attrs() __attribute__((no_builtin("memset"))) __attribute__((no_builtin("memcpy"))) {}
+
+// CHECK-LABEL: define void @wildcard_wins() #1
+void wildcard_wins() __attribute__((no_builtin("memset"))) __attribute__((no_builtin("*"))) __attribute__((no_builtin("memcpy"))) {}
+
+// CHECK: attributes #0 = {{{.*}}"no-builtin-memcpy"{{.*}}}
+// CHECK: attributes #1 = {{{.*}}"no-builtins"{{.*}}}
+// CHECK: attributes #2 = {{{.*}}"no-builtin-memcpy"{{.*}}"no-builtin-memset"{{.*}}}
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -1068,6 +1068,62 @@
   S.Context, AL, Cond, Msg, DiagType, ArgDependent, cast(D)));
 }
 
+NoBuiltinAttr *
+Sema::mergeNoBuiltinAttr(Sema , Decl *D, const AttributeCommonInfo ,
+ llvm::ArrayRef FunctionNames) {
+  const StringRef Wildcard = "*";
+  llvm::SmallSetVector FunctionNamesSet;
+
+  // Insert previous NoBuiltin attributes.
+  if (D->hasAttr())
+for (StringRef FunctionName : D->getAttr()->functionNames())
+  FunctionNamesSet.insert(FunctionName);
+  // Insert new NoBuiltin attributes.
+  for (StringRef FunctionName : FunctionNames)
+FunctionNamesSet.insert(FunctionName);
+
+  // Wildcard is a superset of all builtins, we keep only this one.
+  if (FunctionNamesSet.count(Wildcard) > 0) {
+FunctionNamesSet.clear();
+FunctionNamesSet.insert(Wildcard);
+  }
+
+  assert((FunctionNamesSet.count(Wildcard) == 0) ||
+ (FunctionNamesSet.size() == 1) && "Wildcard must be on its own");
+
+  llvm::SmallVector UniqFunctionNames =
+  FunctionNamesSet.takeVector();
+  llvm::sort(UniqFunctionNames);
+
+  if (D->hasAttr())
+D->dropAttr();
+
+  return ::new (S.Context) NoBuiltinAttr(
+  S.Context, CI, UniqFunctionNames.data(), UniqFunctionNames.size());
+}
+
+static void handleNoBuiltin(Sema , Decl *D, const ParsedAttr ) {
+  if (!checkAttributeAtLeastNumArgs(S, AL, 1))
+return;
+
+  llvm::SmallVector FunctionNames;
+  for (unsigned I = 0, E = AL.getNumArgs(); I != E; ++I) {
+StringRef FunctionName;
+SourceLocation LiteralLoc;
+if 

[PATCH] D68028: [clang] Add no_builtin attribute

[Guillaume Chatelet via Phabricator via cfe-commits]

gchatelet added a comment.

@tejohnson I believe this is the missing part for D67923 
.

I'm unsure if we still need the `BitVector` at all in the `TLI` since it could 
be a simple attribute lookup on the function. Do you see any problematic 
interactions with the inlining phase?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68028



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68028: [clang] Add no_builtin attribute

[Guillaume Chatelet via Phabricator via cfe-commits]

gchatelet updated this revision to Diff 221939.
gchatelet added a comment.

- Checks function name validity and errors when passed 0 argument.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68028

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Sema/Sema.h
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/CodeGen/no-builtin.c
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/Sema/no-builtin.c

Index: clang/test/Sema/no-builtin.c
===
--- /dev/null
+++ clang/test/Sema/no-builtin.c
@@ -0,0 +1,9 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -S -emit-llvm -o - -verify %s
+
+void foo() __attribute__((no_builtin)) {} // expected-error {{'no_builtin' attribute takes at least 1 argument}}
+
+void bar() __attribute__((no_builtin())) {} // expected-error {{'no_builtin' attribute takes at least 1 argument}}
+
+void invalid_builtin() __attribute__((no_builtin("not_a_builtin"))) {} // expected-error {{use of unknown builtin not_a_builtin}}
+
+int __attribute__((no_builtin("*"))) variable; // expected-warning {{'no_builtin' attribute only applies to functions}}
Index: clang/test/Misc/pragma-attribute-supported-attributes-list.test
===
--- clang/test/Misc/pragma-attribute-supported-attributes-list.test
+++ clang/test/Misc/pragma-attribute-supported-attributes-list.test
@@ -74,6 +74,7 @@
 // CHECK-NEXT: NSConsumed (SubjectMatchRule_variable_is_parameter)
 // CHECK-NEXT: NSConsumesSelf (SubjectMatchRule_objc_method)
 // CHECK-NEXT: Naked (SubjectMatchRule_function)
+// CHECK-NEXT: NoBuiltin (SubjectMatchRule_function)
 // CHECK-NEXT: NoCommon (SubjectMatchRule_variable)
 // CHECK-NEXT: NoDebug (SubjectMatchRule_type_alias, SubjectMatchRule_hasType_functionType, SubjectMatchRule_objc_method, SubjectMatchRule_variable_not_is_parameter)
 // CHECK-NEXT: NoDestroy (SubjectMatchRule_variable)
Index: clang/test/CodeGen/no-builtin.c
===
--- /dev/null
+++ clang/test/CodeGen/no-builtin.c
@@ -0,0 +1,20 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -S -emit-llvm -o - %s | FileCheck %s
+
+// CHECK-LABEL: define void @foo_no_mempcy() #0
+void foo_no_mempcy() __attribute__((no_builtin("memcpy"))) {}
+
+// CHECK-LABEL: define void @foo_no_builtins() #1
+void foo_no_builtins() __attribute__((no_builtin("*"))) {}
+
+// CHECK-LABEL: define void @foo_no_mempcy_memset() #2
+void foo_no_mempcy_memset() __attribute__((no_builtin("memset", "memcpy"))) {}
+
+// CHECK-LABEL: define void @separate_attrs() #2
+void separate_attrs() __attribute__((no_builtin("memset"))) __attribute__((no_builtin("memcpy"))) {}
+
+// CHECK-LABEL: define void @wildcard_wins() #1
+void wildcard_wins() __attribute__((no_builtin("memset"))) __attribute__((no_builtin("*"))) __attribute__((no_builtin("memcpy"))) {}
+
+// CHECK: attributes #0 = {{{.*}}"no-builtin-memcpy"{{.*}}}
+// CHECK: attributes #1 = {{{.*}}"no-builtins"{{.*}}}
+// CHECK: attributes #2 = {{{.*}}"no-builtin-memcpy"{{.*}}"no-builtin-memset"{{.*}}}
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -1068,6 +1068,62 @@
   S.Context, AL, Cond, Msg, DiagType, ArgDependent, cast(D)));
 }
 
+NoBuiltinAttr *
+Sema::mergeNoBuiltinAttr(Sema , Decl *D, const AttributeCommonInfo ,
+ llvm::ArrayRef FunctionNames) {
+  const StringRef Wildcard = "*";
+  llvm::SmallSetVector FunctionNamesSet;
+
+  // Insert previous NoBuiltin attributes.
+  if (D->hasAttr())
+for (StringRef FunctionName : D->getAttr()->functionNames())
+  FunctionNamesSet.insert(FunctionName);
+  // Insert new NoBuiltin attributes.
+  for (StringRef FunctionName : FunctionNames)
+FunctionNamesSet.insert(FunctionName);
+
+  // Wildcard is a superset of all builtins, we keep only this one.
+  if (FunctionNamesSet.count(Wildcard) > 0) {
+FunctionNamesSet.clear();
+FunctionNamesSet.insert(Wildcard);
+  }
+
+  assert((FunctionNamesSet.count(Wildcard) == 0) ||
+ (FunctionNamesSet.size() == 1) && "Wildcard must be on its own");
+
+  llvm::SmallVector UniqFunctionNames =
+  FunctionNamesSet.takeVector();
+  llvm::sort(UniqFunctionNames);
+
+  if (D->hasAttr())
+D->dropAttr();
+
+  return ::new (S.Context) NoBuiltinAttr(
+  S.Context, CI, UniqFunctionNames.data(), UniqFunctionNames.size());
+}
+
+static void handleNoBuiltin(Sema , Decl *D, const ParsedAttr ) {
+  if (!checkAttributeAtLeastNumArgs(S, AL, 1))
+return;
+
+  llvm::SmallVector FunctionNames;
+  for (unsigned I = 0, E = AL.getNumArgs(); I != E; ++I) {
+StringRef FunctionName;
+SourceLocation 

[PATCH] D68028: [clang] Add no_builtin attribute

[Guillaume Chatelet via Phabricator via cfe-commits]

gchatelet added a comment.

@aaron.ballman thx a lot for the review. I really appreciate it, especially 
because I'm not too familiar with this part of the codebase.




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

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68028



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68028: [clang] Add no_builtin attribute

[Guillaume Chatelet via Phabricator via cfe-commits]

gchatelet updated this revision to Diff 221924.
gchatelet marked 11 inline comments as done.
gchatelet added a comment.

- Address aaron ballman comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68028

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Sema/Sema.h
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/CodeGen/no-builtin.c
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/Sema/no-builtin.c

Index: clang/test/Sema/no-builtin.c
===
--- /dev/null
+++ clang/test/Sema/no-builtin.c
@@ -0,0 +1,4 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -S -emit-llvm -o - -verify %s
+
+void foo() __attribute__((no_builtin)) {}   // expected-error {{'no_builtin' attribute takes at least 1 argument}}
+void bar() __attribute__((no_builtin())) {} // expected-error {{'no_builtin' attribute takes at least 1 argument}}
Index: clang/test/Misc/pragma-attribute-supported-attributes-list.test
===
--- clang/test/Misc/pragma-attribute-supported-attributes-list.test
+++ clang/test/Misc/pragma-attribute-supported-attributes-list.test
@@ -74,6 +74,7 @@
 // CHECK-NEXT: NSConsumed (SubjectMatchRule_variable_is_parameter)
 // CHECK-NEXT: NSConsumesSelf (SubjectMatchRule_objc_method)
 // CHECK-NEXT: Naked (SubjectMatchRule_function)
+// CHECK-NEXT: NoBuiltin (SubjectMatchRule_function)
 // CHECK-NEXT: NoCommon (SubjectMatchRule_variable)
 // CHECK-NEXT: NoDebug (SubjectMatchRule_type_alias, SubjectMatchRule_hasType_functionType, SubjectMatchRule_objc_method, SubjectMatchRule_variable_not_is_parameter)
 // CHECK-NEXT: NoDestroy (SubjectMatchRule_variable)
Index: clang/test/CodeGen/no-builtin.c
===
--- /dev/null
+++ clang/test/CodeGen/no-builtin.c
@@ -0,0 +1,20 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -S -emit-llvm -o - %s | FileCheck %s
+
+// CHECK-LABEL: define void @foo_no_mempcy() #0
+void foo_no_mempcy() __attribute__((no_builtin("memcpy"))) {}
+
+// CHECK-LABEL: define void @foo_no_builtins() #1
+void foo_no_builtins() __attribute__((no_builtin("*"))) {}
+
+// CHECK-LABEL: define void @foo_no_mempcy_memset() #2
+void foo_no_mempcy_memset() __attribute__((no_builtin("memset", "memcpy"))) {}
+
+// CHECK-LABEL: define void @separate_attrs() #2
+void separate_attrs() __attribute__((no_builtin("memset"))) __attribute__((no_builtin("memcpy"))) {}
+
+// CHECK-LABEL: define void @wildcard_wins() #1
+void wildcard_wins() __attribute__((no_builtin("memset"))) __attribute__((no_builtin("*"))) __attribute__((no_builtin("memcpy"))) {}
+
+// CHECK: attributes #0 = {{{.*}}"no-builtin-memcpy"{{.*}}}
+// CHECK: attributes #1 = {{{.*}}"no-builtins"{{.*}}}
+// CHECK: attributes #2 = {{{.*}}"no-builtin-memcpy"{{.*}}"no-builtin-memset"{{.*}}}
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -1068,6 +1068,59 @@
   S.Context, AL, Cond, Msg, DiagType, ArgDependent, cast(D)));
 }
 
+NoBuiltinAttr *
+Sema::mergeNoBuiltinAttr(Sema , Decl *D, const AttributeCommonInfo ,
+ llvm::ArrayRef FunctionNames) {
+  const StringRef Wildcard = "*";
+  llvm::SmallSetVector FunctionNamesSet;
+
+  // Insert previous NoBuiltin attributes.
+  if (D->hasAttr())
+for (StringRef FunctionName : D->getAttr()->functionNames())
+  FunctionNamesSet.insert(FunctionName);
+  // Insert new NoBuiltin attributes.
+  for (StringRef FunctionName : FunctionNames)
+FunctionNamesSet.insert(FunctionName);
+
+  // Wildcard is a superset of all builtins, we keep only this one.
+  if (FunctionNamesSet.count(Wildcard) > 0) {
+FunctionNamesSet.clear();
+FunctionNamesSet.insert(Wildcard);
+  }
+
+  assert((FunctionNamesSet.count(Wildcard) == 0) ||
+ (FunctionNamesSet.size() == 1) && "Wildcard must be on its own");
+
+  llvm::SmallVector UniqFunctionNames =
+  FunctionNamesSet.takeVector();
+  llvm::sort(UniqFunctionNames);
+
+  if (D->hasAttr())
+D->dropAttr();
+
+  return ::new (S.Context) NoBuiltinAttr(
+  S.Context, CI, UniqFunctionNames.data(), UniqFunctionNames.size());
+}
+
+static void handleNoBuiltin(Sema , Decl *D, const ParsedAttr ) {
+  if (!checkAttributeAtLeastNumArgs(S, AL, 1))
+return;
+
+  llvm::SmallVector FunctionNames;
+  for (unsigned I = 0, E = AL.getNumArgs(); I != E; ++I) {
+StringRef FunctionName;
+SourceLocation LiteralLoc;
+if (!S.checkStringLiteralArgumentAttr(AL, I, FunctionName, ))
+  return;
+
+// TODO: check that function names are valid for the
+// TargetLibraryInfo.
+FunctionNames.push_back(FunctionName);
+  }
+
+  

[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()) {
+  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  : 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())
+for (StringRef FunctionName : D->getAttr()->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())
+D->dropAttr();
+

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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68028: [clang] Add no_builtin attribute

[Guillaume Chatelet via Phabricator via cfe-commits]

gchatelet created this revision.
gchatelet added reviewers: tejohnson, courbet, theraven, t.p.northover, 
jdoerfert.
Herald added subscribers: cfe-commits, mgrang.
Herald added a project: clang.

This is a follow up on https://reviews.llvm.org/D61634
This patch is simpler and only adds the no_builtin attribute.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D68028

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/CodeGen/no-builtin.c
  clang/test/Misc/pragma-attribute-supported-attributes-list.test

Index: clang/test/Misc/pragma-attribute-supported-attributes-list.test
===
--- clang/test/Misc/pragma-attribute-supported-attributes-list.test
+++ clang/test/Misc/pragma-attribute-supported-attributes-list.test
@@ -74,6 +74,7 @@
 // CHECK-NEXT: NSConsumed (SubjectMatchRule_variable_is_parameter)
 // CHECK-NEXT: NSConsumesSelf (SubjectMatchRule_objc_method)
 // CHECK-NEXT: Naked (SubjectMatchRule_function)
+// CHECK-NEXT: NoBuiltin (SubjectMatchRule_function)
 // CHECK-NEXT: NoCommon (SubjectMatchRule_variable)
 // CHECK-NEXT: NoDebug (SubjectMatchRule_type_alias, SubjectMatchRule_hasType_functionType, SubjectMatchRule_objc_method, SubjectMatchRule_variable_not_is_parameter)
 // CHECK-NEXT: NoDestroy (SubjectMatchRule_variable)
Index: clang/test/CodeGen/no-builtin.c
===
--- /dev/null
+++ clang/test/CodeGen/no-builtin.c
@@ -0,0 +1,20 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -S -emit-llvm -o - %s | FileCheck %s
+
+// CHECK-LABEL: define void @foo_no_mempcy() #0
+void foo_no_mempcy() __attribute__((no_builtin("memcpy"))) {}
+
+// CHECK-LABEL: define void @foo_no_builtins() #1
+void foo_no_builtins() __attribute__((no_builtin)) {}
+
+// CHECK-LABEL: define void @foo_no_mempcy_memset() #2
+void foo_no_mempcy_memset() __attribute__((no_builtin("memset", "memcpy"))) {}
+
+// CHECK-LABEL: define void @separate_attrs() #2
+void separate_attrs() __attribute__((no_builtin("memset"))) __attribute__((no_builtin("memcpy"))) {}
+
+// CHECK-LABEL: define void @wildcard_wins() #1
+void wildcard_wins() __attribute__((no_builtin("memset"))) __attribute__((no_builtin)) __attribute__((no_builtin("memcpy"))) {}
+
+// CHECK: attributes #0 = {{{.*}}"no-builtin-memcpy"{{.*}}}
+// CHECK: attributes #1 = {{{.*}}"no-builtins"{{.*}}}
+// CHECK: attributes #2 = {{{.*}}"no-builtin-memcpy"{{.*}}"no-builtin-memset"{{.*}}}
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -1068,6 +1068,42 @@
   S.Context, AL, Cond, Msg, DiagType, ArgDependent, cast(D)));
 }
 
+static void handleNoBuiltin(Sema , Decl *D, const ParsedAttr ) {
+  const StringRef Wildcard = "*";
+  llvm::SmallSetVector FunctionNames;
+
+  // Insert previous NoBuiltin attributes.
+  if (D->hasAttr())
+for (StringRef FunctionName : D->getAttr()->functionNames())
+  FunctionNames.insert(FunctionName);
+
+  if (AL.getNumArgs() == 0)
+FunctionNames.insert(Wildcard);
+  else
+for (unsigned I = 0, E = AL.getNumArgs(); I != E; ++I) {
+  StringRef FunctionName;
+  SourceLocation LiteralLoc;
+  if (!S.checkStringLiteralArgumentAttr(AL, I, FunctionName, ))
+return;
+  FunctionNames.insert(FunctionName);
+}
+
+  // Wildcard is a super set of all builtins, we keep only this one.
+  if (FunctionNames.count(Wildcard) > 0) {
+FunctionNames.clear();
+FunctionNames.insert(Wildcard);
+  }
+
+  auto UniqueFunctionNames = FunctionNames.takeVector();
+  llvm::sort(UniqueFunctionNames);
+
+  if (D->hasAttr())
+D->dropAttr();
+
+  D->addAttr(::new (S.Context) NoBuiltinAttr(
+  S.Context, AL, UniqueFunctionNames.data(), UniqueFunctionNames.size()));
+}
+
 static void handlePassObjectSizeAttr(Sema , Decl *D, const ParsedAttr ) {
   if (D->hasAttr()) {
 S.Diag(D->getBeginLoc(), diag::err_attribute_only_once_per_parameter) << AL;
@@ -6573,6 +6609,9 @@
   case ParsedAttr::AT_DiagnoseIf:
 handleDiagnoseIfAttr(S, D, AL);
 break;
+  case ParsedAttr::AT_NoBuiltin:
+handleNoBuiltin(S, D, AL);
+break;
   case ParsedAttr::AT_ExtVectorType:
 handleExtVectorTypeAttr(S, D, AL);
 break;
Index: clang/lib/CodeGen/CGCall.cpp
===
--- clang/lib/CodeGen/CGCall.cpp
+++ clang/lib/CodeGen/CGCall.cpp
@@ -1849,6 +1849,22 @@
   FuncAttrs.addAttribute(llvm::Attribute::NoDuplicate);
 if (TargetDecl->hasAttr())
   FuncAttrs.addAttribute(llvm::Attribute::Convergent);
+if (const auto *Attr = TargetDecl->getAttr()) {
+  const auto HasWildcard = llvm::is_contained(Attr->functionNames(), "*");
+  assert(!HasWildcard ||
+ Attr->functionNames_size() == 1 &&