Skip to content

Conversation

@Szelethus
Copy link
Contributor

@Szelethus Szelethus commented Jan 6, 2025

Exactly what it says on the tin! These were written ages ago (2010s), but are still functional, only the docs are missing.

image

Exactly what it says on the tin! These were written ages ago (2010s),
but are still functional, only the docs are missing.
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Jan 6, 2025
@llvmbot
Copy link
Member

llvmbot commented Jan 6, 2025

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-static-analyzer-1

Author: Kristóf Umann (Szelethus)

Changes

Exactly what it says on the tin! These were written ages ago (2010s), but are still functional, only the docs are missing.

image


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

2 Files Affected:

  • (modified) clang/include/clang/Basic/Attr.td (+1-1)
  • (modified) clang/include/clang/Basic/AttrDocs.td (+62)
diff --git a/clang/include/clang/Basic/Attr.td b/clang/include/clang/Basic/Attr.td
index 90d2a2056fe1ba..0454642b52cf35 100644
--- a/clang/include/clang/Basic/Attr.td
+++ b/clang/include/clang/Basic/Attr.td
@@ -2776,7 +2776,7 @@ def Ownership : InheritableAttr {
   let Args = [IdentifierArgument<"Module">,
               VariadicParamIdxArgument<"Args">];
   let Subjects = SubjectList<[HasFunctionProto]>;
-  let Documentation = [Undocumented];
+  let Documentation = [OwnershipDocs];
 }
 
 def Packed : InheritableAttr {
diff --git a/clang/include/clang/Basic/AttrDocs.td b/clang/include/clang/Basic/AttrDocs.td
index fdad4c9a3ea191..b1b4685ae9259b 100644
--- a/clang/include/clang/Basic/AttrDocs.td
+++ b/clang/include/clang/Basic/AttrDocs.td
@@ -1389,6 +1389,68 @@ Query for this attribute with ``__has_attribute(overloadable)``.
   }];
 }
 
+def OwnershipDocs : Documentation {
+  let Heading = "ownership_holds, ownership_returns, ownership_takes (Clang "
+                "Static Analyzer)";
+  let Category = DocCatFunction;
+  let Content = [{
+
+.. note::
+
+  In order for the Clang Static Analyzer to acknowledge these attributes, the
+  ``Optimistic`` config needs to be set to true for the checker
+  ``unix.DynamicMemoryModeling``:
+
+  ``-Xclang -analyzer-config -Xclang unix.DynamicMemoryModeling:Optimistic=true``
+
+These attributes are used by the Clang Static Analyzer's dynamic memory modeling
+facilities to mark custom allocating/deallocating functions.
+
+All 3 attributes' first parameter of type string is the type of the allocation:
+``malloc``, ``new``, etc. to allow for catching mismatched deallocation bugs to
+be found.
+
+* Use ``ownership_returns`` to mark a function as an allocating function. Takes
+  1 parameter to denote the allocation type.
+* Use ``ownership_takes`` to mark a function as a deallocating function. Takes 2
+  parameters: the allocation type, and the index of the parameter that is being
+  deallocated (counting from 1).
+* Use ``ownership_holds`` to mark that a function takes over the ownership of a
+  piece of memory. The analyzer will assume that the memory is not leaked even
+  if it finds that the last pointer referencing it went out of scope (almost as
+  if it was freed). Takes 2 parameters: the allocation type, and the index of
+  the parameter whose ownership will be taken over (counting from 1).
+
+Example:
+
+.. code-block:: c
+
+  // Denotes that my_malloc will return with adynamically allocated piece of
+  // memory using malloc().
+  void __attribute((ownership_returns(malloc))) *my_malloc(size_t);
+
+  // Denotes that my_free will deallocate its parameter using free().
+  void __attribute((ownership_takes(malloc, 1))) my_free(void *);
+
+  // Denotes that my_hold will take over the ownership of its parameter that was
+  // allocated via malloc().
+  void __attribute((ownership_holds(malloc, 1))) my_hold(void *);
+
+Further reading about dynamic memory modeling in the Clang Static Analyzer is
+found in these checker docs:
+:ref:`unix.Malloc <unix-Malloc>`, :ref:`unix.MallocSizeof <unix-MallocSizeof>`,
+:ref:`unix.MismatchedDeallocator <unix-MismatchedDeallocator>`,
+:ref:`cplusplus.NewDelete <cplusplus-NewDelete>`,
+:ref:`cplusplus.NewDeleteLeaks <cplusplus-NewDeleteLeaks>`,
+:ref:`optin.taint.TaintedAlloc <optin-taint-TaintedAlloc>`.
+Mind that many more checkers are affected by dynamic memory modeling changes to
+some extent.
+
+Further reading for other annotations:
+`Source Annotations in the Clang Static Analyzer <https://clang-analyzer.llvm.org/annotations.html>`_.
+}];
+}
+
 def ObjCMethodFamilyDocs : Documentation {
   let Category = DocCatFunction;
   let Content = [{

Copy link
Contributor

@steakhal steakhal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks pretty good. Thanks for the docs. That's always a nice way to start a year.

Comment on lines 1409 to 1411
All 3 attributes' first parameter of type string is the type of the allocation:
``malloc``, ``new``, etc. to allow for catching mismatched deallocation bugs to
be found.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here I'm not exactly sure what etc refers to. Is the set of accepted values here backed in or it can be the name of any previously declared function? Or even any identifier?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, from what I understand, you can set the allocation type to something totally custom, like "lasagna" as well, and if you that for all of your custom dynamic memory functions correctly, it should catch mismatch errors (you deallocated memory with "cheese", but it was allocated with "lasagna"). There is a single exception for "malloc" (and should be for new, new[], etc), MallocChecker will be sure that functions with "malloc" annotations will be treated the same as the standard "malloc" and "free".

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This wasn't clear to me from the proposed description. I think it deserves to be noted.

Copy link
Contributor

@NagyDonat NagyDonat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for adding this documentation!

I added an inline suggestion that -- hopefully -- clarifies the differences between held and taken memory areas; but the original is also completely OK. Feel free to ignore or tweak my suggestion if you prefer.

Co-authored-by: Donát Nagy <[email protected]>
Co-authored-by: Balazs Benics <[email protected]>
@steakhal
Copy link
Contributor

steakhal commented Jan 6, 2025

Copy link
Contributor

@steakhal steakhal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks truly awesome

@steakhal
Copy link
Contributor

steakhal commented Jan 6, 2025

Could you please check the generated html prior merging this, just to be on the safe side it looks as intended.

Co-authored-by: Balazs Benics <[email protected]>
Co-authored-by: isuckatcs <[email protected]>
@Szelethus Szelethus merged commit 7ce15f3 into llvm:main Jan 7, 2025
6 of 8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

clang:frontend Language frontend issues, e.g. anything involving "Sema" clang:static analyzer clang Clang issues not falling into any other category documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants