[PATCH] D80522: [Analyzer] [NFC] Parameter Regions

2020-06-09 Thread Balogh , Ádám via Phabricator via cfe-commits 
baloghadamsoftware added a comment.

In D80522#2082095 , @teemperor wrote:

> This introduced a compiler warning:
>
>   
> /Users/teemperor/1llvm/llvm-project/clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h:1102:23:
>  warning: 'getDecl' overrides a member function but is not marked 'override' 
> [-Winconsistent-missing-override]
> const ObjCIvarDecl *getDecl() const;
> ^
>   
> /Users/teemperor/1llvm/llvm-project/clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h:898:28:
>  note: overridden virtual function is here
> virtual const ValueDecl *getDecl() const = 0;
>  ^
>


Thx! Fixed now!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80522/new/

https://reviews.llvm.org/D80522



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D80522: [Analyzer] [NFC] Parameter Regions

2020-06-09 Thread Raphael Isemann via Phabricator via cfe-commits 
teemperor added a comment.

This introduced a compiler warning:

  
/Users/teemperor/1llvm/llvm-project/clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h:1102:23:
 warning: 'getDecl' overrides a member function but is not marked 'override' 
[-Winconsistent-missing-override]
const ObjCIvarDecl *getDecl() const;
^
  
/Users/teemperor/1llvm/llvm-project/clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h:898:28:
 note: overridden virtual function is here
virtual const ValueDecl *getDecl() const = 0;
 ^


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80522/new/

https://reviews.llvm.org/D80522



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D80522: [Analyzer] [NFC] Parameter Regions

2020-06-09 Thread Balogh , Ádám via Phabricator via cfe-commits 
This revision was automatically updated to reflect the committed changes.
Closed by commit rG98db1f990fc2: [Analyzer] [NFC] Parameter Regions (authored 
by baloghadamsoftware).

Changed prior to commit:
  https://reviews.llvm.org/D80522?vs=268131=269477#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80522/new/

https://reviews.llvm.org/D80522

Files:
  clang/include/clang/StaticAnalyzer/Checkers/SValExplainer.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/Regions.def
  clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
  clang/lib/StaticAnalyzer/Core/CallEvent.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
  clang/lib/StaticAnalyzer/Core/MemRegion.cpp
  clang/lib/StaticAnalyzer/Core/Store.cpp
  clang/test/Analysis/explain-svals.c
  clang/test/Analysis/explain-svals.cpp
  clang/test/Analysis/explain-svals.m
  clang/unittests/StaticAnalyzer/CMakeLists.txt
  clang/unittests/StaticAnalyzer/ParamRegionTest.cpp

Index: clang/unittests/StaticAnalyzer/ParamRegionTest.cpp
===
--- /dev/null
+++ clang/unittests/StaticAnalyzer/ParamRegionTest.cpp
@@ -0,0 +1,109 @@
+//===- unittests/StaticAnalyzer/ParamRegionTest.cpp ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "Reusables.h"
+
+#include "clang/Tooling/Tooling.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace ento {
+namespace {
+
+class ParamRegionTestConsumer : public ExprEngineConsumer {
+  void performTest(const Decl *D) {
+StoreManager  = Eng.getStoreManager();
+MemRegionManager  = StMgr.getRegionManager();
+const StackFrameContext *SFC =
+Eng.getAnalysisDeclContextManager().getStackFrame(D);
+
+if (const auto *FD = dyn_cast(D)) {
+  for (const auto *P : FD->parameters()) {
+const TypedValueRegion *Reg = MRMgr.getVarRegion(P, SFC);
+if (SFC->inTopFrame())
+  assert(isa(Reg));
+else
+  assert(isa(Reg));
+  }
+} else if (const auto *CD = dyn_cast(D)) {
+  for (const auto *P : CD->parameters()) {
+const TypedValueRegion *Reg = MRMgr.getVarRegion(P, SFC);
+if (SFC->inTopFrame())
+  assert(isa(Reg));
+else
+  assert(isa(Reg));
+  }
+} else if (const auto *MD = dyn_cast(D)) {
+  for (const auto *P : MD->parameters()) {
+const TypedValueRegion *Reg = MRMgr.getVarRegion(P, SFC);
+if (SFC->inTopFrame())
+  assert(isa(Reg));
+else
+  assert(isa(Reg));
+  }
+}
+  }
+
+public:
+  ParamRegionTestConsumer(CompilerInstance ) : ExprEngineConsumer(C) {}
+
+  bool HandleTopLevelDecl(DeclGroupRef DG) override {
+for (const auto *D : DG) {
+  performTest(D);
+}
+return true;
+  }
+};
+
+class ParamRegionTestAction : public ASTFrontendAction {
+public:
+  std::unique_ptr CreateASTConsumer(CompilerInstance ,
+ StringRef File) override {
+return std::make_unique(Compiler);
+  }
+};
+
+TEST(ParamRegion, ParamRegionTest) {
+  EXPECT_TRUE(
+  tooling::runToolOnCode(std::make_unique(),
+ R"(void foo(int n) {
+  auto lambda = [n](int m) {
+return n + m;
+  };
+
+  int k = lambda(2);
+}
+
+void bar(int l) {
+  foo(l);
+}
+
+struct S {
+  int n;
+  S(int nn): n(nn) {}
+};
+
+void baz(int p) {
+  S s(p);
+})"));
+  EXPECT_TRUE(
+  tooling::runToolOnCode(std::make_unique(),
+ R"(@interface O
++ alloc;
+- initWithInt:(int)q;
+@end
+
+void qix(int r) {
+  O *o = [[O alloc] initWithInt:r];
+})",
+ "input.m"));
+}
+
+} // namespace
+} // namespace ento
+} // namespace clang
Index: clang/unittests/StaticAnalyzer/CMakeLists.txt

[PATCH] D80522: [Analyzer] [NFC] Parameter Regions

2020-06-08 Thread Artem Dergachev via Phabricator via cfe-commits 
NoQ added inline comments.



Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h:404-407
   /// Returns memory location for a parameter variable within the callee stack
   /// frame. May fail; returns null on failure.
-  const VarRegion *getParameterLocation(unsigned Index,
-unsigned BlockCount) const;
+  const ParamVarRegion *getParameterLocation(unsigned Index,
+ unsigned BlockCount) const;

I guess we should document the landmine discussed in D80366#inline-747804.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80522/new/

https://reviews.llvm.org/D80522



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D80522: [Analyzer] [NFC] Parameter Regions

2020-06-03 Thread Artem Dergachev via Phabricator via cfe-commits 
NoQ added inline comments.



Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h:921
+public:
+  const VarDecl *getDecl() const override = 0;
+

baloghadamsoftware wrote:
> NoQ wrote:
> > What's the point of overriding one pure virtual method with another pure 
> > virtual method?
> To constrain the return value from `Decl` to `VarDecl`. Without this we have 
> change all the callers to cast the return value from this function to 
> `VarDecl`. It would be ugly.
Aha, covariance! Nice!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80522/new/

https://reviews.llvm.org/D80522



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D80522: [Analyzer] [NFC] Parameter Regions

2020-06-03 Thread Balogh , Ádám via Phabricator via cfe-commits 
baloghadamsoftware marked 2 inline comments as done.
baloghadamsoftware added inline comments.



Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h:921
+public:
+  const VarDecl *getDecl() const override = 0;
+

NoQ wrote:
> What's the point of overriding one pure virtual method with another pure 
> virtual method?
To constrain the return value from `Decl` to `VarDecl`. Without this we have 
change all the callers to cast the return value from this function to 
`VarDecl`. It would be ugly.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80522/new/

https://reviews.llvm.org/D80522



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D80522: [Analyzer] [NFC] Parameter Regions

2020-06-03 Thread Balogh , Ádám via Phabricator via cfe-commits 
baloghadamsoftware updated this revision to Diff 268131.
baloghadamsoftware added a comment.

Comment fixed.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80522/new/

https://reviews.llvm.org/D80522

Files:
  clang/include/clang/StaticAnalyzer/Checkers/SValExplainer.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/Regions.def
  clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
  clang/lib/StaticAnalyzer/Core/CallEvent.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
  clang/lib/StaticAnalyzer/Core/MemRegion.cpp
  clang/lib/StaticAnalyzer/Core/Store.cpp
  clang/test/Analysis/explain-svals.c
  clang/test/Analysis/explain-svals.cpp
  clang/test/Analysis/explain-svals.m
  clang/unittests/StaticAnalyzer/CMakeLists.txt
  clang/unittests/StaticAnalyzer/ParamRegionTest.cpp

