[NFC][analyzer] Rewrite comment header of MallocChecker #165443
Conversation
This commit rewrites the big comment block at the beginning of MallocChecker because it contained lots of obsolete and inaccurate information. I see that this block is a bit verbose (especially compared to the analogous comments in other checkers), but as this is one of the most complex checker families, I think it's useful to give this overview at the beginning of the source file.
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: Donát Nagy (NagyDonat) ChangesThis commit rewrites the big comment block at the beginning of MallocChecker because it contained lots of obsolete and inaccurate information. I see that this block is a bit verbose (especially compared to the analogous comments in other checkers), but as this is one of the most complex checker families, I think it's useful to give this overview at the beginning of the source file. Full diff: https://github.com/llvm/llvm-project/pull/165443.diff 1 Files Affected:
diff --git a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
index 70baab54df563..ec7ef237b7c31 100644
--- a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -6,41 +6,45 @@
//
//===----------------------------------------------------------------------===//
//
-// This file defines a variety of memory management related checkers, such as
+// This file defines checkers that report memory management errors such as
// leak, double free, and use-after-free.
//
-// The following checkers are defined here:
+// The logic for modeling memory allocations is implemented in the checker
+// family which is called 'MallocChecker' for historical reasons. (This name is
+// inaccurate, something like 'DynamicMemory' would be more precise.)
//
-// * MallocChecker
-// Despite its name, it models all sorts of memory allocations and
-// de- or reallocation, including but not limited to malloc, free,
-// relloc, new, delete. It also reports on a variety of memory misuse
-// errors.
-// Many other checkers interact very closely with this checker, in fact,
-// most are merely options to this one. Other checkers may register
-// MallocChecker, but do not enable MallocChecker's reports (more details
-// to follow around its field, ChecksEnabled).
-// It also has a boolean "Optimistic" checker option, which if set to true
-// will cause the checker to model user defined memory management related
-// functions annotated via the attribute ownership_takes, ownership_holds
-// and ownership_returns.
+// The reports produced by this backend are exposed through several frontends:
+// * MallocChecker: reports all misuse of dynamic memory allocated by
+// malloc, related functions (like calloc, realloc etc.) and the functions
+// annotated by ownership_returns. (Here the name "MallocChecker" is
+// reasonably accurate; don't confuse this checker frontend with the whole
+// misnamed family.)
+// * NewDeleteChecker: reports most misuse (anything but memory leaks) of
+// memory managed by the C++ operators new and new[].
+// * NewDeleteLeaksChecker: reports leaks of dynamic memory allocated by
+// the C++ operators new and new[].
+// * MismatchedDeallocatorChecker: reports situations where the allocation
+// and deallocation is mismatched, e.g. memory allocated via malloc is
+// passed to operator delete.
+// * InnerPointerChecker: reports use of pointers to the internal buffer of
+// a std::string instance after operations that invalidate them.
+// * TaintedAllocChecker: reports situations where the size argument of a
+// memory allocation function or array new operator is tainted (i.e. comes
+// from an untrusted source and can be controlled by an attacker).
//
-// * NewDeleteChecker
-// Enables the modeling of new, new[], delete, delete[] in MallocChecker,
-// and checks for related double-free and use-after-free errors.
+// In addition to these frontends this file also defines the registration
+// functions for "unix.DynamicMemoryModeling". This registers the callbacks of
+// the checker family MallocChecker without enabling any of the frontends and
+// and handle two checker options which are attached to this "modeling
+// checker" because they affect multiple checker frontends.
//
-// * NewDeleteLeaksChecker
-// Checks for leaks related to new, new[], delete, delete[].
-// Depends on NewDeleteChecker.
-//
-// * MismatchedDeallocatorChecker
-// Enables checking whether memory is deallocated with the corresponding
-// allocation function in MallocChecker, such as malloc() allocated
-// regions are only freed by free(), new by delete, new[] by delete[].
-//
-// InnerPointerChecker interacts very closely with MallocChecker, but unlike
-// the above checkers, it has it's own file, hence the many InnerPointerChecker
-// related headers and non-static functions.
+// Note that what the users see as the checker "cplusplus.InnerPointer" is a
+// combination of the frontend InnerPointerChecker (within this family) which
+// emits the bug reports and a separate checker class (also named
+// InnerPointerChecker) which is defined in InnerPointerChecker.cpp and does a
+// significant part of the modeling. This cooperation is enabled by several
+// non-static helper functions that are defined within this translation unit
+// and used in InnerPointerChecker.cpp.
//
//===----------------------------------------------------------------------===//
|
|
@llvm/pr-subscribers-clang-static-analyzer-1 Author: Donát Nagy (NagyDonat) ChangesThis commit rewrites the big comment block at the beginning of MallocChecker because it contained lots of obsolete and inaccurate information. I see that this block is a bit verbose (especially compared to the analogous comments in other checkers), but as this is one of the most complex checker families, I think it's useful to give this overview at the beginning of the source file. Full diff: https://github.com/llvm/llvm-project/pull/165443.diff 1 Files Affected:
diff --git a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
index 70baab54df563..ec7ef237b7c31 100644
--- a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -6,41 +6,45 @@
//
//===----------------------------------------------------------------------===//
//
-// This file defines a variety of memory management related checkers, such as
+// This file defines checkers that report memory management errors such as
// leak, double free, and use-after-free.
//
-// The following checkers are defined here:
+// The logic for modeling memory allocations is implemented in the checker
+// family which is called 'MallocChecker' for historical reasons. (This name is
+// inaccurate, something like 'DynamicMemory' would be more precise.)
//
-// * MallocChecker
-// Despite its name, it models all sorts of memory allocations and
-// de- or reallocation, including but not limited to malloc, free,
-// relloc, new, delete. It also reports on a variety of memory misuse
-// errors.
-// Many other checkers interact very closely with this checker, in fact,
-// most are merely options to this one. Other checkers may register
-// MallocChecker, but do not enable MallocChecker's reports (more details
-// to follow around its field, ChecksEnabled).
-// It also has a boolean "Optimistic" checker option, which if set to true
-// will cause the checker to model user defined memory management related
-// functions annotated via the attribute ownership_takes, ownership_holds
-// and ownership_returns.
+// The reports produced by this backend are exposed through several frontends:
+// * MallocChecker: reports all misuse of dynamic memory allocated by
+// malloc, related functions (like calloc, realloc etc.) and the functions
+// annotated by ownership_returns. (Here the name "MallocChecker" is
+// reasonably accurate; don't confuse this checker frontend with the whole
+// misnamed family.)
+// * NewDeleteChecker: reports most misuse (anything but memory leaks) of
+// memory managed by the C++ operators new and new[].
+// * NewDeleteLeaksChecker: reports leaks of dynamic memory allocated by
+// the C++ operators new and new[].
+// * MismatchedDeallocatorChecker: reports situations where the allocation
+// and deallocation is mismatched, e.g. memory allocated via malloc is
+// passed to operator delete.
+// * InnerPointerChecker: reports use of pointers to the internal buffer of
+// a std::string instance after operations that invalidate them.
+// * TaintedAllocChecker: reports situations where the size argument of a
+// memory allocation function or array new operator is tainted (i.e. comes
+// from an untrusted source and can be controlled by an attacker).
//
-// * NewDeleteChecker
-// Enables the modeling of new, new[], delete, delete[] in MallocChecker,
-// and checks for related double-free and use-after-free errors.
+// In addition to these frontends this file also defines the registration
+// functions for "unix.DynamicMemoryModeling". This registers the callbacks of
+// the checker family MallocChecker without enabling any of the frontends and
+// and handle two checker options which are attached to this "modeling
+// checker" because they affect multiple checker frontends.
//
-// * NewDeleteLeaksChecker
-// Checks for leaks related to new, new[], delete, delete[].
-// Depends on NewDeleteChecker.
-//
-// * MismatchedDeallocatorChecker
-// Enables checking whether memory is deallocated with the corresponding
-// allocation function in MallocChecker, such as malloc() allocated
-// regions are only freed by free(), new by delete, new[] by delete[].
-//
-// InnerPointerChecker interacts very closely with MallocChecker, but unlike
-// the above checkers, it has it's own file, hence the many InnerPointerChecker
-// related headers and non-static functions.
+// Note that what the users see as the checker "cplusplus.InnerPointer" is a
+// combination of the frontend InnerPointerChecker (within this family) which
+// emits the bug reports and a separate checker class (also named
+// InnerPointerChecker) which is defined in InnerPointerChecker.cpp and does a
+// significant part of the modeling. This cooperation is enabled by several
+// non-static helper functions that are defined within this translation unit
+// and used in InnerPointerChecker.cpp.
//
//===----------------------------------------------------------------------===//
|
|
Reviewing this PR is low priority: it only modifies a comment and I'm reasonably confident that the new text is more accurate than the old one, so I'm planning to merge this within a few days if nobody objects. (But feel free to offer suggestions or clarifications if you have time to look at this.) |
There was a problem hiding this comment.
I had a quick look at the comment and it looks nice. I haven't checked the content wrt to the checker implementation though.
To me the most interesting point it this comment of the PR summary:
I see that this block is a bit verbose (especially compared to the analogous comments in other checkers), but as this is one of the most complex checker families, I think it's useful to give this overview at the beginning of the source file.
I'm all for comments like this. This is the right place to document the design and the overall architecture. Potentially also including some ascii-art if applicable.
You probably know that IMO a comment inline in code should be brief, preferably a single line or two, and under very exceptional cases more. It is because it taxes every time someone wants to just read the code. The comment there would take up precious lines and make you scroll through. In contrast to this a comment at the top of the file has plenty of space, and a newcomer could (and should) opt in reading these. And as always, comments should be kept up to date to the extent possible.
@steakhal Thanks for clarifying this! I included that reasoning in the PR summary because after our last discussion about long comments (which were in the middle of the code and turned out to be mostly unnecessary – I was able to shorten them) I felt that you would perhaps oppose this other long comment as well.
It seems that I have unusually high natural tolerance for comment blocks that take up lines and break the flow of the code, but I'll pay attention to this issue when I write comments. (To me, even a very large comment block is much less issue than a layer of indirection where I need to "Jump to definition" to see what happens behind that ambiguous function name.) |
3 participants
This commit rewrites the big comment block at the beginning of MallocChecker because it contained lots of obsolete and inaccurate information. I see that this block is a bit verbose (especially compared to the analogous comments in other checkers), but as this is one of the most complex checker families, I think it's useful to give this overview at the beginning of the source file.