[Clang] Add support for attribute 'escape'

[guitard0g]

Summary

The 'escape' attribute indicates that the annotated pointer parameter
may escape the scope of the function. This attribute is meant to be
used for compiler diagnostics/static analysis checks. The attribute
will first be used by the Swift compiler in a new implicit bridging
diagnostic, but may have other non-Swift use-cases for diagnostics.

[NoQ]

may have other non-Swift use-cases for diagnostics.

I'm looking forward to taking advantage of this attribute in the static analyzer's StackAddrEscapeChecker!

clang/include/clang/Basic/Attr.td

1952

Shouldn't both this attribute and the one above be InheritableAttr?

Comments in this file say:

/// An inheritable attribute is inherited by later redeclarations.

Which is arguably exactly what we want? Like, it should be sufficient to put the attribute into the header, there's no need to duplicate it in the implementation?

clang/include/clang/Basic/AttrDocs.td

260

Are you sure you want "can escape" instead of "will escape"? In the former case it's impossible to implement the warning without false positives ("well, it says it may escape, but I know for sure that if I pass these other arguments then it won't escape"). In the latter case, of course, a lot of places where the value escapes conditionally won't be able to wear the attribute. Do I understand correctly that you want to proceed with the more aggressive diagnostic?

[guitard0g]

clang/include/clang/Basic/AttrDocs.td

260

Hmm that's an interesting question. Initially my thought was that any possibility of escaping should eliminate the option of passing in a temporary pointer, which I think is reasonable in Swift's implicit bridging use-case. In terms of the API contract, I was thinking that any API author who annotates their function parameter with 'escaping' is effectively saying you shouldn't pass in a pointer that you're not comfortable escaping. I think it may be a little restrictive to say that the pointer will always escape for certain, but I do think that anybody using a function that takes an escaping parameter should treat this as if it were always escaping (if that makes sense). I suppose my question then would be, do you think it's better to say "will escape" if that's what the client calling into an API should expect (even though the pointer might not always escape in the implementation)?

[guitard0g]

Change Escape/NoEscape to use InheritableAttr and update an attribute test to fix test failure.

[guitard0g]

clang/include/clang/Basic/Attr.td

1952

Oh! Good point, I totally agree.

[vsavchenko]

Great job! It looks good, but I have a couple of minor tweaks to suggest.

I see that the "applies to pointer arguments only" warning is not tested for noescape, but I still find it to be a good practice to write a test with a bunch of cases with attributes applied in wrong places.

Additionally, I believe that escape and noescape convey the same information, and it is totally okay to have two attributes because the user has to have a choice to say either "I do escape it" or "I definitely do not escape it" when the default is unspecified at all. But I do think that we should add a check disallowing both of these attributes to be used at the same time for the same parameter, since they directly contradict one another.

[guitard0g]

Add tests for diagnostics of incorrect usage. Diagnose when escape and noescape are used on the same parameter.

[guitard0g]

@vsavchenko

I see that the "applies to pointer arguments only" warning is not tested for noescape, but I still find it to be a good practice to write a test with a bunch of cases with attributes applied in wrong places.

Updated with some tests for this!

Additionally, I believe that escape and noescape convey the same information, and it is totally okay to have two attributes because the user has to have a choice to say either "I do escape it" or "I definitely do not escape it" when the default is unspecified at all. But I do think that we should add a check disallowing both of these attributes to be used at the same time for the same parameter, since they directly contradict one another.

Ah that's a great point, most recent update makes these two mutually exclusive.

[vsavchenko]

Awesome, I have nothing to add at this point!
Let's still wait for @aaron.ballman to check it.

[aaron.ballman]

The attribute will first be used by the Swift compiler in a new implicit bridging diagnostic, but may have other non-Swift use-cases for diagnostics.

This means there's no use of this attribute within Clang which suggests to me that this attribute belongs in the Swift compiler until there's a reason to upstream it. Or are you planning on using this attribute within Clang in the Very Near Future?

clang/include/clang/Basic/Attr.td

1957

You should define these attributes as being mutually exclusive; e.g.,

def : MutualExclusions<[Escape, NoEscape]>;

clang/include/clang/Basic/AttrDocs.td

260

`escape` placed on a function parameter of a pointer type is used to indicate that the pointer can escape the function.

