[analyzer][attr] Add docs to ownership_* attributes

[Szelethus]

Summary

Pretty much what it says on the tin! I worked a bunch on this checker so I had some vague memory (hehe) what these annotations mean, and also found this thread in the mailing list:
https://discourse.llvm.org/t/ownership-attribute-for-malloc-etc-checking/16260

[aaron.ballman]

Thank you for adding documentation for these attributes, it's greatly appreciated!

clang/include/clang/Basic/AttrDocs.td

1171–1175

1177–1180

1189

I'm a bit confused on this last bit; how is the user supposed to statically know that value? I read this as claiming my_malloc returns 1 byte of dynamic storage; can the user tie the allocation to a parameter value instead?

1192–1193

We should document whether this is a 1-based index or a 0-based index, and we should also explain how the index works for member functions where there may be an implicit this argument involved.

[steakhal]

I haven't looked at the content but I have the feeling that we should probably backport this to release/17.x

[steakhal]

clang/test/Analysis/malloc-annotations.c

151–155

Please, consider elaborating on this test with some comments, as it's not clear to me at first glance - without reading anything about the attributes - what's going on here.

[Szelethus]

Fixes according to reviewer comments.

[Szelethus]

clang/include/clang/Basic/AttrDocs.td

1189

I share your confusion, I guess. What I've written here is my best understanding of how this works, unfortunately, these annotations were made in ~2010 (when I was still in elementary school) and abandoned since.

I read this as claiming my_malloc returns 1 byte of dynamic storage;

Thats what the corresponding code in the Static Analyzer leads me to believe as well.

can the user tie the allocation to a parameter value instead?

No.

[donat.nagy]

clang/include/clang/Basic/AttrDocs.td

1189

I'd guess that this annotation was designed for factory functions that construct an instance of a concrete struct type on the heap. Allowing "generic" malloc variants (where the allocation size is specified by a parameter) would be a good additional feature, but I think that the current annotation also has its own uses.

Perhaps it would be good to provide examples like

void __attribute((ownership_takes(malloc, 1))) free_widget(struct widget *);
struct widget *__attribute((ownership_returns(malloc, sizeof(struct widget)))) create_widget(void);
void __attribute((ownership_holds(malloc, 1))) hold_widget(struct widget *);

that correspond to this use case. (By the way, does the checker handle non-void* pointers?)

[aaron.ballman]

clang/include/clang/Basic/AttrDocs.td

1189

I share your confusion, I guess. What I've written here is my best understanding of how this works, unfortunately, these annotations were made in ~2010 (when I was still in elementary school) and abandoned since.

This comment came on the same week when I had someone much younger than me ask me what Tetris was. I'll go shake my cane at some passing clouds, now. ;-)

1189

That code doesn't compile because the argument is somehow "out of bounds": https://godbolt.org/z/3e4TbhcT6

I did some digging and the second argument in all three cases is a parameter index, at least according to the attribute handling code: https://github.com/llvm/llvm-project/blob/895c4ac33fc8b21bd69b017d1cecf874ca47e178/clang/lib/Sema/SemaDeclAttr.cpp#L1857

The static analyzer uses that information to determine which expression specifies the size: https://github.com/llvm/llvm-project/blob/895c4ac33fc8b21bd69b017d1cecf874ca47e178/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp#L1693

So I think the way to document this is that the second argument to ownership_returns specifies the parameter index which will specify the size of the the allocation. e.g.,

__attribute__((ownership_returns(malloc, 2))) void *func(unsigned val, unsigned size);

this doesn't return 2 bytes of allocated storage, it's saying the second parameter specifies the size of what's returned, in bytes.

(Someone should double-check my logic against how the static analyzer actually behaves just to be sure I'm right about this.)

clang/include/clang/Basic/Attr.td

OwnershipKind getOwnKind() const {
       return isHolds() ? Holds :
              isTakes() ? Takes :
              Returns;
     }
   }];
   let Args = [IdentifierArgument<"Module">,
               VariadicParamIdxArgument<"Args">];
   let Subjects = SubjectList<[HasFunctionProto]>;
-  let Documentation = [Undocumented];
+  let Documentation = [OwnershipDocs];
 }
 
 def Packed : InheritableAttr {
   let Spellings = [GCC<"packed">];
 //  let Subjects = [Tag, Field];
   let Documentation = [Undocumented];
 }
 