Index: clang/unittests/StaticAnalyzer/ParamRegionTest.cpp
===
--- /dev/null
+++ clang/unittests/StaticAnalyzer/ParamRegionTest.cpp
@@ -0,0 +1,109 @@
+//===- unittests/StaticAnalyzer/ParamRegionTest.cpp ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "Reusables.h"
+
+#include "clang/Tooling/Tooling.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace ento {
+namespace {
+
+class ParamRegionTestConsumer : public ExprEngineConsumer {
+  void performTest(const Decl *D) {
+StoreManager  = Eng.getStoreManager();
+MemRegionManager  = StMgr.getRegionManager();
+const StackFrameContext *SFC =
+Eng.getAnalysisDeclContextManager().getStackFrame(D);
+
+if (const auto *FD = dyn_cast(D)) {
+  for (const auto *P : FD->parameters()) {
+const TypedValueRegion *Reg = MRMgr.getVarRegion(P, SFC);
+if (SFC->inTopFrame())
+  assert(isa(Reg));
+else
+  assert(isa(Reg));
+  }
+} else if (const auto *CD = dyn_cast(D)) {
+  for (const auto *P : CD->parameters()) {
+const TypedValueRegion *Reg = MRMgr.getVarRegion(P, SFC);
+if (SFC->inTopFrame())
+  assert(isa(Reg));
+else
+  assert(isa(Reg));
+  }
+} else if (const auto *MD = dyn_cast(D)) {
+  for (const auto *P : MD->parameters()) {
+const TypedValueRegion *Reg = MRMgr.getVarRegion(P, SFC);
+if (SFC->inTopFrame())
+  assert(isa(Reg));
+else
+  assert(isa(Reg));
+  }
+}
+  }
+
+public:
+  ParamRegionTestConsumer(CompilerInstance ) : ExprEngineConsumer(C) {}
+
+  bool HandleTopLevelDecl(DeclGroupRef DG) override {
+for (const auto *D : DG) {
+  performTest(D);
+}
+return true;
+  }
+};
+
+class ParamRegionTestAction : public ASTFrontendAction {
+public:
+  std::unique_ptr CreateASTConsumer(CompilerInstance ,
+ StringRef File) override {
+return std::make_unique(Compiler);
+  }
+};
+
+TEST(ParamRegion, ParamRegionTest) {
+  EXPECT_TRUE(
+  tooling::runToolOnCode(std::make_unique(),
+ R"(void foo(int n) {
+ auto lambda = [n](int m) {
+   return n + m;
+ };
+
+ int k = lambda(2);
+   }
+
+   void bar(int l) {
+ foo(l);
+   }
+
+   struct S {
+ int n;
+ S(int nn): n(nn) {}
+   };
+
+   void baz(int p) {
+ S s(p);
+   })"));
+  EXPECT_TRUE(
+  tooling::runToolOnCode(std::make_unique(),
+ R"(@interface O
+   + alloc;
+   - initWithInt:(int)q;
+   @end
+
+   void qix(int r) {
+ O *o = [[O alloc] initWithInt:r];
+   })",
+ "input.m"));
+}
+
+} // namespace
+} // namespace ento
+} // namespace clang
Index: clang/unittests/StaticAnalyzer/CMakeLists.txt
===
--- clang/unittests/StaticAnalyzer/CMakeLists.txt
+++ clang/unittests/StaticAnalyzer/CMakeLists.txt
@@ -8,6 +8,7 @@

[PATCH] D80522: [Analyzer] [NFC] Parameter Regions -- Alternative Approach

2020-06-01 Thread Artem Dergachev via Phabricator via cfe-commits 
NoQ accepted this revision.
NoQ added a comment.
This revision is now accepted and ready to land.

Looks great, thanks! I like how it turned out, i guess let's prefer this to 
D79704 .




Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h:921
+public:
+  const VarDecl *getDecl() const override = 0;
+

What's the point of overriding one pure virtual method with another pure 
virtual method?



Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h:977
+// implementing stack frame creations for functions without decl (functions
+// passed by function pointer) methods of `ParamVarRegion` must be updated.
+class ParamVarRegion : public VarRegion {

*unknown* function pointer!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80522/new/

https://reviews.llvm.org/D80522



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D80522: [Analyzer] [NFC] Parameter Regions -- Alternative Approach

2020-05-28 Thread Balogh , Ádám via Phabricator via cfe-commits 
baloghadamsoftware marked an inline comment as done.
baloghadamsoftware added a comment.

> I will check tomorrow whether we could remove `DeclRegion` without too much 
> code duplication.

It turns out that `DeclRegion` is used in 7 different checkers plus in 
`BugReporterVisitor`s. One typical use case is to get a name for the region (by 
getting the `Decl` and trying to cast it to `NamedDecl`), but there are also 
others as well. For now I suggest to keep it to avoid code duplication.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80522/new/

https://reviews.llvm.org/D80522



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D80522: [Analyzer] [NFC] Parameter Regions -- Alternative Approach

2020-05-28 Thread Balogh , Ádám via Phabricator via cfe-commits 
baloghadamsoftware marked 5 inline comments as done.
baloghadamsoftware added inline comments.



Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h:917
 assert(isa(sReg) || isa(sReg) ||
isa(sReg) || isa(sReg));
   }

balazske wrote:
> This assert should not be duplicated. It has the place in `VarRegion` or 
> `NonParamVarRegion`, but not both.
Copy-paste...


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80522/new/

https://reviews.llvm.org/D80522



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D80522: [Analyzer] [NFC] Parameter Regions -- Alternative Approach

2020-05-28 Thread Balogh , Ádám via Phabricator via cfe-commits 
baloghadamsoftware updated this revision to Diff 266851.
baloghadamsoftware marked an inline comment as done.
baloghadamsoftware added a comment.