FWIW, this reads a bit strangely to me because that's already the case without the attribute. Anything that's not marked *can* escape. I think "will escape" more clearly identifies to the person reading the function signature that the parameter should be treated as though it will escape. If it does not escape, that's not necessarily an issue because that shouldn't change how the user treats the argument (for example, it may not escape today but it may start to escape in the future).

One thing the docs are missing is an explanation of what benefit you get from using this annotation. Does this improve optimizations? Enable better checking somewhere? That sort of thing (with examples) would be good to add.

clang/lib/Sema/SemaDeclAttr.cpp

1568

This should be handled directly from Attr.td, I'll put some comments in there.

clang/test/SemaObjCXX/escape.mm

1–2

I don't see any use of blocks, so I don't think you need -fblocks. I think you can also remove the C++11 RUN line as it doesn't really add any test coverage.

24

Can you also add test coverage for the more likely case of inheriting mutually exclusive attributes? e.g.,

void foo() __attribute__((escape));
void foo() __attribute__((noescape));

void bar() __attribute__((noescape));
void bar() __attribute__((escape));

[NoQ]

clang/include/clang/Basic/AttrDocs.td

260

If I understand correctly, the new attribute proclaims "Do not, under any circumstances, pass stack pointers here". Like, it doesn't necessarily escape every time, but you should expect the function to be able to stash that pointer somewhere without telling you, for indefinite amount of time, so if you put your temporaries into this function you're 100% doing something wrong and you want to be notified about this.

This take probably raises more questions though. Like, if the pointer is a heap pointer but we see a nearby delete of that pointer with our own eyes, do we warn too? Where exactly is the boundary between short-lived and long-lived objects? (IIUC the answer is yes, we do want to warn, because the function stashes the pointer for *indefinite* amount of time so it should be the judge for when to delete it, not you).

I think there could be a simple compiler warning associated with that attribute which would pattern-match the argument to see if it's an immediate stack address such as &Class() or &Var where Var is a local variable. A static analyzer check for the more sophisticated cases works too of course.

clang/include/clang/Basic/Attr.td

def ObjCInertUnsafeUnretained : TypeAttr {
   let Documentation = [Undocumented];
 }
 
 def ObjCKindOf : TypeAttr {
   let Spellings = [Keyword<"__kindof">];
   let Documentation = [Undocumented];
 }
 
-def NoEscape : Attr {
+def NoEscape : InheritableAttr {
   let Spellings = [Clang<"noescape">];
   let Subjects = SubjectList<[ParmVar]>;
   let Documentation = [NoEscapeDocs];
 }
 
+def Escape : InheritableAttr {
+  let Spellings = [Clang<"escape">];
+  let Subjects = SubjectList<[ParmVar]>;
+  let Documentation = [EscapeDocs];
+}
+
 def AssumeAligned : InheritableAttr {
   let Spellings = [GCC<"assume_aligned">];
   let Subjects = SubjectList<[ObjCMethod, Function]>;
   let Args = [ExprArgument<"Alignment">, ExprArgument<"Offset", 1>];
   let Documentation = [AssumeAlignedDocs];
 }
 
 def AllocAlign : InheritableAttr {

clang/include/clang/Basic/AttrDocs.td

.. code-block:: c
   void escapingFunc(__attribute__((noescape)) BlockTy block) {
     g0 = block; // Not OK.
     g1 = Block_copy(block); // Not OK either.
   }
 
   }];
 }
 
