[Clang] Permit noescape on non-pointer types

[Xazax-hun]

In modern C++ we often use span, string_view or other view objects instead of raw pointers. This PR opens the door to mark those noescape. This can be useful to document the API contracts, for interop with memory safe languages like Swift or Rust, and possibly in the future to implement take this into account in codegen.

The PR also adds a feature test macro. The goal is to help avoiding warnings when the code is compiler by earlier versions of clang that does not permit this attribute on non-pointer types.

[llvmbot]

@llvm/pr-subscribers-clang-codegen

@llvm/pr-subscribers-clang

Author: Gábor Horváth (Xazax-hun)

Changes

In modern C++ we often use span, string_view or other view objects instead of raw pointers. This PR opens the door to mark those noescape. This can be useful to document the API contracts, for interop with memory safe languages like Swift or Rust, and possibly in the future to implement take this into account in codegen.

The PR also adds a feature. The goal is to help avoiding warnings when the code is compiler by earlier versions of clang that does not permit this attribute on non-pointer types.

---

Full diff: https://github.com/llvm/llvm-project/pull/117344.diff

5 Files Affected:

• (modified) clang/include/clang/Basic/AttrDocs.td (+6-6)
• (modified) clang/include/clang/Basic/Features.def (+1)
• (modified) clang/lib/Sema/SemaDeclAttr.cpp (-8)
• (modified) clang/test/SemaCXX/noescape-attr.cpp (+7-2)
• (modified) clang/test/SemaObjCXX/noescape.mm (+3-3)

