[PATCH] D156787: [analyzer][attr] Add docs to ownership_* attributes

Tue Aug 8 07:57:33 PDT 2023

aaron.ballman added inline comments.

================
Comment at: clang/include/clang/Basic/AttrDocs.td:1189
+``onwership_returns``: Functions with this annotation return dynamic memory.
+The second annotation parameter is the size of the returned memory in bytes.
+
----------------
donat.nagy wrote:
> Szelethus wrote:
> > aaron.ballman wrote:
> > > 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?
> > 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. 
> 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?)
> 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. ;-)

================
Comment at: clang/include/clang/Basic/AttrDocs.td:1189
+``onwership_returns``: Functions with this annotation return dynamic memory.
+The second annotation parameter is the size of the returned memory in bytes.
+
----------------
aaron.ballman wrote:
> donat.nagy wrote:
> > Szelethus wrote:
> > > aaron.ballman wrote:
> > > > 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?
> > > 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. 
> > 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?)
> > 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. ;-)
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.)

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

https://reviews.llvm.org/D156787