+def EscapeDocs : Documentation {
+  let Category = DocCatVariable;
+  let Content = [{
+``escape`` placed on a function parameter of a pointer type is used to indicate
+that the pointer can escape the function. This means that a reference to the object
+the pointer points to that is derived from the parameter value may survive
+after the function returns.
+
+For example:
+
+.. code-block:: c
+
+  int *gp;
+
+  void escapingFunc(__attribute__((escape)) int *p) {
+    gp = p;
+  }
+
+  }];
+}
+
 def CarriesDependencyDocs : Documentation {
   let Category = DocCatFunction;
   let Content = [{
 The ``carries_dependency`` attribute specifies dependency propagation into and
 out of functions.
 
 When specified on a function or Objective-C method, the ``carries_dependency``
 attribute means that the return value carries a dependency out of the function,

clang/lib/Sema/SemaDeclAttr.cpp

static void handleReturnsNonNullAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
   SourceRange SR = getFunctionOrMethodResultSourceRange(D);
   if (!attrNonNullArgCheck(S, ResultType, AL, SourceRange(), SR,
                            /* isReturnValue */ true))
     return;
 
   D->addAttr(::new (S.Context) ReturnsNonNullAttr(S.Context, AL));
 }
 
-static void handleNoEscapeAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
+static void handleXEscapeAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
   if (D->isInvalidDecl())
     return;
 
-  // noescape only applies to pointer types.
+  // escape/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));
+  switch (AL.getKind()) {
+  default:
+    llvm_unreachable("invalid escape attribute");
+  case ParsedAttr::AT_NoEscape:
+    // escape/noescape are mutually exclusive attributes.
+    if (!checkAttrMutualExclusion<EscapeAttr>(S, D, AL))
+      handleSimpleAttribute<NoEscapeAttr>(S, D, AL);
+    return;
+  case ParsedAttr::AT_Escape:
+    if (!checkAttrMutualExclusion<NoEscapeAttr>(S, D, AL))
+      handleSimpleAttribute<EscapeAttr>(S, D, AL);
+    return;
+  }
 }
 
 static void handleAssumeAlignedAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
   Expr *E = AL.getArgAsExpr(0),
        *OE = AL.getNumArgs() > 1 ? AL.getArgAsExpr(1) : nullptr;
   S.AddAssumeAlignedAttr(D, AL, E, OE);
 }
 
case ParsedAttr::AT_NonNull:
     if (auto *PVD = dyn_cast<ParmVarDecl>(D))
       handleNonNullAttrParameter(S, PVD, AL);
     else
       handleNonNullAttr(S, D, AL);
     break;
   case ParsedAttr::AT_ReturnsNonNull:
     handleReturnsNonNullAttr(S, D, AL);
     break;
+  case ParsedAttr::AT_Escape:
   case ParsedAttr::AT_NoEscape:
-    handleNoEscapeAttr(S, D, AL);
+    handleXEscapeAttr(S, D, AL);
     break;
   case ParsedAttr::AT_AssumeAligned:
     handleAssumeAlignedAttr(S, D, AL);
     break;
   case ParsedAttr::AT_AllocAlign:
     handleAllocAlignAttr(S, D, AL);
     break;
   case ParsedAttr::AT_Ownership:

clang/test/AST/ast-dump-attr.cpp

 namespace TestNoEscape {
   void noescapeFunc(int *p0, __attribute__((noescape)) int *p1) {}
   // CHECK: `-FunctionDecl{{.*}} noescapeFunc 'void (int *, __attribute__((noescape)) int *)'
   // CHECK-NEXT: ParmVarDecl
   // CHECK-NEXT: ParmVarDecl
   // CHECK-NEXT: NoEscapeAttr
 }
 
