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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