diff --git a/clang/include/clang/Basic/AttrDocs.td b/clang/include/clang/Basic/AttrDocs.td
index 6fb2eb3eb3e663..e41ac26a6a03e9 100644
--- a/clang/include/clang/Basic/AttrDocs.td
+++ b/clang/include/clang/Basic/AttrDocs.td
@@ -228,12 +228,12 @@ members, and static locals.
 def NoEscapeDocs : Documentation {
   let Category = DocCatVariable;
   let Content = [{
-``noescape`` placed on a function parameter of a pointer type is used to inform
-the compiler that the pointer cannot escape: that is, no reference to the object
-the pointer points to that is derived from the parameter value will survive
-after the function returns. Users are responsible for making sure parameters
-annotated with ``noescape`` do not actually escape. Calling ``free()`` on such
-a parameter does not constitute an escape.
+``noescape`` placed on a function parameter is used to inform the compiler that
+the pointer cannot escape: that is, no reference to the object the pointer
+points to that is derived from the parameter value will survive after the
+function returns. Users are responsible for making sure parameters annotated
+with ``noescape`` do not actually escape. Calling ``free()`` on such a
+parameter does not constitute an escape.
 
 For example:
 
diff --git a/clang/include/clang/Basic/Features.def b/clang/include/clang/Basic/Features.def
index 9088c867d53ce4..5a5cd8f77311cd 100644
--- a/clang/include/clang/Basic/Features.def
+++ b/clang/include/clang/Basic/Features.def
@@ -87,6 +87,7 @@ FEATURE(attribute_overloadable, true)
 FEATURE(attribute_unavailable_with_message, true)
 FEATURE(attribute_unused_on_fields, true)
 FEATURE(attribute_diagnose_if_objc, true)
+FEATURE(attribute_noescape_nonpointer, true)
 FEATURE(blocks, LangOpts.Blocks)
 FEATURE(c_thread_safety_attributes, true)
 FEATURE(cxx_exceptions, LangOpts.CXXExceptions)
diff --git a/clang/lib/Sema/SemaDeclAttr.cpp b/clang/lib/Sema/SemaDeclAttr.cpp
index 146d9c86e0715a..9e04c9615ed642 100644
--- a/clang/lib/Sema/SemaDeclAttr.cpp
+++ b/clang/lib/Sema/SemaDeclAttr.cpp
@@ -1331,14 +1331,6 @@ static void handleNoEscapeAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
   if (D->isInvalidDecl())
     return;
 
-  // noescape only applies to pointer types.
-  QualType T = cast<ParmVarDecl>(D)->getType();
-  if (!S.isValidPointerAttrType(T, /* RefOkay */ true)) {
-    S.Diag(AL.getLoc(), diag::warn_attribute_pointers_only)
-        << AL << AL.getRange() << 0;
-    return;
-  }
-
   D->addAttr(::new (S.Context) NoEscapeAttr(S.Context, AL));
 }
 
diff --git a/clang/test/SemaCXX/noescape-attr.cpp b/clang/test/SemaCXX/noescape-attr.cpp
index 78dc4f07ffef87..a1ca875eb390a4 100644
--- a/clang/test/SemaCXX/noescape-attr.cpp
+++ b/clang/test/SemaCXX/noescape-attr.cpp
@@ -1,7 +1,12 @@
 // RUN: %clang_cc1 -fsyntax-only -verify %s
 
+// expected-no-diagnostics
+
 template<typename T>
 void test1(T __attribute__((noescape)) arr, int size);
 
-// expected-warning@+1 {{'noescape' attribute only applies to pointer arguments}}
-void test2(int __attribute__((noescape)) arr, int size);
\ No newline at end of file
+void test2(int __attribute__((noescape)) arr, int size);
+
+#if !__has_feature(attribute_noescape_nonpointer)
+  #error "attribute_noescape_nonpointer should be supported"
+#endif
diff --git a/clang/test/SemaObjCXX/noescape.mm b/clang/test/SemaObjCXX/noescape.mm
index 999a91b87300b7..1ce58064fa2fe7 100644
--- a/clang/test/SemaObjCXX/noescape.mm
+++ b/clang/test/SemaObjCXX/noescape.mm
@@ -23,11 +23,11 @@
 template <class T>
 void noescapeFunc7(__attribute__((noescape)) T &&);
 
-void invalidFunc0(int __attribute__((noescape))); // expected-warning {{'noescape' attribute only applies to pointer arguments}}
+void invalidFunc0(int __attribute__((noescape)));
 void invalidFunc1(int __attribute__((noescape(0)))); // expected-error {{'noescape' attribute takes no arguments}}
 void invalidFunc2(int0 *__attribute__((noescape))); // expected-error {{use of undeclared identifier 'int0'; did you mean 'int'?}}
-void invalidFunc3(__attribute__((noescape)) int (S::*Ty)); // expected-warning {{'noescape' attribute only applies to pointer arguments}}
-void invalidFunc4(__attribute__((noescape)) void (S::*Ty)()); // expected-warning {{'noescape' attribute only applies to pointer arguments}}
+void invalidFunc3(__attribute__((noescape)) int (S::*Ty));
+void invalidFunc4(__attribute__((noescape)) void (S::*Ty)());
 int __attribute__((noescape)) g; // expected-warning {{'noescape' attribute only applies to parameters}}
 
 struct S1 {

[erichkeane]

I see you've enabled it, but the tests don't actually show that this has any effect besides suppressing a diagnostic. I want to see something that shows that the attribute is actually DOING something on non-pointer types.

Also, requires a release note.

[erichkeane]

I don't get this macro/error here. This will never happen in clang, we support this feature.

[Xazax-hun]

This PR introduced this feature and the goal of this test to make sure feature testing works as expected. I am happy to remove the test if you find it redundant.

[erichkeane]

Got it, thanks for clarifying.

[Xazax-hun]

I want to see something that shows that the attribute is actually DOING something on non-pointer types.

I added a test to make sure the attribute is actually applied to the AST. Admittedly, there is not a lot that this can be used for as of now (other than documenting the intent that a parameter should not escape). But this is something that we want to rely on extensively in our downstream fork in Swift to support safer interop between C, C++ and Swift. I also suspect that this can be potentially useful information for generating ergonomic/safe bindings for Rust or other memory safe languages as well.

This attribute also came up in https://discourse.llvm.org/t/rfc-a-clangir-based-safe-c/83245

Let me know if this makes sense. Otherwise, we could keep this change in our downstream fork until more work is accumulated for this attribute to do something.

Also, requires a release note.

Done.

[erichkeane]

I would still like to see a CodeGen test for this to show that it modifies the LLVM-IR.

I see the attribute is referenced once in our codegen (CodeGenTypes::arrangeObjCMessageSendSignature), which looks to modify the IR, so I'd like to see a test that validates this changes with these types.

[erichkeane]

Got it, thanks for clarifying.

[Xazax-hun]

I'd like to see a test that validates this changes with these types.

Sorry, I missed the part in CodeGen. So noescape is used to emit the nocapute parameter attribute in the LLVM IR. This parameter attribute is only valid for pointers and we currently do not seem to have a way in the LLVM IR to express nocapture for a field of a struct.

I changed the PR to make sure we do not emit nocapture in the IR where we should not and added some tests validating this.

[erichkeane]

I'd like to see a test that validates this changes with these types.

Sorry, I missed the part in CodeGen. So noescape is used to emit the nocapute parameter attribute in the LLVM IR. This parameter attribute is only valid for pointers and we currently do not seem to have a way in the LLVM IR to express nocapture for a field of a struct.

I changed the PR to make sure we do not emit nocapture in the IR where we should not and added some tests validating this.

Thanks! I guess this leaves me confused as to what the PURPOSE of this patch is if it doesn't allow the attribute to get to IR? What is it that consumes this? Do we have some sort of analysis tool that works on it? If so, can we write a test that shows that behavior?

[Xazax-hun]

What is it that consumes this?

We are planning to consume this downstream in the Swift compiler for C++ interop. Swift has non-escapable types, like Swift's Span type. The interop layer cannot bridge a C++ std::span as Swift's Span without this additional information from the API's authors.

The need to annotate this property was also raised in https://discourse.llvm.org/t/rfc-a-clangir-based-safe-c/83245 which was the main motivation to make this change upstream, to avoid having multiple different spellings across the parallel efforts to interoperate between safe and unsafe code.

[AaronBallman]

We are planning to consume this downstream in the Swift compiler for C++ interop. Swift has non-escapable types, like Swift's Span type. The interop layer cannot bridge a C++ std::span as Swift's Span without this additional information from the API's authors.

Why should this live upstream if it basically only benefits a single downstream?

The need to annotate this property was also raised in https://discourse.llvm.org/t/rfc-a-clangir-based-safe-c/83245 which was the main motivation to make this change upstream, to avoid having multiple different spellings across the parallel efforts to interoperate between safe and unsafe code.

That RFC did not really get much support because CIR isn't anywhere near far enough along to start making plans for it, so it's not really a compelling reason to move forward with these changes yet.

[Xazax-hun]

Why should this live upstream if it basically only benefits a single downstream?

This is a fair question, my intuition was that if the need came up multiple times independently, it might be worth to have it upstream. But it is also fair to wait until there is an actual upstream user, or multiple downstream users. I am OK with closing this and keep it downstream until either of those happens. The only risk is downstream users diverging, which is something we can always deal with during the RFC process before something is upstreamed.

[AaronBallman]

Why should this live upstream if it basically only benefits a single downstream?

This is a fair question, my intuition was that if the need came up multiple times independently, it might be worth to have it upstream. But it is also fair to wait until there is an actual upstream user, or multiple downstream users. I am OK with closing this and keep it downstream until either of those happens. The only risk is downstream users diverging, which is something we can always deal with during the RFC process before something is upstreamed.

My (slight) preference is to keep this in the downstream until we have a need upstream (or multiple downstreams need it). WDYT @erichkeane?

[erichkeane]

Why should this live upstream if it basically only benefits a single downstream?

This is a fair question, my intuition was that if the need came up multiple times independently, it might be worth to have it upstream. But it is also fair to wait until there is an actual upstream user, or multiple downstream users. I am OK with closing this and keep it downstream until either of those happens. The only risk is downstream users diverging, which is something we can always deal with during the RFC process before something is upstreamed.

My (slight) preference is to keep this in the downstream until we have a need upstream (or multiple downstreams need it). WDYT @erichkeane?

I'm incredibly on the fence here. We DO have a clang-tidy pass that actually consumes this it looks (bugprone-no-escape), so there is potential value here. Additionally, AST matcher folks might find this change valuable.

Seeing as the REST of it is in the CFE, I can squint and see value to having it upstream.

[AaronBallman]

Why should this live upstream if it basically only benefits a single downstream?

This is a fair question, my intuition was that if the need came up multiple times independently, it might be worth to have it upstream. But it is also fair to wait until there is an actual upstream user, or multiple downstream users. I am OK with closing this and keep it downstream until either of those happens. The only risk is downstream users diverging, which is something we can always deal with during the RFC process before something is upstreamed.

My (slight) preference is to keep this in the downstream until we have a need upstream (or multiple downstreams need it). WDYT @erichkeane?

I'm incredibly on the fence here. We DO have a clang-tidy pass that actually consumes this it looks (bugprone-no-escape), so there is potential value here. Additionally, AST matcher folks might find this change valuable.

I don't see a whole lot of benefit to AST matcher folks, but perhaps I'm missing something?

CC @PiotrZSL @HerrCai0907 @5chmidti for opinions from clang-tidy folks on whether bugprone-no-escape would want to see these changes or not.

[efriedma-quic]

I'm not sure I understand the intended meaning here. Is noescape on a struct argument supposed to recursively apply to every pointer field of the struct? If that's the intended meaning, should we restrict this to structs that have pointer fields?

[Xazax-hun]

If that's the intended meaning, should we restrict this to structs that have pointer fields?

Yeah, that is the intended meaning. I am on the fence about restricting to structs with pointers though, mostly to more ergonomically support use cases like:

struct SometimesHasPtr {
#ifdef COMPILE_TIME_FEATURE_FLAG
int *ptr;
#endif
};

A diagnostic to warn on structs that have no pointers would trigger on SometimesHasPtr in some build configurations but not in others. If you think that is not a blocker, I can add a diagnostic.

[5chmidti]

whether bugprone-no-escape would want to see these changes or not.

The bugprone-no-escape check seems to be a bit limited because it only works with dispatch_async and dispatch_after (from Apple, it seems). It could probably be expanded to include some more async constructs, but adding support for other variable types looks like it would be a positive that would immediately work for the above functions. Only the diagnostic of the check would need to be adjusted a bit.