[PATCH] D52390: [analyzer] StackSizeChecker
Umann Kristóf via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Sat Sep 22 05:13:01 PDT 2018
Szelethus added a comment.
Hi!
Always great to see a new checker! I've started working in this project little over half a year ago, so I don't claim to be an expert, read my remarks as such! It'll be some time before I go through the entire code, but so far here are the things that caught my eye.
I think you should move the checker related files to a new folder, maybe `StackOverflow/`?
================
Comment at: include/clang/StaticAnalyzer/Checkers/Checkers.td:143-146
+def StackSizeChecker : Checker<"StackSize">,
+ HelpText<"Warn if estimated stack usage exceeds the StackUsageLimit parameter (measured in bytes, defaults to 100000)">,
+ DescFile<"StackSizeChecker.cpp">;
+
----------------
Alphabetical sorting?
(get it? //alpha//betical? I'm sorry.)
================
Comment at: include/clang/StaticAnalyzer/Checkers/StackUsageMeasuringVisitor.h:16
+//===----------------------------------------------------------------------===//
+
+#include "llvm/ADT/DenseMap.h"
----------------
Missing header guard.
================
Comment at: include/clang/StaticAnalyzer/Checkers/StackUsageMeasuringVisitor.h:25
+#include <functional>
+#include <map>
+
----------------
You included `map` but I don't see `std::map` anywhere?
================
Comment at: include/clang/StaticAnalyzer/Checkers/StackUsageMeasuringVisitor.h:28
+namespace clang {
+namespace stack {
+
----------------
Use `ento` after `clang`. Btw, does anybody know what `ento` refers to? Never got to know it :D
```
namespace clang {
namespace ento {
namespace stack {
================
Comment at: include/clang/StaticAnalyzer/Checkers/StackUsageMeasuringVisitor.h:57-60
+ explicit StackUsageMeasuringVisitor(ASTContext &Context)
+ : Context(Context), ContinueTraversing(true), ExitFlag(false),
+ HardExit(false), ForceTemporariesToConstant(false),
+ ExitPredicate(nullptr) {}
----------------
Since this is the only constructor of this visitor, I think C++11 in-class member initialization would be nicer.
================
Comment at: include/clang/StaticAnalyzer/Checkers/StackUsageMeasuringVisitor.h:62
+
+ Usage collected() const;
+
----------------
It isn't obvious to me what this function does.
================
Comment at: include/clang/StaticAnalyzer/Checkers/StackUsageMeasuringVisitor.h:64
+
+ // The visitor should traver post-order, because each node needs
+ // precalculated information in their children.
----------------
travel
================
Comment at: include/clang/StaticAnalyzer/Checkers/StackUsageMeasuringVisitor.h:94
+private:
+ void clear();
+ bool hasUsageStored(const Stmt *S) const {
----------------
Are you sure this function belongs in a visitor?
================
Comment at: include/clang/StaticAnalyzer/Checkers/StackUsageMeasuringVisitor.h:143-144
+
+} // stack
+} // clang
----------------
`// end of namespace .*`
================
Comment at: lib/StaticAnalyzer/Checkers/StackSizeChecker.cpp:19
+#include "clang/AST/StmtCXX.h"
+#include "clang/StaticAnalyzer/Checkers/StackUsageMeasuringVisitor.h"
+#include "clang/StaticAnalyzer/Core/AnalyzerOptions.h"
----------------
I bet you can't just `#include` this without getting some nasty buildbut errors down the line, just `#include "StackUsageMeasuringVisitor.h"`.
================
Comment at: lib/StaticAnalyzer/Checkers/StackSizeChecker.cpp:25
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include <sstream>
+
----------------
Don't include standard stream libraries: https://llvm.org/docs/CodingStandards.html#include-iostream-is-forbidden
================
Comment at: lib/StaticAnalyzer/Checkers/StackSizeChecker.cpp:34
+ int Used;
+ bool operator==(const StackInt &Other) const { return Used == Other.Used; }
+ void Profile(llvm::FoldingSetNodeID &ID) const { ID.AddInteger(Used); }
----------------
If you have a conversion operator to int, do you need `operator==`?
================
Comment at: lib/StaticAnalyzer/Checkers/StackSizeChecker.cpp:41
+ : public Checker<check::PreCall, check::PostCall, check::EndFunction> {
+ mutable std::unique_ptr<BugType> StackOverflowBugType;
+
----------------
I think you don't need to make this `mutable` anymore, you can just initialize it in the constructor.
================
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;
----------------
```
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;
}
================
Comment at: lib/StaticAnalyzer/Checkers/StackSizeChecker.cpp:78
+}
+}
+
----------------
`// end of anonymous namespace`
================
Comment at: lib/StaticAnalyzer/Checkers/StackSizeChecker.cpp:80
+
+REGISTER_LIST_WITH_PROGRAMSTATE(StackSizeList, StackInt)
+
----------------
Would look better right below the definition of `StackInt`.
================
Comment at: lib/StaticAnalyzer/Checkers/StackSizeChecker.cpp:81
+REGISTER_LIST_WITH_PROGRAMSTATE(StackSizeList, StackInt)
+
+void StackSizeChecker::generateError(int NumBytes, SourceRange Range,
----------------
I think some dividers would help a lot:
```
//===----------------------------------------------------------------------===//
// Methods for StackSizeChecker.
//===----------------------------------------------------------------------===//
================
Comment at: lib/StaticAnalyzer/Checkers/StackSizeChecker.cpp:94
+ // Generate the report.
+ std::ostringstream ErrorMessage;
+ ErrorMessage << "Estimated stack usage is " << NumBytes << " bytes";
----------------
Use LLVM streams.
```
llvm::SmallString<20> WarningBuf;
llvm::raw_svector_ostream OS(WarningBuf);
OS << "whatever"<< " needs" << " to" << " be" << " concatenated";
================
Comment at: lib/StaticAnalyzer/Checkers/StackSizeChecker.cpp:104
+ ProgramStateRef State = C.getState();
+ auto StackLevels = State->get<StackSizeList>();
+ if (length(StackLevels) != countPredecessors(C))
----------------
It isn't obvious to me why what "StackLevels" mean. Did you mean depth?
================
Comment at: lib/StaticAnalyzer/Checkers/StackSizeChecker.cpp:107-113
+ stack::StackUsageMeasuringVisitor Visitor(C.getASTContext());
+ auto Declaration =
+ dyn_cast_or_null<FunctionDecl>(C.getLocationContext()->getDecl());
+ int NumBytes =
+ Visitor
+ .sumFunctionUpUntilTemplate(const_cast<FunctionDecl *>(Declaration))
+ .Maximal;
----------------
You use `dyn_cast_or_null`, which to me indicates that there's a possibility of `Declaration` being null, but you never check it.
================
Comment at: lib/StaticAnalyzer/Checkers/StackSizeChecker.cpp:110-118
+ int NumBytes =
+ Visitor
+ .sumFunctionUpUntilTemplate(const_cast<FunctionDecl *>(Declaration))
+ .Maximal;
+ if (!StackLevels.isEmpty())
+ NumBytes += StackLevels.getHead();
+
----------------
In essence, this looks like
```
if (/* Calculated stack usage so far*/ > StackUsageLimit)
generateError(/*...*/);
```
but to me it's very obfuscated. Can you please either add more comments or move this to a separate function?
================
Comment at: lib/StaticAnalyzer/Checkers/StackSizeChecker.cpp:132-136
+ int NumBytes =
+ Visitor.sumFunctionUpUntil(Declaration, Call.getOriginExpr()).Constant +
+ Visitor.sumStmtWithTemporariesExtracted(Call.getOriginExpr()).Constant;
+ if (!StackLevels.isEmpty())
+ NumBytes += StackLevels.getHead();
----------------
Same issue here. It would be better just to see something like `int CurrentStackUsage = getCurrentStackUsage(C);`
================
Comment at: lib/StaticAnalyzer/Checkers/StackUsageMeasuringVisitor.cpp:17
+
+#include <clang/StaticAnalyzer/Checkers/StackUsageMeasuringVisitor.h>
+
----------------
#include "StackUsageMeasuringVisitor.h"
================
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};
+
----------------
Constant static data members should be in-class initialized. https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Rc-in-class-initializer
================
Comment at: lib/StaticAnalyzer/Checkers/StackUsageMeasuringVisitor.cpp:60
+}
+}
+
----------------
`// end of anonymous namespace`
================
Comment at: lib/StaticAnalyzer/Checkers/StackUsageMeasuringVisitor.cpp:61
+}
+
+void StackUsageMeasuringVisitor::putUsage(const Stmt *S, Usage U) {
----------------
```
//===----------------------------------------------------------------------===//
// Methods for StackUsageMeasuringVisitor.
//===----------------------------------------------------------------------===//
================
Comment at: lib/StaticAnalyzer/Checkers/StackUsageMeasuringVisitor.cpp:64-65
+ if (U.ExitFlag) {
+ ExitFlag = true;
+ ContinueTraversing = !HardExit;
+ }
----------------
To me it seems like `ContinueTraversing` always equals to `!ExitFlag`. Maybe remove one of them?
================
Comment at: lib/StaticAnalyzer/Checkers/StackUsageMeasuringVisitor.cpp:77
+ return U;
+ } else {
+ return Usage::Empty;
----------------
https://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return
================
Comment at: lib/StaticAnalyzer/Checkers/StackUsageMeasuringVisitor.cpp:104
+ return U;
+ } else {
+ return Usage::Empty;
----------------
https://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return
Repository:
rC Clang
https://reviews.llvm.org/D52390
More information about the cfe-commits
mailing list