+namespace TestEscape {
+  void escapeFunc(int *p0, __attribute__((escape)) int *p1) {}
+  // CHECK: NamespaceDecl{{.*}} TestEscape
+  // CHECK-NEXT: `-FunctionDecl{{.*}} escapeFunc 'void (int *, int *)'
+  // CHECK-NEXT: ParmVarDecl
+  // CHECK-NEXT: ParmVarDecl
+  // CHECK-NEXT: EscapeAttr
+}
+
 namespace TestSuppress {
   [[gsl::suppress("at-namespace")]];
   // CHECK: NamespaceDecl{{.*}} TestSuppress
   // CHECK-NEXT: EmptyDecl{{.*}}
   // CHECK-NEXT: SuppressAttr{{.*}} at-namespace
   [[gsl::suppress("on-decl")]]
   void TestSuppressFunction();
   // CHECK: FunctionDecl{{.*}} TestSuppressFunction

clang/test/AST/ast-dump-attr.m

 // CHECK-NEXT: |-ParmVarDecl{{.*}} Test11 'int'
 // CHECK-NEXT: `-NSConsumesSelfAttr
 -(void)Test12: (int *) [[clang::noescape]] Test13  to:(int)Test14 [[clang::ns_consumes_self]];
 // CHECK: ObjCMethodDecl{{.*}} Test12:to: 'void'
 // CHECK-NEXT: |-ParmVarDecl{{.*}} Test13 'int *'
 // CHECK-NEXT: | `-NoEscapeAttr
 // CHECK-NEXT: |-ParmVarDecl{{.*}} Test14 'int'
 // CHECK-NEXT: `-NSConsumesSelfAttr
+-(void)Test15: (int *) [[clang::escape]] Test16;
+// CHECK: ObjCMethodDecl{{.*}} Test15: 'void'
+// CHECK-NEXT: -ParmVarDecl{{.*}} Test16 'int *'
+// CHECK-NEXT: `-EscapeAttr
 @end

clang/test/Misc/pragma-attribute-supported-attributes-list.test

 // CHECK-NEXT: DLLExport (SubjectMatchRule_function, SubjectMatchRule_variable, SubjectMatchRule_record, SubjectMatchRule_objc_interface)
 // CHECK-NEXT: DLLImport (SubjectMatchRule_function, SubjectMatchRule_variable, SubjectMatchRule_record, SubjectMatchRule_objc_interface)
 // CHECK-NEXT: Destructor (SubjectMatchRule_function)
 // CHECK-NEXT: DisableTailCalls (SubjectMatchRule_function, SubjectMatchRule_objc_method)
 // CHECK-NEXT: EnableIf (SubjectMatchRule_function)
 // CHECK-NEXT: EnforceTCB (SubjectMatchRule_function)
 // CHECK-NEXT: EnforceTCBLeaf (SubjectMatchRule_function)
 // CHECK-NEXT: EnumExtensibility (SubjectMatchRule_enum)
+// CHECK-NEXT: Escape (SubjectMatchRule_variable_is_parameter)
 // CHECK-NEXT: ExcludeFromExplicitInstantiation (SubjectMatchRule_variable, SubjectMatchRule_function, SubjectMatchRule_record)
 // CHECK-NEXT: ExternalSourceSymbol ((SubjectMatchRule_record, SubjectMatchRule_enum, SubjectMatchRule_enum_constant, SubjectMatchRule_field, SubjectMatchRule_function, SubjectMatchRule_namespace, SubjectMatchRule_objc_category, SubjectMatchRule_objc_implementation, SubjectMatchRule_objc_interface, SubjectMatchRule_objc_method, SubjectMatchRule_objc_property, SubjectMatchRule_objc_protocol, SubjectMatchRule_record, SubjectMatchRule_type_alias, SubjectMatchRule_variable))
 // CHECK-NEXT: FlagEnum (SubjectMatchRule_enum)
 // CHECK-NEXT: Flatten (SubjectMatchRule_function)
 // CHECK-NEXT: GNUInline (SubjectMatchRule_function)
 // CHECK-NEXT: HIPManaged (SubjectMatchRule_variable)
 // CHECK-NEXT: Hot (SubjectMatchRule_function)
 // CHECK-NEXT: IBAction (SubjectMatchRule_objc_method_is_instance)

clang/test/SemaObjCXX/escape.mm

+// RUN: %clang_cc1 -fsyntax-only -verify -fblocks -std=c++11 %s
+// RUN: %clang_cc1 -fsyntax-only -verify -fblocks -std=c++1z %s
+
+struct S {};
+
+template <class T>
+void escapeFunc0(__attribute__((noescape)) T); // expected-warning {{'noescape' attribute only applies to pointer arguments}}
+template <class T>
+void escapeFunc1(__attribute__((escape)) const T &);
+template <class T>
+void escapeFunc2(__attribute__((escape)) T &&);
+
+void escapeFunc3(__attribute__((escape)) int *);
+void escapeFunc4(__attribute__((escape)) void *);
+void escapeFunc5(__attribute__((escape)) int **);
+
+void invalidFunc0(int __attribute__((escape))); // expected-warning {{'escape' attribute only applies to pointer arguments}}
+void invalidFunc1(int __attribute__((escape(0)))); // expected-error {{'escape' attribute takes no arguments}}
+void invalidFunc2(__attribute__((escape)) int (S::*Ty)); // expected-warning {{'escape' attribute only applies to pointer arguments}}
+void invalidFunc3(__attribute__((escape)) void (S::*Ty)()); // expected-warning {{'escape' attribute only applies to pointer arguments}}
+
+void invalidFunc4(__attribute__((escape)) __attribute__((noescape)) int *); // expected-error {{'noescape' and 'escape' attributes are not compatible}} \
+// expected-note {{conflicting attribute is here}}
+void invalidFunc5(__attribute__((noescape)) __attribute__((escape)) int *); // expected-error {{'escape' and 'noescape' attributes are not compatible}} \
+// expected-note {{conflicting attribute is here}}
+
+int __attribute__((escape)) g; // expected-warning {{'escape' attribute only applies to parameters}}
+int * __attribute__((escape)) h; // expected-warning {{'escape' attribute only applies to parameters}}