[PATCH] D52390: [analyzer] StackSizeChecker

[Umann Kristóf via Phabricator via cfe-commits]

Szelethus added subscribers: rnkovacs, baloghadamsoftware.
Szelethus added a comment.

I think the idea for a checker like this is great! I left some inline comments, but most of them are minor nits. I have some general remarks to make however:

- Your code lacks comments, especially a nice documentation describing this problem, how you approach it, what the general algorithm is etc. For example, you use `Usage` extensively, but I am still a little unsure what the difference is between `Empty` and `EmptyFlagged`. I'd encourage you to look at a couple nice examples:
  1. @baloghadamsoftware's `IteratorChecker` <https://github.com/llvm-mirror/clang/blob/master/lib/StaticAnalyzer/Checkers/IteratorChecker.cpp> (A long code with fairly long comments to support it),
  2. @rnkovacs's `DeleteWithNonVirtualDtorChecker` <https://github.com/llvm-mirror/clang/blob/master/lib/StaticAnalyzer/Checkers/DeleteWithNonVirtualDtorChecker.cpp> (A shorter code with shorter explanation),
  3. My `UninitializedObjectChecker` <https://github.com/llvm-mirror/clang/tree/master/lib/StaticAnalyzer/Checkers/UninitializedObject> (Very similar to your checker in terms of quantity of files etc)

- Make sure you follow the LLVM Coding Standards <https://llvm.org/docs/CodingStandards.html>.
- As stated before, should move this to a new directory.
- For the amount of checker implementation you have, there aren't to many test cases. I can see that you handle `CXXTryStmt`, but there are no test cases for it.
- This revision is way too large for comfortable reading, you can help this by learning from my mistake and split it up into smaller patches. @NoQ summarized the reasons for this very nicely here: https://reviews.llvm.org/D45532#1083670
  1. Post a checker class with an empty callback [1]. Even in this stage, some discussion could be had with the general direction the implementation should go. This avoids the potential of investing a lot of effort into a project that could be wrong from the get-go. Don't get me wrong, I'm not at all saying this is the case here!
  2. Add implementation for checking a very easy case, such as a function with 10 array declarations, each having an absurd size.
  3. Add each new functionality as a separate patch, and add dependencies with "Edit Related Revisions". For example, one for handling branches, one for loops, etc.

Let me emphasize that I'm a beginner myself, so I could be wrong on many of these ^-^

[1] This could be a nice first patch:

  //=======- StackSizeChecker.cpp ----------------------------------*- C++ -*-==//
  //
  //                     The LLVM Compiler Infrastructure
  //
  // This file is distributed under the University of Illinois Open Source
  // License. See LICENSE.TXT for details.
  //
  //===----------------------------------------------------------------------===//
  //
  //  This file defines a checker that checks for suspiciously high stack use.
  //  The checker produces a warning whenever the given stack limit is exceeded.
  //  The limit is defined by the "StackUsageLimit" analyzer parameter,
  //  or defaults to 100000 bytes.
  //
  //===----------------------------------------------------------------------===//
  
  class StackSizeChecker
      : public Checker<check::PreCall, check::PostCall, check::EndFunction> {
    mutable std::unique_ptr<BugType> StackOverflowBugType;
  
  public:
    int StackUsageLimit;
  
    void checkPreCall(const CallEvent &Call, CheckerContext &C) const {}
    void checkPostCall(const CallEvent &Call, CheckerContext &C) const {}
    void checkEndFunction(CheckerContext &C) const {}
  };
  
  void clang::ento::registerStackSizeChecker(CheckerManager &Mgr) {
    StackSizeChecker *Check = Mgr.registerChecker<StackSizeChecker>();
    Check->StackUsageLimit = Mgr.getAnalyzerOptions().getOptionAsInteger(
        "StackUsageLimit", /*DefaultVal*/ 100000, Check);
  }



================
Comment at: include/clang/StaticAnalyzer/Checkers/StackUsageMeasuringVisitor.h:30
+
+struct Usage {
+
----------------
To me, `Usage` seems to be a way too general name. Also, for a class being used this heavily, it has no comments explaining what it does. Is this a property of a variable? A function call?


================
Comment at: include/clang/StaticAnalyzer/Checkers/StackUsageMeasuringVisitor.h:46
+                 // its children.
+  bool ExitFlag; // Tells the visitor to cease consuming the tree.
+
----------------
Which tree? I know you refer to the //statement// tree after reading the code for a while, but it would be more obvious if you put some comments for `Usage`.


================
Comment at: include/clang/StaticAnalyzer/Checkers/StackUsageMeasuringVisitor.h:94-131
+  void clear();
+  bool hasUsageStored(const Stmt *S) const {
+    return StmtSubtreeUsages.find(S) != StmtSubtreeUsages.end();
+  }
+  bool hasUsageStored(const Decl *D) const {
+    return DeclSubtreeUsages.find(D) != DeclSubtreeUsages.end();
+  }
----------------
Szelethus wrote:
> Are you sure this function belongs in a visitor?
Actually, it might be cleaner to move these into a new class, and have `StackUsageMeasuringVisitor` manage an object from that class.


================
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);
----------------
Would `isEmptyFlagged` be a better method name?


