[PATCH] D52390: [analyzer] Add StackSizeChecker to StaticAnalyzer

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

One final nit apart from a few in-line comments (most of them are just arguing with @Szelethus anyways 😛): KB vs. KiB <https://en.wikipedia.org/wiki/Kibibyte> (remember how a 2 TB hard drive appears as 1.8 TB on your machine? Because it's TiB!) is one of my pet peeves especially that Windows and most people //still// get it wrong nowadays (and they also teach it wrong). Can we change the default from `100 000` to `102 400`? That would make it truly `100 KiB` in the proper sense.



================
Comment at: include/clang/StaticAnalyzer/Checkers/StackUsageMeasuringVisitor.h:16
+//===----------------------------------------------------------------------===//
+
+#include "llvm/ADT/DenseMap.h"
----------------
Szelethus wrote:
> Missing header guard.
Speaking of missing header guards and in general: apart from tests, it might be a good idea to run CSA and Clang-Tidy //on your checker//'s code. E.g. Tidy has a [[ https://clang.llvm.org/extra/clang-tidy/checks/llvm-header-guard.html | `llvm-header-guard` ]] checker -- which might //not// find this particular issue, but in general we could find some issues on this code itself.

(For the ultimate version, run your own checker on your own checker's code... maybe you yourself used a too large stack somewhere! 😜 )


================
Comment at: include/clang/StaticAnalyzer/Checkers/StackUsageMeasuringVisitor.h:102-111
+  bool hasEmptyFlaggedUsageStored(const Stmt *S) const {
+    auto iter = StmtSubtreeUsages.find(S);
+    return iter != StmtSubtreeUsages.end() &&
+           iter->second == Usage::EmptyFlagged;
+  }
+  bool hasEmptyFlaggedUsageStored(const Decl *D) const {
+    auto iter = DeclSubtreeUsages.find(D);
----------------
Szelethus wrote:
> Would `isEmptyFlagged` be a better method name?
Then calling this method would mean answering this question: "A Stmt/Decl is empty flagged?" This is a "What?" moment.


================
Comment at: include/clang/StaticAnalyzer/Checkers/StackUsageMeasuringVisitor.h:117
+
+  int varSize(const VarDecl *Var) const {
+    return Var->hasLocalStorage() * sizeInBytes(Var->getType());
----------------
If we're talking about sizes, `int` (especially because it's //`signed`// `int`) might not be the most appropriate return type for these functions.


================
Comment at: include/clang/StaticAnalyzer/Checkers/StackUsageMeasuringVisitor.h:124
+               ? 0
+               : Context.getTypeSize(T) / Context.getCharWidth();
+  }
----------------
Szelethus wrote:
> The function name states `sizeInBytes`, but
> * `ASTContext::getTypeSizeInChars` returns the width in `CharUnits`,
> * which you divide with `Context.getCharWidth()`, which is in bits (according to the doxygen comments),
> * and we end up getting the amount of bytes `T` occupies?
> I might make a fool out of myself, but wouldn't the formula look something like this: `(Context.getTypeSize(T) * Context.getCharWidth()) / 8`?
Need to make a distinction whether or not we want to use `byte` in the sense of `8 bit` or in the sense of `sizeof(char)`. Also, the code calls `getTypeSize`, not `getTypeSize`~~`InChars`~~. This gives the answer in pure bits.

(Because of this, your suggested formula is wrong. `getTypeSize()` is already in bits, which you multiply further by `getCharWidth()` -- which is //on some systems// 8 -- then you divide it out. In most conventional systems the division cancels out the multiplication, and on systems where a `sizeof(char)`-byte is not 8, you'll get a non-integer quotient truncated as result.) 

So:

 * in case of you want to define `byte` to be `8 bits` forever and always, the divisor should be `8`, not `getCharWidth()`.
 * In case you want to define `byte` to be `sizeof(char)`, then the current code **is** correct, but you could just use: [[ https://clang.llvm.org/doxygen/classclang_1_1ASTContext.html#aa8c44e88d6ce9b3cce885b62d3cd5dff | `Context.getTypeSizeInChar()` ]] `.` [[ https://clang.llvm.org/doxygen/classclang_1_1CharUnits.html#a968b1a66a449ab256c4dd176bd663c07 | `getQuantity()` ]].

Just for the record: the C++14 standard says the following in § 1.7 `intro.memory`:

> The fundamental storage unit in the C++ memory is the **byte**.  A byte is //at least large enough// to contain any member of the basic execution character set (-> § 2.3 `lex.charset`) and the eight-bit code units of the Unicode UTF-8 encoding form and is composed of a contiguous sequence of bits, **the number of which is implementation-defined**.

(`lex.charset` is basically an ASCII subset of 96 cardinality, lowercase, uppercase, parens, etc.)

I'm not sure what the convention is for error reports when referring to bytes, but maybe it would be better to consider `byte` in the C++ sense and let the value change based on what target the analyzer file is being compiled for.


================
Comment at: lib/StaticAnalyzer/Checkers/StackSizeChecker.cpp:25
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include <sstream>
+
----------------
Szelethus wrote:
> Don't include standard stream libraries: https://llvm.org/docs/CodingStandards.html#include-iostream-is-forbidden
> Note that using the other stream headers (`<sstream>` for example) is not problematic in this regard 


================
Comment at: lib/StaticAnalyzer/Checkers/StackSizeChecker.cpp:41
+    : public Checker<check::PreCall, check::PostCall, check::EndFunction> {
+  mutable std::unique_ptr<BugType> StackOverflowBugType;
+
----------------
Szelethus wrote:
> I think you don't need to make this `mutable` anymore, you can just initialize it in the constructor.
`generateError` is const-qualified but writes this.


================
Comment at: lib/StaticAnalyzer/Checkers/StackSizeChecker.cpp:112
+      Visitor
+          .sumFunctionUpUntilTemplate(const_cast<FunctionDecl *>(Declaration))
+          .Maximal;
----------------
`const_cast` catches the eye. Why do you need to make this non-const?


================
Comment at: test/Analysis/stack-size.cpp:3
+
+// In this test an integer is 4 bytes, a bool is 1 byte.
+// The analysis uses the same method of calculation as the misc-stack-size
----------------
Could we use `int32_t` instead of `int`, and `uint8_t` instead of `bool`?


Repository:
  rC Clang

https://reviews.llvm.org/D52390