Updated according to the comments of @balazske.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80522/new/

https://reviews.llvm.org/D80522

Files:
  clang/include/clang/StaticAnalyzer/Checkers/SValExplainer.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/Regions.def
  clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
  clang/lib/StaticAnalyzer/Core/CallEvent.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
  clang/lib/StaticAnalyzer/Core/MemRegion.cpp
  clang/lib/StaticAnalyzer/Core/Store.cpp
  clang/test/Analysis/explain-svals.c
  clang/test/Analysis/explain-svals.cpp
  clang/test/Analysis/explain-svals.m
  clang/unittests/StaticAnalyzer/CMakeLists.txt
  clang/unittests/StaticAnalyzer/ParamRegionTest.cpp

Index: clang/unittests/StaticAnalyzer/ParamRegionTest.cpp
===
--- /dev/null
+++ clang/unittests/StaticAnalyzer/ParamRegionTest.cpp
@@ -0,0 +1,109 @@
+//===- unittests/StaticAnalyzer/ParamRegionTest.cpp ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "Reusables.h"
+
+#include "clang/Tooling/Tooling.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace ento {
+namespace {
+
+class ParamRegionTestConsumer : public ExprEngineConsumer {
+  void performTest(const Decl *D) {
+StoreManager  = Eng.getStoreManager();
+MemRegionManager  = StMgr.getRegionManager();
+const StackFrameContext *SFC =
+Eng.getAnalysisDeclContextManager().getStackFrame(D);
+
+if (const auto *FD = dyn_cast(D)) {
+  for (const auto *P : FD->parameters()) {
+const TypedValueRegion *Reg = MRMgr.getVarRegion(P, SFC);
+if (SFC->inTopFrame())
+  assert(isa(Reg));
+else
+  assert(isa(Reg));
+  }
+} else if (const auto *CD = dyn_cast(D)) {
+  for (const auto *P : CD->parameters()) {
+const TypedValueRegion *Reg = MRMgr.getVarRegion(P, SFC);
+if (SFC->inTopFrame())
+  assert(isa(Reg));
+else
+  assert(isa(Reg));
+  }
+} else if (const auto *MD = dyn_cast(D)) {
+  for (const auto *P : MD->parameters()) {
+const TypedValueRegion *Reg = MRMgr.getVarRegion(P, SFC);
+if (SFC->inTopFrame())
+  assert(isa(Reg));
+else
+  assert(isa(Reg));
+  }
+}
+  }
+
+public:
+  ParamRegionTestConsumer(CompilerInstance ) : ExprEngineConsumer(C) {}
+
+  bool HandleTopLevelDecl(DeclGroupRef DG) override {
+for (const auto *D : DG) {
+  performTest(D);
+}
+return true;
+  }
+};
+
+class ParamRegionTestAction : public ASTFrontendAction {
+public:
+  std::unique_ptr CreateASTConsumer(CompilerInstance ,
+ StringRef File) override {
+return std::make_unique(Compiler);
+  }
+};
+
+TEST(ParamRegion, ParamRegionTest) {
+  EXPECT_TRUE(
+  tooling::runToolOnCode(std::make_unique(),
+ R"(void foo(int n) {
+ auto lambda = [n](int m) {
+   return n + m;
+ };
+
+ int k = lambda(2);
+   }
+
+   void bar(int l) {
+ foo(l);
+   }
+
+   struct S {
+ int n;
+ S(int nn): n(nn) {}
+   };
+
+   void baz(int p) {
+ S s(p);
+   })"));
+  EXPECT_TRUE(
+  tooling::runToolOnCode(std::make_unique(),
+ R"(@interface O
+   + alloc;
+   - initWithInt:(int)q;
+   @end
+
+   void qix(int r) {
+ O *o = [[O alloc] initWithInt:r];
+   })",
+ "input.m"));
+}
+
+} // namespace
+} // namespace ento
+} // namespace clang
Index: clang/unittests/StaticAnalyzer/CMakeLists.txt
===
--- clang/unittests/StaticAnalyzer/CMakeLists.txt
+++

[PATCH] D80522: [Analyzer] [NFC] Parameter Regions -- Alternative Approach

2020-05-27 Thread Balázs Kéri via Phabricator via cfe-commits 
balazske added inline comments.



Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h:917
 assert(isa(sReg) || isa(sReg) ||
isa(sReg) || isa(sReg));
   }

This assert should not be duplicated. It has the place in `VarRegion` or 
`NonParamVarRegion`, but not both.



Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h:976
+
+/// ParamVarRegion - Represents a region for paremters. Only parameters of the
+/// function in the current stack frame are represented as `ParamVarRegion`s.

typo here: "paremters"



Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h:978
+/// function in the current stack frame are represented as `ParamVarRegion`s.
+/// Parameters of top-level analyzed functions as well as captured paremeters
+/// by lambdas and blocks are repesented as `VarRegion`s.