clang/include/clang/Basic/AttrDocs.td

 
 Support for unmarked overloads is not present in some versions of clang. You may
 query for it using ``__has_extension(overloadable_unmarked)``.
 
 Query for this attribute with ``__has_attribute(overloadable)``.
   }];
 }
 
+def OwnershipDocs : Documentation {
+  let Category = DocCatFunction;
+  let Heading = "ownership_holds, ownership_returns, ownership_takes";
+  let Content = [{
+These attributes help the static analyzer understand custom ownership management
+functions. None of these affect the generated binary in any way, they are only
+meant to assist the Clang Static Analyzer. You can use these annotations if your
+code uses wrapper functions around ``malloc`` or ``free``, or uses functions
+that take over ownership altogether.
+
+Each annotation has two parameters: the first is the type of allocator used to
+allocate the memory (the only accepted value currently is ``malloc``, but
+we intend to recognize ``new`` in the future). We use this to check whether the
+correct deallocator is used. The second is an integer value, described below.
+
+.. code-block:: c
+
+  void __attribute((ownership_takes(malloc, 1))) my_free(void *);
+  void *__attribute((ownership_returns(malloc, 1))) my_malloc(size_t);
+  void __attribute((ownership_holds(malloc, 1))) my_hold(void *);
+
+``onwership_returns``: Functions with this annotation return dynamic memory.
+The second annotation parameter is the size of the returned memory in bytes.
+
+``ownership_takes``: Functions with this annotation deallocate one of the
+function parameters. The second annotation parameter is the index of the
+function parameter to deallocate.
+
+``ownership_holds``: Functions with this annotation take over the ownership of
+one of the parameters. The static analyzer doesn't assume it to be deallocated
+immediately, but assumes that it will be before the end of the program. The
+second annotation parameter is the index of the function parameter to take hold
+of. Indexing starts at 1, so the first parameter's index is 1. The implicit this
+parameter is not supported at this time.
+
+.. code-block:: c
+
+  void foobar() {
+    int *p = my_malloc(sizeof(int));
+  } // warn: Memory leak
+
+  void foo() {
+    int *p = my_malloc(sizeof(int));
+    my_free(p);
+    free(p); // warn: Attempt to free released memory
+  }
+
+  void bar() {
+    int *p = my_malloc(sizeof(int));
+    my_hold(p);
+    *p = 4; // no warn: Pointer is not released
+  } // no warn: Ownership is transferred and assumed to have not escaped
+  }];
+}
+
 def ObjCMethodFamilyDocs : Documentation {
   let Category = DocCatFunction;
   let Content = [{
 Many methods in Objective-C have conventional meanings determined by their
 selectors. It is sometimes useful to be able to mark a method as having a
 particular conventional meaning despite not having the right selector, or as
 not having the conventional meaning that its selector would suggest. For these
 use cases, we provide an attribute to specifically describe the "method family"

clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp

 
 ProgramStateRef MallocChecker::FreeMemAttr(CheckerContext &C,
                                            const CallEvent &Call,
                                            const OwnershipAttr *Att,
                                            ProgramStateRef State) const {
   if (!State)
     return nullptr;
 
+  // TODO: Check the attribute docs in AttrDocs.td, it states that only malloc
+  // is accepted. Please adjust if we start supporting "new".
   if (Att->getModule()->getName() != "malloc")
     return nullptr;
 
   bool IsKnownToBeAllocated = false;
 
   for (const auto &Arg : Att->args()) {
     ProgramStateRef StateI =
         FreeMemAux(C, Call, State, Arg.getASTIndex(),

clang/test/Analysis/malloc-annotations.c

 
 // This case is (possibly) ok, be conservative
 int * af5(void) {
   int *p = my_malloc(12);
   my_hold(p);
   return p; // no-warning
 }
 
+// Test ownership_holds -- we expect the ownership to be taken over, and a
+// result assume that the memory will not leak, but also not released yet.
+void af6(void) {
+  int *p = my_malloc(12);
+  my_hold(p);
+  *p = 4; // no-warn: memory is not released yet
+} // no-warn: assume my_hold takes care of releasing the memory
+
 
 
 // This case tests that storing malloc'ed memory to a static variable which is
 // then returned is not leaked.  In the absence of known contracts for functions
 // or inter-procedural analysis, this is a conservative answer.
 int *f3(void) {
   static int *p = 0;
   p = malloc(12);