================
Comment at: include/clang/StaticAnalyzer/Checkers/StackUsageMeasuringVisitor.h:124
+               ? 0
+               : Context.getTypeSize(T) / Context.getCharWidth();
+  }
----------------
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`?


================
Comment at: include/clang/StaticAnalyzer/Checkers/StackUsageMeasuringVisitor.h:135-138
+  bool ContinueTraversing;
+  bool ExitFlag;
+  bool HardExit;
+  bool ForceTemporariesToConstant;
----------------
Needs explanation.


================
Comment at: lib/StaticAnalyzer/Checkers/StackSizeChecker.cpp:46
+public:
+  StackSizeChecker() : StackUsageLimit(0) {}
+  int StackUsageLimit;
----------------
Why initialize here if you're going to overwrite it when you register the checker?


================
Comment at: lib/StaticAnalyzer/Checkers/StackSizeChecker.cpp:54
+
+int countPredecessors(const CheckerContext &C) {
+  const auto *Frame = C.getStackFrame()->getParent();
----------------
I think this would be better as `static` with the definition being out-of-line on the bottom of the file (but not before registering the checker, as it's traditionally the last function in the checker's TU).


================
Comment at: lib/StaticAnalyzer/Checkers/StackSizeChecker.cpp:68-77
+int length(const llvm::ImmutableList<StackInt>& L) {
+  int N = 0;
+  auto I = L.begin();
+  auto E = L.end();
+  while(I != E){
+    ++N;
+    ++I;
----------------
Szelethus wrote:
> ```
> size_t length(const llvm::ImmutableList<StackInt> &L) {
>   int Length = 0;
>   for (auto It = L.begin(), E = L.end(); It != E; ++It)
>     ++Length;
>   return Length;
> }
Actually, same here, should be `static` and defined out of line, so the declarations and documentation can be placed on top of the file and implementation can be found on the bottom.


================
Comment at: lib/StaticAnalyzer/Checkers/StackSizeChecker.cpp:157-158
+  StackSizeChecker *Check = Mgr.registerChecker<StackSizeChecker>();
+  Check->StackUsageLimit = Mgr.getAnalyzerOptions().getOptionAsInteger(
+      "StackUsageLimit", 100000, Check);
+}
----------------
```
"StackUsageLimit", /*DefaultVal*/ 100000, Check);


================
Comment at: lib/StaticAnalyzer/Checkers/StackUsageMeasuringVisitor.cpp:22-23
+
+const Usage Usage::Empty = {0, 0, false, false};
+const Usage Usage::EmptyFlagged = {0, 0, false, true};
+
----------------
Szelethus wrote:
> Constant static data members should be in-class initialized. https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Rc-in-class-initializer
Actually, these shouldn't be static variables, but rather static functions (and probably within the class definition):
```
static Usage getEmptyUsage() { return Usage{0, 0, false, false}; }
static Usage getEmptyFlaggedUsage() { return Usage{0, 0, false, true}; }


================
Comment at: lib/StaticAnalyzer/Checkers/StackUsageMeasuringVisitor.cpp:37-39
+Usage withoutConstant(Usage U) {
+  return {0, U.Maximal, U.DynAlloc, U.ExitFlag};
+}
----------------
I think this would be more fitting as a static member function.


================
Comment at: lib/StaticAnalyzer/Checkers/StackUsageMeasuringVisitor.cpp:41
+
+bool countsAsZero(const Expr *Ex) {
+  auto Cls = Ex->getStmtClass();
----------------
I don't really understand what this function does. 


================
Comment at: lib/StaticAnalyzer/Checkers/StackUsageMeasuringVisitor.cpp:109
+
+int StackUsageMeasuringVisitor::exprRetSize(const Expr *E) const {
+  auto StmtCls = E->getStmtClass();
----------------
`getReturnExpressionSize`?


================
Comment at: lib/StaticAnalyzer/Checkers/StackUsageMeasuringVisitor.cpp:153
+    } else if (D->getType()->isDependentType()) {
+      // A template declaration was found, signal the algorithm.
+      putUsage(D, Usage::EmptyFlagged);
----------------
Can you rephrase this? "signal the algorithm" doesn't sound too descriptive to me.


================
Comment at: test/Analysis/stack-size-callchain.cpp:1
+// RUN: %clang_cc1 -analyze -analyzer-checker=core,alpha.core.StackSize -analyzer-config alpha.core.StackSize:StackUsageLimit=200 -std=c++11 -triple x86_64-unknown-linux-gnu -verify %s
+
----------------
We only started doing this recently, but you can actually organize the run line like this:
```
// RUN: %clang_cc1 -analyze -analyzer-checker=core,alpha.core.StackSize \
// RUN:   -analyzer-config alpha.core.StackSize:StackUsageLimit=200 \
// RUN:   -std=c++11 -triple x86_64-unknown-linux-gnu -verify %s


Repository:
  rC Clang

https://reviews.llvm.org/D52390