"paremeters"



Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h:979
+/// Parameters of top-level analyzed functions as well as captured paremeters
+/// by lambdas and blocks are repesented as `VarRegion`s.
+

`NonParamVarRegion`s ?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80522/new/

https://reviews.llvm.org/D80522



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D80522: [Analyzer] [NFC] Parameter Regions -- Alternative Approach

2020-05-26 Thread Balogh , Ádám via Phabricator via cfe-commits 
baloghadamsoftware marked 4 inline comments as done.
baloghadamsoftware added a comment.

In D80522#2055040 , @NoQ wrote:

> Nice, looks like you managed to reuse most of the code. I still feel like we 
> should ditch `DeclRegion` entirely (forcing its ~5 current users consider its 
> specific variants separately) but i don't insist.


Thank you for you comments. I will check tomorrow whether we could remove 
`DeclRegion` without too much code duplication.




Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp:539-540
   (void)VV;
-  assert(cast(VV.castAs().getRegion())
- ->getStackFrame()->getParent()
- ->getStackFrame() == LC->getStackFrame());
+  if (const auto *VR =
+  dyn_cast(VV.castAs().getRegion())) {
+assert(VR->getStackFrame()->getParent()

NoQ wrote:
> Why did you relax this `cast<>` to `dyn_cast<>`?
I started from the original approach and forgot to reset this file.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80522/new/

https://reviews.llvm.org/D80522



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D80522: [Analyzer] [NFC] Parameter Regions -- Alternative Approach

2020-05-26 Thread Balogh , Ádám via Phabricator via cfe-commits 
baloghadamsoftware updated this revision to Diff 266298.
baloghadamsoftware added a comment.
Herald added a subscriber: wuzish.

Wrong diff uploaded previously, now fixed.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80522/new/

https://reviews.llvm.org/D80522

Files:
  clang/include/clang/StaticAnalyzer/Checkers/SValExplainer.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/Regions.def
  clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
  clang/lib/StaticAnalyzer/Core/CallEvent.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
  clang/lib/StaticAnalyzer/Core/MemRegion.cpp
  clang/lib/StaticAnalyzer/Core/Store.cpp
  clang/test/Analysis/explain-svals.c
  clang/test/Analysis/explain-svals.cpp
  clang/test/Analysis/explain-svals.m
  clang/unittests/StaticAnalyzer/CMakeLists.txt
  clang/unittests/StaticAnalyzer/ParamRegionTest.cpp

Index: clang/unittests/StaticAnalyzer/ParamRegionTest.cpp
===
--- /dev/null
+++ clang/unittests/StaticAnalyzer/ParamRegionTest.cpp
@@ -0,0 +1,109 @@
+//===- unittests/StaticAnalyzer/ParamRegionTest.cpp ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "Reusables.h"
+
+#include "clang/Tooling/Tooling.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace ento {
+namespace {
+
+class ParamRegionTestConsumer : public ExprEngineConsumer {
+  void performTest(const Decl *D) {
+StoreManager  = Eng.getStoreManager();
+MemRegionManager  = StMgr.getRegionManager();
+const StackFrameContext *SFC =
+Eng.getAnalysisDeclContextManager().getStackFrame(D);
+
+if (const auto *FD = dyn_cast(D)) {
+  for (const auto *P : FD->parameters()) {
+const TypedValueRegion *Reg = MRMgr.getVarRegion(P, SFC);
+if (SFC->inTopFrame())
+  assert(isa(Reg));
+else
+  assert(isa(Reg));
+  }
+} else if (const auto *CD = dyn_cast(D)) {
+  for (const auto *P : CD->parameters()) {
+const TypedValueRegion *Reg = MRMgr.getVarRegion(P, SFC);
+if (SFC->inTopFrame())
+  assert(isa(Reg));
+else
+  assert(isa(Reg));
+  }
+} else if (const auto *MD = dyn_cast(D)) {
+  for (const auto *P : MD->parameters()) {
+const TypedValueRegion *Reg = MRMgr.getVarRegion(P, SFC);
+if (SFC->inTopFrame())
+  assert(isa(Reg));
+else
+  assert(isa(Reg));
+  }
+}
+  }
+
+public:
+  ParamRegionTestConsumer(CompilerInstance ) : ExprEngineConsumer(C) {}
+
+  bool HandleTopLevelDecl(DeclGroupRef DG) override {
+for (const auto *D : DG) {
+  performTest(D);
+}
+return true;
+  }
+};
+
+class ParamRegionTestAction : public ASTFrontendAction {
+public:
+  std::unique_ptr CreateASTConsumer(CompilerInstance ,
+ StringRef File) override {
+return std::make_unique(Compiler);
+  }
+};
+
+TEST(ParamRegion, ParamRegionTest) {
+  EXPECT_TRUE(
+  tooling::runToolOnCode(std::make_unique(),
+ R"(void foo(int n) {
+ auto lambda = [n](int m) {
+   return n + m;
+ };
+
+ int k = lambda(2);
+   }
+
+   void bar(int l) {
+ foo(l);
+   }
+
+   struct S {
+ int n;
+ S(int nn): n(nn) {}
+   };
+
+   void baz(int p) {
+ S s(p);
+   })"));
+  EXPECT_TRUE(
+  tooling::runToolOnCode(std::make_unique(),
+ R"(@interface O
+   + alloc;
+   - initWithInt:(int)q;
+   @end
+
+   void qix(int r) {
+ O *o = [[O alloc] initWithInt:r];
+   })",
+ "input.m"));
+}
+
+} // namespace
+} // namespace ento
+} // namespace clang
Index: clang/unittests/StaticAnalyzer/CMakeLists.txt
===
--- clang/unittests/StaticAnalyzer/CMakeLists.txt
+++

