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