[PATCH] D80522: [Analyzer] [NFC] Parameter Regions -- Alternative Approach

2020-05-26 Thread Balogh , Ádám via Phabricator via cfe-commits 
baloghadamsoftware updated this revision to Diff 266296.
baloghadamsoftware added a comment.
Herald added subscribers: llvm-commits, MaskRay, hiraditya, arichardson, 
nemanjai, emaste.
Herald added a reviewer: espindola.
Herald added a reviewer: MaskRay.
Herald added a project: LLVM.

Updated according to the comments.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80522/new/

https://reviews.llvm.org/D80522

Files:
  clang/include/clang/StaticAnalyzer/Checkers/SValExplainer.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/Regions.def
  clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
  clang/lib/StaticAnalyzer/Core/CallEvent.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
  clang/lib/StaticAnalyzer/Core/MemRegion.cpp
  clang/lib/StaticAnalyzer/Core/Store.cpp
  clang/test/Analysis/explain-svals.c
  clang/test/Analysis/explain-svals.cpp
  clang/test/Analysis/explain-svals.m
  clang/unittests/StaticAnalyzer/CMakeLists.txt
  clang/unittests/StaticAnalyzer/ParamRegionTest.cpp
  compiler-rt/lib/fuzzer/afl/afl_driver.cpp
  lld/ELF/InputFiles.cpp
  lld/test/ELF/invalid/verneed-shared.test
  lld/test/ELF/invalid/verneed-shared.yaml
  lldb/include/lldb/Utility/Args.h
  lldb/test/API/python_api/hello_world/TestHelloWorld.py
  lldb/test/API/python_api/sbdata/TestSBData.py
  llvm/include/llvm/Analysis/ObjCARCAnalysisUtils.h
  llvm/include/llvm/CodeGen/ResourcePriorityQueue.h
  llvm/include/llvm/Support/YAMLTraits.h
  llvm/lib/Analysis/ObjCARCAliasAnalysis.cpp
  llvm/lib/CodeGen/SelectionDAG/ResourcePriorityQueue.cpp
  llvm/lib/Support/YAMLTraits.cpp
  llvm/test/CodeGen/PowerPC/pr45709.ll
  llvm/unittests/Support/YAMLIOTest.cpp
  llvm/utils/gn/secondary/clang/lib/StaticAnalyzer/Core/BUILD.gn

Index: llvm/utils/gn/secondary/clang/lib/StaticAnalyzer/Core/BUILD.gn
===
--- llvm/utils/gn/secondary/clang/lib/StaticAnalyzer/Core/BUILD.gn
+++ llvm/utils/gn/secondary/clang/lib/StaticAnalyzer/Core/BUILD.gn
@@ -55,6 +55,7 @@
 "SimpleConstraintManager.cpp",
 "SimpleSValBuilder.cpp",
 "Store.cpp",
+"SubEngine.cpp",
 "SymbolManager.cpp",
 "TextDiagnostics.cpp",
 "WorkList.cpp",
Index: llvm/unittests/Support/YAMLIOTest.cpp
===
--- llvm/unittests/Support/YAMLIOTest.cpp
+++ llvm/unittests/Support/YAMLIOTest.cpp
@@ -333,6 +333,7 @@
   uint16_tu16;
   uint8_t u8;
   boolb;
+  charc;
   int64_t s64;
   int32_t s32;
   int16_t s16;
@@ -357,6 +358,7 @@
   io.mapRequired("u16",  bt.u16);
   io.mapRequired("u8",   bt.u8);
   io.mapRequired("b",bt.b);
+  io.mapRequired("c",bt.c);
   io.mapRequired("s64",  bt.s64);
   io.mapRequired("s32",  bt.s32);
   io.mapRequired("s16",  bt.s16);
@@ -386,6 +388,7 @@
 "u16:  65000\n"
 "u8:   255\n"
 "b:false\n"
+"c:'c'\n"
 "s64:  -50\n"
 "s32:  -20\n"
 "s16:  -32000\n"
@@ -396,7 +399,7 @@
 "h16:  0x8765\n"
 "h32:  0xFEDCBA98\n"
 "h64:  0xFEDCBA9876543210\n"
-   "...\n");
+"...\n");
   yin >> map;
 
   EXPECT_FALSE(yin.error());
@@ -407,6 +410,7 @@
   EXPECT_EQ(map.u16, 65000);
   EXPECT_EQ(map.u8,  255);
   EXPECT_EQ(map.b,   false);
+  EXPECT_EQ(map.c,   'c');
   EXPECT_EQ(map.s64, -50LL);
   EXPECT_EQ(map.s32, -20L);
   EXPECT_EQ(map.s16, -32000);
@@ -434,6 +438,7 @@
 map.u16 = 5;
 map.u8  = 254;
 map.b   = true;
+map.c   = 'd';
 map.s64 = -60LL;
 map.s32 = -20;
 map.s16 = -32000;
@@ -463,6 +468,7 @@
 EXPECT_EQ(map.u16,  5);
 EXPECT_EQ(map.u8,   254);
 EXPECT_EQ(map.b,true);
+EXPECT_EQ(map.c,'d');
 EXPECT_EQ(map.s64,  -60LL);
 EXPECT_EQ(map.s32,  -20L);
 EXPECT_EQ(map.s16,  -32000);
Index: llvm/test/CodeGen/PowerPC/pr45709.ll
===
--- llvm/test/CodeGen/PowerPC/pr45709.ll
+++ llvm/test/CodeGen/PowerPC/pr45709.ll
@@ -10,37 +10,30 @@
 define dso_local void @_ZN1a1bEv(<4 x float> %in) local_unnamed_addr #0 align 2 {
 ; CHECK-LABEL: _ZN1a1bEv:
 ; CHECK:   # %bb.0:
-; CHECK-NEXT:bc 12, 4*cr5+lt, .LBB0_6
-; CHECK-NEXT:b .LBB0_1
-; CHECK-NEXT:  .LBB0_1: # %.preheader
-; CHECK-NEXT:b .LBB0_2
-; CHECK-NEXT:  .LBB0_2:
-; CHECK-NEXT:b .LBB0_3
-; CHECK-NEXT:  .LBB0_3:

[PATCH] D80522: [Analyzer] [NFC] Parameter Regions -- Alternative Approach

2020-05-26 Thread Artem Dergachev via Phabricator via cfe-commits 
NoQ added a comment.

Nice, looks like you managed to reuse most of the code. I still feel like we 
should ditch `DeclRegion` entirely (forcing its ~5 current users consider its 
specific variants separately) but i don't insist.




Comment at: clang/include/clang/StaticAnalyzer/Checkers/SValExplainer.h:232-233
+OS << Index;
+if (Index % 10 == 1 && Index != 11)
+  OS << "st ";
+else if (Index % 10 == 2 && Index != 12)

There's a standard function for that. I never remember what it's called, 
something something numeric something plural something suffix, but it's there.



Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp:539-540
   (void)VV;
-  assert(cast(VV.castAs().getRegion())
- ->getStackFrame()->getParent()
- ->getStackFrame() == LC->getStackFrame());
+  if (const auto *VR =
+  dyn_cast(VV.castAs().getRegion())) {
+assert(VR->getStackFrame()->getParent()

Why did you relax this `cast<>` to `dyn_cast<>`?



Comment at: clang/lib/StaticAnalyzer/Core/MemRegion.cpp:183
+  assert (getDecl() &&
+  "`ParamVarRegion` does not support functions without `Decl`");
+  return getDecl()->getType();

It will eventually have to. Maybe `"...not implemented yet"?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80522/new/

https://reviews.llvm.org/D80522



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D80522: [Analyzer] [NFC] Parameter Regions -- Alternative Approach

2020-05-25 Thread Balogh , Ádám via Phabricator via cfe-commits 
baloghadamsoftware created this revision.
baloghadamsoftware added reviewers: NoQ, Szelethus, martong, balazske.
baloghadamsoftware added a project: clang.
Herald added subscribers: ASDenysPetrov, steakhal, Charusso, gamesh411, dkrupp, 
donat.nagy, mikhail.ramalho, a.sidorin, rnkovacs, szepet, xazax.hun, 
whisperity, mgorny.

Currently, parameters of functions without their definition present cannot be 
represented as regions because it would be difficult to ensure that the same 
declaration is used in every case. To overcome this, we split `VarRegion` to 
two subclasses: `NonParamVarRegion` and `ParamVarRegion`. The latter does not 
store the `Decl` of the parameter variable. Instead it stores the index of the 
parameter which enables retrieving the actual `Decl` every time using the 
function declaration of the stack frame. To achieve this we also removed 
storing of `Decl` from `DeclRegion` and made `getDecl()` pure virtual. The 
individual `Decl`s are stored in the appropriate subclasses, such as 
`FieldRegion`, `ObjCIvarRegion` and the newly introduced `NonParamVarRegion`.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D80522

Files:
  clang/include/clang/StaticAnalyzer/Checkers/SValExplainer.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/Regions.def
  clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
  clang/lib/StaticAnalyzer/Core/CallEvent.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
  clang/lib/StaticAnalyzer/Core/MemRegion.cpp
  clang/lib/StaticAnalyzer/Core/Store.cpp
  clang/test/Analysis/explain-svals.c
  clang/test/Analysis/explain-svals.cpp
  clang/test/Analysis/explain-svals.m
  clang/unittests/StaticAnalyzer/CMakeLists.txt
  clang/unittests/StaticAnalyzer/ParamRegionTest.cpp

Index: clang/unittests/StaticAnalyzer/ParamRegionTest.cpp
===
--- /dev/null
+++ clang/unittests/StaticAnalyzer/ParamRegionTest.cpp
@@ -0,0 +1,92 @@
+//===- unittests/StaticAnalyzer/ParamRegionTest.cpp ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "Reusables.h"
+
+#include "clang/Tooling/Tooling.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace ento {
+namespace {
+
+class ParamRegionTestConsumer : public ExprEngineConsumer {
+  void performTest(const Decl *D) {
+StoreManager  = Eng.getStoreManager();
+MemRegionManager  = StMgr.getRegionManager();
+const StackFrameContext *SFC =
+Eng.getAnalysisDeclContextManager().getStackFrame(D);
+
+if (const auto *FD = dyn_cast(D)) {
+  for (const auto *P : FD->parameters()) {
+const TypedValueRegion *Reg = MRMgr.getVarRegion(P, SFC);
+if (SFC->inTopFrame())
+  assert(isa(Reg));
+else
+  assert(isa(Reg));
+  }
+} else if (const auto *CD = dyn_cast(D)) {
+  for (const auto *P : CD->parameters()) {
+const TypedValueRegion *Reg = MRMgr.getVarRegion(P, SFC);
+if (SFC->inTopFrame())
+  assert(isa(Reg));
+else
+  assert(isa(Reg));
+  }
+} else if (const auto *MD = dyn_cast(D)) {
+  for (const auto *P : MD->parameters()) {
+const TypedValueRegion *Reg = MRMgr.getVarRegion(P, SFC);
+if (SFC->inTopFrame())
+  assert(isa(Reg));
+else
+  assert(isa(Reg));
+  }
+}
+  }
+
+public:
+  ParamRegionTestConsumer(CompilerInstance ) : ExprEngineConsumer(C) {}
+
+  bool HandleTopLevelDecl(DeclGroupRef DG) override {
+for (const auto *D : DG) {
+  performTest(D);
+}
+return true;
+  }
+};
+
+class ParamRegionTestAction : public ASTFrontendAction {
+public:
+  std::unique_ptr CreateASTConsumer(CompilerInstance ,
+ StringRef File) override {
+return std::make_unique(Compiler);
+  }
+};
+
+TEST(ParamRegion, ParamRegionTest) {
+  EXPECT_TRUE(tooling::runToolOnCode(
+  std::make_unique(),
+  "void foo(int n) { "
+  "auto lambda = [n](int m) { return n + m; }; "
+  "int k = lambda(2); } "
+  "void bar(int l) { foo(l); }"
+  "struct S { int n; S(int nn): n(nn) {} };"
+  "void baz(int p) { S s(p); }"));
+  EXPECT_TRUE(tooling::runToolOnCode(
+  std::make_unique(),
+  "@interface O \n"
+  "+alloc; \n"
+

[PATCH] D80522: [Analyzer] [NFC] Parameter Regions -- Alternative Approach

2020-05-25 Thread Balogh , Ádám via Phabricator via cfe-commits 
baloghadamsoftware added a comment.

This is an alternative approach to D79704 . 
The advantage is that we must not duplicate code everywhere where `VarRegion` 
(and sometimes also `DeclRegion`) is used. The drawback is that after we 
implement handling of parameters of functions without `Decl` (paramters of 
unknown functions passed by pointer) the `getDecl()` method may return 
`nullptr`. We must handle this case everywhere in the code where `VarRegion` or 
`DeclRegion` is used and parameters of such functions may appear. (We should do 
this in the original apprach as well, but there it is more difficult to forget 
such a place because `ParamRegion` is used explicitly. Furthermore, where 
handling of `ParamRegion` is completely forgotten we usually do not crash, but 
if we forget to check the result of `getDecl()` for `nullptr` we get null 
pointer dereferences.)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80522/new/

https://reviews.llvm.org/D80522



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits