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

[Balogh , Ádám via Phabricator via cfe-commits]

baloghadamsoftware abandoned this revision.
baloghadamsoftware added a comment.

Superseded by D80522 .


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

https://reviews.llvm.org/D79704



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

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

[Balogh , Ádám via Phabricator via cfe-commits]

baloghadamsoftware added a comment.

Alternative approach is D80522 .


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

https://reviews.llvm.org/D79704



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

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

[Balogh , Ádám via Phabricator via cfe-commits]

baloghadamsoftware updated this revision to Diff 265928.
baloghadamsoftware marked an inline comment as done.
baloghadamsoftware added a comment.

`FIXME` and assertions added.


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

https://reviews.llvm.org/D79704

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/include/clang/StaticAnalyzer/Core/PathSensitive/Store.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h
  clang/lib/StaticAnalyzer/Checkers/NSErrorChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp
  
clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp
  clang/lib/StaticAnalyzer/Checkers/ValistChecker.cpp
  clang/lib/StaticAnalyzer/Core/BugReporterVisitors.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/RegionStore.cpp
  clang/lib/StaticAnalyzer/Core/Store.cpp
  clang/lib/StaticAnalyzer/Core/SymbolManager.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.getRegionForParam(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.getRegionForParam(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.getRegionForParam(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"
+  "-initWithInt:(int)q; \n"
+  "@end \n"
+  "void qix(int r) { O *o = [[O alloc] initWithInt:r]; }",
+  "input.m"));
+}
+
+} // namespace
+} // namespace ento
+} // namespace clang
Index: clang/unittests/StaticAnalyzer/CMakeLists.txt
===
--- 

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

[Balogh , Ádám via Phabricator via cfe-commits]

baloghadamsoftware added a comment.

The big question to decide: Either we keep `ParamRegion` as a separate region 
in the class hierarchy and at the few places where `DeclRegion` or `VarRegion` 
is used and parameters are possible we duplicate the few lines. (Current 
status.)

The other way is to remove `Decl` from `DeclRegion` and make `getDecl()` pure 
virtual. Also we split `VarRegion` into two subclasses, one of the is the 
non-parameter variable regions and the other is `ParamVarRegion`. The first one 
stores the `Decl`, the second one calculates it. This saves code duplication, 
however after we implement creation of stack frames for calls without `Decl` we 
must update all the code using `DeclRegion` and `VarRegion` where parameters 
are possible to handle `nullptr` as return value for `getDecl()`.

@NoQ, please decide, which way to go.


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

https://reviews.llvm.org/D79704



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

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

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

Thanks for addressing my comments!




Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h:309
+  /// Get the lvalue for a parameter.
+  Loc getLValue(const Expr *Call, unsigned Index,
+const LocationContext *LC) const;

baloghadamsoftware wrote:
> martong wrote:
> > Is this used anywhere in this patch?
> > 
> > And why isn't it a `CallExpr` (instead of `Expr`)?
> The origin expression of a call may be a set of different kinds of 
> expressions: `CallExpr`, `CXXConstructExpr`, `BlockExpr` or `ObjCMessageExpr`.
Is this function used anywhere in this patch? Could not find any reference to 
it.

> The origin expression of a call may be a set of different kinds of 
> expressions: CallExpr, CXXConstructExpr, BlockExpr or ObjCMessageExpr.

Okay, makes sense, could you please document it then in the comment for the 
function? And perhaps we should assert on these kinds as a sanity check.



Comment at: clang/lib/StaticAnalyzer/Core/MemRegion.cpp:915
+return cast(I.getCapturedRegion());
+} else if (const auto *PR = dyn_cast(OrigR)) {
+  if (PR->getDecl() == VD)

baloghadamsoftware wrote:
> martong wrote:
> > Both the `VarRegion` and `ParamRegion` cases here do the same.
> > This suggest that maybe it would be better to have `VarRegion` as a base 
> > class for `ParamVarRegion`.
> > This is aligned to what Artem suggested:
> > > We can even call it VarRegion and have sub-classes be ParamVarRegion and 
> > > NonParamVarRegion or something like that.
> > But maybe `NonParamVarRegion` is not needed since that is actually the 
> > `VarRegion`.
> Usually we do not inherit from concrete classes by changing a method 
> implementation that already exist. The other reason is even stronger: 
> `NonParamVarRegion` //must// store the `Decl` of the variable, 
> `ParamVarRegion` //must not//.
> Usually we do not inherit from concrete classes by changing a method 
> implementation that already exist.

That's what we exactly do all over the place in the AST library.
Which method(s) should we change by the way?

Nevermind, I am perfectly fine having VarRegion as base and NonParamVarRegion 
and ParamVarRegion as derived.




Comment at: clang/lib/StaticAnalyzer/Core/MemRegion.cpp:1230
+
+const TypedValueRegion*
+MemRegionManager::getRegionForParam(const ParmVarDecl *PVD,

baloghadamsoftware wrote:
> martong wrote:
> > Why the return type is not `ParamVarRegion*`?
> Because for top-level functions and captured parameters it still returns 
> `VarRegion`.
Okay, makes sense, could you please document it then in the comment for the 
function?


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

https://reviews.llvm.org/D79704



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

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

[Balogh , Ádám via Phabricator via cfe-commits]

baloghadamsoftware marked 7 inline comments as done.
baloghadamsoftware added a comment.

In D79704#2042130 , @martong wrote:

> Can we find the FunctionDecl if the call happens through a function pointer? 
> Or in that case do we actually find the VarDecl of the function pointer?


Yes, if it has a declaration, then we can retrieve it from the stack frame.

> So, in this patch, we are trying to solve only that problem when the callee 
> `FunctionDecl` does not have a definition?

Not even that. This  patch is just a non-functional change that prepares the 
ground for such improvements. The next patch intends to solve them problem 
where the function has declaration but no definition.

> This happens in case of top-level params and in case of function pointers, 
> plus the special argument construction, am I right?

Top-level is a special case: there we do not have `Expr` either so we cannot 
handle it. For top-level functions the regions remain `VarRegions`. There is 
nothing special in parameters when analyzed top-level.

> Based on the answers to the above questions, possibly we are trying to solve 
> two problems here. Could we split then this patch into two?
> 
> 1. callee FunctionDecl does not have a definition. This could assert on 
> having an accessable Decl in ParamVarRegion. Here we could have the 
> foundations of the next patch, i.e. we could have ParamVarRegion and 
> NonParamVarRegion.
> 2. function pointers, plus the special argument construction: ParamVarRegion 
> may not have a Decl and we'd remove the related lines from 
> getCalleeAnalysisDeclContext. This obviously requires more tests. And more 
> brainstorming to get the AnalysisDeclContext when there is no Decl available 
> for the function in getCalleeAnalysisDeclContext.

That is exactly my suggestion, but not this, current patch, but the next one. 
The next patch is for functions without definition but declarations, and there 
could be another one in the future for functions without declaration. Of course 
this latter requires some extra code into `ParamRegion`s to handle cases where 
we do not have a `Decl`. I cannot write it now because I cannot make a test for 
it, not even a unit test.

In D79704#2046495 , @martong wrote:

> Seems like many changes could be simplified or absolutely not needed in this 
> patch if ParamRegion (or with a better name ParamVarRegion) was derived from 
> VarRegion. Is there any difficulties in that derivation?


It is one of my suggestions, I am still waiting for @NoQ's opinion: let us 
remove the `Decl` from `DeclRegion` and make `getDecl()` pure virtual there. 
Also not implement it in `VarRegion`, but as @NoQ suggested, create something 
like `PlainVarRegion` or `NonParamVarRegion` where we store it, and 
`ParamVarRegion` where we retrieve it instead of storing it.




Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h:309
+  /// Get the lvalue for a parameter.
+  Loc getLValue(const Expr *Call, unsigned Index,
+const LocationContext *LC) const;

martong wrote:
> Is this used anywhere in this patch?
> 
> And why isn't it a `CallExpr` (instead of `Expr`)?
The origin expression of a call may be a set of different kinds of expressions: 
`CallExpr`, `CXXConstructExpr`, `BlockExpr` or `ObjCMessageExpr`.



Comment at: clang/lib/StaticAnalyzer/Core/MemRegion.cpp:195
+if (Index >= FD->param_size())
+  return nullptr;
+

martong wrote:
> In all other cases we `assert`, here we return with a nullptr. Why?
It was accidentally left here from an earlier experiment.



Comment at: clang/lib/StaticAnalyzer/Core/MemRegion.cpp:915
+return cast(I.getCapturedRegion());
+} else if (const auto *PR = dyn_cast(OrigR)) {
+  if (PR->getDecl() == VD)

martong wrote:
> Both the `VarRegion` and `ParamRegion` cases here do the same.
> This suggest that maybe it would be better to have `VarRegion` as a base 
> class for `ParamVarRegion`.
> This is aligned to what Artem suggested:
> > We can even call it VarRegion and have sub-classes be ParamVarRegion and 
> > NonParamVarRegion or something like that.
> But maybe `NonParamVarRegion` is not needed since that is actually the 
> `VarRegion`.
Usually we do not inherit from concrete classes by changing a method 
implementation that already exist. The other reason is even stronger: 
`NonParamVarRegion` //must// store the `Decl` of the variable, `ParamVarRegion` 
//must not//.



Comment at: clang/lib/StaticAnalyzer/Core/MemRegion.cpp:1230
+
+const TypedValueRegion*
+MemRegionManager::getRegionForParam(const ParmVarDecl *PVD,

martong wrote:
> Why the return type is not `ParamVarRegion*`?
Because for top-level functions and captured parameters it still returns 
`VarRegion`.

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

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

Seems like many changes could be simplified or absolutely not needed in this 
patch if ParamRegion (or with a better name ParamVarRegion) was derived from 
VarRegion. Is there any difficulties in that derivation?




Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h:309
+  /// Get the lvalue for a parameter.
+  Loc getLValue(const Expr *Call, unsigned Index,
+const LocationContext *LC) const;

Is this used anywhere in this patch?

And why isn't it a `CallExpr` (instead of `Expr`)?



Comment at: 
clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp:519
+  return false; // CleanupAttr attaches destructors, which cause escaping.
+  } else {
+const ParmVarDecl *PVD = PR->getDecl();

Again, ParamRegion should derive from VarRegion.



Comment at: 
clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp:439
+VD = cast(VR->getDecl());
+  } else if (const auto *PR =
+ dyn_cast(cast(Sym)->getRegion())) 
{

This would be simpler  (noop?) again if ParamRegion was derived from VarRegion.



Comment at: 
clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp:618
 return std::string(VR->getDecl()->getName());
+  if (const auto *PR = dyn_cast_or_null(MR))
+return std::string(PR->getDecl()->getName());

What if ParamRegion was derived from VarRegion ?



Comment at: clang/lib/StaticAnalyzer/Core/MemRegion.cpp:195
+if (Index >= FD->param_size())
+  return nullptr;
+

In all other cases we `assert`, here we return with a nullptr. Why?



Comment at: clang/lib/StaticAnalyzer/Core/MemRegion.cpp:915
+return cast(I.getCapturedRegion());
+} else if (const auto *PR = dyn_cast(OrigR)) {
+  if (PR->getDecl() == VD)

Both the `VarRegion` and `ParamRegion` cases here do the same.
This suggest that maybe it would be better to have `VarRegion` as a base class 
for `ParamVarRegion`.
This is aligned to what Artem suggested:
> We can even call it VarRegion and have sub-classes be ParamVarRegion and 
> NonParamVarRegion or something like that.
But maybe `NonParamVarRegion` is not needed since that is actually the 
`VarRegion`.



Comment at: clang/lib/StaticAnalyzer/Core/MemRegion.cpp:1230
+
+const TypedValueRegion*
+MemRegionManager::getRegionForParam(const ParmVarDecl *PVD,

Why the return type is not `ParamVarRegion*`?



Comment at: clang/lib/StaticAnalyzer/Core/SymbolManager.cpp:580
+
+bool SymbolReaper::isLive(const ParamRegion *PR,
+  bool includeStoreBindings) const{

This is almost the copy-paste code for isLive(VarRegion). This again suggests 
that maybe ParamRegion (or ParamVarRegion?) should be derived from VarRegion. 
And in isLive(VarRegion) we should dyn_cast to ParamVarRegion and do the extra 
stuff if needed.


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

https://reviews.llvm.org/D79704



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

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

[Balogh , Ádám via Phabricator via cfe-commits]

baloghadamsoftware updated this revision to Diff 264979.
baloghadamsoftware added a comment.

SVal explanation extended and tests added for it.


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

https://reviews.llvm.org/D79704

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/include/clang/StaticAnalyzer/Core/PathSensitive/Store.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h
  clang/lib/StaticAnalyzer/Checkers/NSErrorChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp
  
clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp
  clang/lib/StaticAnalyzer/Checkers/ValistChecker.cpp
  clang/lib/StaticAnalyzer/Core/BugReporterVisitors.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/RegionStore.cpp
  clang/lib/StaticAnalyzer/Core/Store.cpp
  clang/lib/StaticAnalyzer/Core/SymbolManager.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.getRegionForParam(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.getRegionForParam(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.getRegionForParam(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"
+  "-initWithInt:(int)q; \n"
+  "@end \n"
+  "void qix(int r) { O *o = [[O alloc] initWithInt:r]; }",
+  "input.m"));
+}
+
+} // namespace
+} // namespace ento
+} // namespace clang
Index: clang/unittests/StaticAnalyzer/CMakeLists.txt
===
--- 

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

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

Wow, this is some nice work and huge effort from all the participants, it was 
pretty informative to read through. Thanks @baloghadamsoftware and @NoQ for 
working on this!

> This means that we always found a Decl. However, this Decl is not stored but 
> retrieved always based on the actual stack frame: we get the Decl of the 
> function/method/block/etc. from the stack frame and find its parameter using 
> the Index. This is always successful, at least in all the analyzer tests.

Can we find the FunctionDecl if the call happens through a function pointer? Or 
in that case do we actually find the VarDecl of the function pointer?

> Without stack frame we do not create ParamRegion. The other two functions 
> creating ParamRegion (CallEvent::addParameterValuesToBindings and the newly 
> created MemRegionManager::getRegionForParam) start from ParmVarDecl which 
> always belongs to a Decl. So we can safely assume that currently all 
> parameter regions have a Decl. Of course, this can be changed in the future, 
> but I must not include dead code in a patch that cannot even be tested in the 
> current phase. Even creation of callee stack frame for functions without 
> definition is not part of this patch, but of the subsequent one.

So, in this patch, we are trying to solve only that problem when the callee 
`FunctionDecl` does not have a definition?

> If Decl is missing, then we cannot get its AnalysisDeclContext either, so how 
> can we get a stack frame? However, currently we need a solution where we can 
> get stack frame for callees with Decl but without definition.

This happens in case of top-level params and in case of function pointers, plus 
the special argument construction, am I right?

Based on the answers to the above questions, possibly we are trying to solve 
two problems here. Could we split then this patch into two?

1. callee FunctionDecl does not have a definition. This could assert on having 
an accessable Decl in ParamVarRegion. Here we could have the foundations of the 
next patch, i.e. we could have ParamVarRegion and NonParamVarRegion.
2. function pointers, plus the special argument construction: ParamVarRegion 
may not have a Decl and we'd remove the related lines from 
getCalleeAnalysisDeclContext. This obviously requires more tests. And more 
brainstorming to get the AnalysisDeclContext when there is no Decl available 
for the function in getCalleeAnalysisDeclContext.

What do you think?




Comment at: clang/lib/StaticAnalyzer/Checkers/ValistChecker.cpp:176-180
   if (const auto *DeclReg = Reg->getAs()) {
 if (isa(DeclReg->getDecl()))
   Reg = C.getState()->getSVal(SV.castAs()).getAsRegion();
+  } else if (const auto *ParamReg = Reg->getAs()) {
+Reg = C.getState()->getSVal(SV.castAs()).getAsRegion();

balazske wrote:
> baloghadamsoftware wrote:
> > balazske wrote:
> > > baloghadamsoftware wrote:
> > > > Szelethus wrote:
> > > > > This is interesting. I looked up `DeclRegion`, and it seems to be the 
> > > > > region that is tied to a `ValueDecl`. `VarDecl` is a subtype of 
> > > > > `ValueDecl`, and `ParmVarDecl` is a subtype of `VarDecl`, so wouldn't 
> > > > > it make sense for `ParamRegion` to be a subtype of `VarRegion`?
> > > > `DeclRegion` stores the `Decl`, `ParamRegion` retrieves it based on the 
> > > > `Index` it stores. There is no is-a relation between them.
> > > During the lifetime of a `ParamRegion` is it possible that it will return 
> > > different `Decl` objects?
> > @NoQ?
> Purpose of the question is that if the answer is "no", the decl could be 
> stored at construction of a `ParamRegion` and it could be a subclass of 
> `DeclRegion`. From the code I do not see that the belonging `Decl` can 
> change, probably yes if the stack frame changes somehow. So the reason to 
> have the `ParamRegion` seems to be to have the expression and index 
> available, maybe use these for profiling (instead of a `Decl`). Still it 
> could be made a subclass of `DeclRegion` that stores additionally the expr 
> and index and profiling works with these values.
> During the lifetime of a ParamRegion is it possible that it will return 
> different Decl objects?

AFAIU, Yes. The `CallExpr` may refer to a `FunctionDecl` (the callee), but that 
Decl may not be the Canonical Decl of the redeclaration chain.



Comment at: clang/lib/StaticAnalyzer/Core/MemRegion.cpp:191
+const ParmVarDecl *ParamRegion::getDecl() const {
+  const Decl *D = getStackFrame()->getDecl();
+

baloghadamsoftware wrote:
> NoQ wrote:
> > baloghadamsoftware wrote:
> > > NoQ wrote:
> > > > baloghadamsoftware wrote:
> > > > > NoQ wrote:
> > > > > > baloghadamsoftware wrote:
> > > > > > > NoQ wrote:
> > > > > > > > This doesn't work when the callee is unknown.
> > > > > > > Please give me an example where the callee is unknown. As I 
> > > > > > > wrote, originally I put a `getExpr()` method here as well and 
> > > > > 

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

[Balogh , Ádám via Phabricator via cfe-commits]

baloghadamsoftware added a comment.

In D79704#2040798 , @NoQ wrote:

> > It turns out that parameter regions are never created for functions without 
> > `Decl` because of the first lines in 
> > `CallEvent::getCalleeAnalysisDeclContext()`.
>
> Removing these lines is more or less the whole point of your work.


Anyway, when we remove it in a later patch, what should we use for stack frame 
of the callee? Currently we retrieve the `AnalysisDeclContext` for the `Decl` 
and then get a stack frame for it. If `Decl` is missing, then we cannot get its 
`AnalysisDeclContext` either, so how can we get a stack frame? However, 
currently we need a solution where we can get stack frame for callees with 
`Decl` but without definition. Since you wrote that this causes crashes because 
at different points different `Decl`s of the same `ParmVarDecl` are used we 
created `ParamRegion` that does not store the `Decl` but dynamically retrieves 
it based on the contents of the actual stack frame.


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

https://reviews.llvm.org/D79704



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

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

[Balogh , Ádám via Phabricator via cfe-commits]

baloghadamsoftware updated this revision to Diff 264598.
baloghadamsoftware added a comment.

Unit test updated to cover all cases.


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

https://reviews.llvm.org/D79704

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/include/clang/StaticAnalyzer/Core/PathSensitive/Store.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h
  clang/lib/StaticAnalyzer/Checkers/NSErrorChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp
  
clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp
  clang/lib/StaticAnalyzer/Checkers/ValistChecker.cpp
  clang/lib/StaticAnalyzer/Core/BugReporterVisitors.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/RegionStore.cpp
  clang/lib/StaticAnalyzer/Core/Store.cpp
  clang/lib/StaticAnalyzer/Core/SymbolManager.cpp
  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.getRegionForParam(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.getRegionForParam(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.getRegionForParam(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"
+  "-initWithInt:(int)q; \n"
+  "@end \n"
+  "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
@@ -6,6 +6,7 @@
   AnalyzerOptionsTest.cpp
   

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

[Balogh , Ádám via Phabricator via cfe-commits]

baloghadamsoftware marked an inline comment as done.
baloghadamsoftware added a comment.

In D79704#2040798 , @NoQ wrote:

> > It turns out that parameter regions are never created for functions without 
> > `Decl` because of the first lines in 
> > `CallEvent::getCalleeAnalysisDeclContext()`.
>
> Removing these lines is more or less the whole point of your work.


Is it? The point of my work is to avoid the crashes you mentioned in 
D49443#1193290  by making the regions 
of the parameters independent of their declarations so we can remove the 
limitation that callee's stack frame is only created for functions with 
definition. (Can you provide me with examples which the crashes you mentioned 
with `VarRegions`? I need them for testing my solution.) It could be a future 
step to also remove the limitation that also allows getting the region for 
parameters without declaration. Even removing the original limitation (creation 
of stack frame without definition) is part of my planned subsequent patch, not 
this one. I am trying to follow incremental development to avoid too huge 
patches.




Comment at: clang/lib/StaticAnalyzer/Core/MemRegion.cpp:191
+const ParmVarDecl *ParamRegion::getDecl() const {
+  const Decl *D = getStackFrame()->getDecl();
+

NoQ wrote:
> baloghadamsoftware wrote:
> > NoQ wrote:
> > > baloghadamsoftware wrote:
> > > > NoQ wrote:
> > > > > baloghadamsoftware wrote:
> > > > > > NoQ wrote:
> > > > > > > This doesn't work when the callee is unknown.
> > > > > > Please give me an example where the callee is unknown. As I wrote, 
> > > > > > originally I put a `getExpr()` method here as well and `getType()` 
> > > > > > fell back to it if it could not find the `Decl()`. However, it was 
> > > > > > never invoked on the whole test suite. (I put an `assert(false)` 
> > > > > > into it, and did not get a crash.
> > > > > > Please give me an example where the callee is unknown.
> > > > > 
> > > > > >>! In D79704#2034571, @NoQ wrote:
> > > > > >>>! In D79704#2032947, @Szelethus wrote:
> > > > > >> Could you give a specific code example? 
> > > > > > 
> > > > > > ```lang=c++
> > > > > > struct S {
> > > > > >   S() {
> > > > > > this; // What region does 'this' point to...
> > > > > >   }
> > > > > > };
> > > > > > 
> > > > > > void foo(void (*bar)(S)) {
> > > > > >   bar(S()); // ...in this invocation?
> > > > > > }
> > > > > > ```
> > > > OK, but it still does not crash the analyzer, even if I enable creation 
> > > > of stack frames for all the callees, even for those without definition. 
> > > > What else should I do to enforce the crash (null pointer dereference)? 
> > > > Try to use `getParameterLocation()` in a unit test?
> > > Yes. I demonstrated how you can get the region without a decl but you 
> > > should turn this into a test that actually calls one of the problematic 
> > > functions. Like, `clang_analyzer_dump()` it or something.
> > Of  course I tried `clang_analyzer_explain()`, but neither 
> > `clang_analyzer_dump()` helps. I also tried a unit test now where I call 
> > `getParameterLocation()` explicitly. It turns out that parameter regions 
> > are never created for functions without `Decl` because of the first lines 
> > in `CallEvent::getCalleeAnalysisDeclContext()`. This function //needs// the 
> > `Decl` to retrieve the `AnalysisDeclContext` of the callee, which is needed 
> > to retrieve its stack  frame by `getCalleeStackFrame()`. Without stack 
> > frame we do not create `ParamRegion`. The other two functions creating 
> > `ParamRegion` (`CallEvent::addParameterValuesToBindings` and the newly 
> > created `MemRegionManager::getRegionForParam`) start from `ParmVarDecl` 
> > which always belongs to a `Decl`. So we can safely assume that currently 
> > all parameter regions have a `Decl`. Of course, this can be changed in the 
> > future, but I must not include dead code in a patch that cannot even be 
> > tested in the current phase. Even creation of callee stack frame for 
> > functions without //definition// is not part of this patch, but of the 
> > subsequent one.
> > neither `clang_analyzer_dump()` helps
> 
> So, does it dump a parameter region? Because it obviously should.
It dumps a temporary region as until now. So this patch does not change the 
behavior of the analyzer at this point.


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

https://reviews.llvm.org/D79704



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

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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

> It turns out that parameter regions are never created for functions without 
> `Decl` because of the first lines in 
> `CallEvent::getCalleeAnalysisDeclContext()`.

Removing these lines is more or less the whole point of your work.


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

https://reviews.llvm.org/D79704



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

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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: clang/lib/StaticAnalyzer/Core/MemRegion.cpp:191
+const ParmVarDecl *ParamRegion::getDecl() const {
+  const Decl *D = getStackFrame()->getDecl();
+

baloghadamsoftware wrote:
> NoQ wrote:
> > baloghadamsoftware wrote:
> > > NoQ wrote:
> > > > baloghadamsoftware wrote:
> > > > > NoQ wrote:
> > > > > > This doesn't work when the callee is unknown.
> > > > > Please give me an example where the callee is unknown. As I wrote, 
> > > > > originally I put a `getExpr()` method here as well and `getType()` 
> > > > > fell back to it if it could not find the `Decl()`. However, it was 
> > > > > never invoked on the whole test suite. (I put an `assert(false)` into 
> > > > > it, and did not get a crash.
> > > > > Please give me an example where the callee is unknown.
> > > > 
> > > > >>! In D79704#2034571, @NoQ wrote:
> > > > >>>! In D79704#2032947, @Szelethus wrote:
> > > > >> Could you give a specific code example? 
> > > > > 
> > > > > ```lang=c++
> > > > > struct S {
> > > > >   S() {
> > > > > this; // What region does 'this' point to...
> > > > >   }
> > > > > };
> > > > > 
> > > > > void foo(void (*bar)(S)) {
> > > > >   bar(S()); // ...in this invocation?
> > > > > }
> > > > > ```
> > > OK, but it still does not crash the analyzer, even if I enable creation 
> > > of stack frames for all the callees, even for those without definition. 
> > > What else should I do to enforce the crash (null pointer dereference)? 
> > > Try to use `getParameterLocation()` in a unit test?
> > Yes. I demonstrated how you can get the region without a decl but you 
> > should turn this into a test that actually calls one of the problematic 
> > functions. Like, `clang_analyzer_dump()` it or something.
> Of  course I tried `clang_analyzer_explain()`, but neither 
> `clang_analyzer_dump()` helps. I also tried a unit test now where I call 
> `getParameterLocation()` explicitly. It turns out that parameter regions are 
> never created for functions without `Decl` because of the first lines in 
> `CallEvent::getCalleeAnalysisDeclContext()`. This function //needs// the 
> `Decl` to retrieve the `AnalysisDeclContext` of the callee, which is needed 
> to retrieve its stack  frame by `getCalleeStackFrame()`. Without stack frame 
> we do not create `ParamRegion`. The other two functions creating 
> `ParamRegion` (`CallEvent::addParameterValuesToBindings` and the newly 
> created `MemRegionManager::getRegionForParam`) start from `ParmVarDecl` which 
> always belongs to a `Decl`. So we can safely assume that currently all 
> parameter regions have a `Decl`. Of course, this can be changed in the 
> future, but I must not include dead code in a patch that cannot even be 
> tested in the current phase. Even creation of callee stack frame for 
> functions without //definition// is not part of this patch, but of the 
> subsequent one.
> neither `clang_analyzer_dump()` helps

So, does it dump a parameter region? Because it obviously should.


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

https://reviews.llvm.org/D79704



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

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

[Balogh , Ádám via Phabricator via cfe-commits]

baloghadamsoftware marked an inline comment as done.
baloghadamsoftware added a comment.

In D79704#2038280 , @NoQ wrote:

> In D79704#2038257 , @Szelethus wrote:
>
> > In D79704#2037100 , @NoQ wrote:
> >
> > > > The code changes make me feel like we're doing a lot of chore (and make 
> > > > it super easy to forget checking for parameters explicitly).
> > >
> > > I wouldn't mind having a common base class for these regions. 
> > > `DeclRegion` should probably be dropped in this case, in order to avoid 
> > > dealing with multiple inheritance. And it's definitely the one that needs 
> > > to be dropped because it currently does nothing except "inheritance for 
> > > code reuse": there are basically no other common properties between its 
> > > sub-classes than the accidental code reuse in its methods.
> >
> >
> > Totally. Something like a `VarRegionBase` but with a more clever name.
>
>
> We can even call it `VarRegion` and have sub-classes be `ParamVarRegion` and 
> `NonParamVarRegion` or something like that.


Do you mean `getDecl()` should be pure virtual with different implementation 
for `ParamVarRegion` (retrieves dynamically based on its `Index`) and 
`NonParamVarRegion` (stores). However, there //are// some places in the code 
that check for `DeclRegion` and use its `getDecl()` so to avoid code 
duplication we can keep `DeclRegion`, but remove storing of `Decl` from it and 
make `getDecl()` pure virtual already at that level.




Comment at: clang/lib/StaticAnalyzer/Core/MemRegion.cpp:191
+const ParmVarDecl *ParamRegion::getDecl() const {
+  const Decl *D = getStackFrame()->getDecl();
+

NoQ wrote:
> baloghadamsoftware wrote:
> > NoQ wrote:
> > > baloghadamsoftware wrote:
> > > > NoQ wrote:
> > > > > This doesn't work when the callee is unknown.
> > > > Please give me an example where the callee is unknown. As I wrote, 
> > > > originally I put a `getExpr()` method here as well and `getType()` fell 
> > > > back to it if it could not find the `Decl()`. However, it was never 
> > > > invoked on the whole test suite. (I put an `assert(false)` into it, and 
> > > > did not get a crash.
> > > > Please give me an example where the callee is unknown.
> > > 
> > > >>! In D79704#2034571, @NoQ wrote:
> > > >>>! In D79704#2032947, @Szelethus wrote:
> > > >> Could you give a specific code example? 
> > > > 
> > > > ```lang=c++
> > > > struct S {
> > > >   S() {
> > > > this; // What region does 'this' point to...
> > > >   }
> > > > };
> > > > 
> > > > void foo(void (*bar)(S)) {
> > > >   bar(S()); // ...in this invocation?
> > > > }
> > > > ```
> > OK, but it still does not crash the analyzer, even if I enable creation of 
> > stack frames for all the callees, even for those without definition. What 
> > else should I do to enforce the crash (null pointer dereference)? Try to 
> > use `getParameterLocation()` in a unit test?
> Yes. I demonstrated how you can get the region without a decl but you should 
> turn this into a test that actually calls one of the problematic functions. 
> Like, `clang_analyzer_dump()` it or something.
Of  course I tried `clang_analyzer_explain()`, but neither 
`clang_analyzer_dump()` helps. I also tried a unit test now where I call 
`getParameterLocation()` explicitly. It turns out that parameter regions are 
never created for functions without `Decl` because of the first lines in 
`CallEvent::getCalleeAnalysisDeclContext()`. This function //needs// the `Decl` 
to retrieve the `AnalysisDeclContext` of the callee, which is needed to 
retrieve its stack  frame by `getCalleeStackFrame()`. Without stack frame we do 
not create `ParamRegion`. The other two functions creating `ParamRegion` 
(`CallEvent::addParameterValuesToBindings` and the newly created 
`MemRegionManager::getRegionForParam`) start from `ParmVarDecl` which always 
belongs to a `Decl`. So we can safely assume that currently all parameter 
regions have a `Decl`. Of course, this can be changed in the future, but I must 
not include dead code in a patch that cannot even be tested in the current 
phase. Even creation of callee stack frame for functions without //definition// 
is not part of this patch, but of the subsequent one.


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

https://reviews.llvm.org/D79704



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

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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

In D79704#2038257 , @Szelethus wrote:

> In D79704#2037100 , @NoQ wrote:
>
> > > The code changes make me feel like we're doing a lot of chore (and make 
> > > it super easy to forget checking for parameters explicitly).
> >
> > I wouldn't mind having a common base class for these regions. `DeclRegion` 
> > should probably be dropped in this case, in order to avoid dealing with 
> > multiple inheritance. And it's definitely the one that needs to be dropped 
> > because it currently does nothing except "inheritance for code reuse": 
> > there are basically no other common properties between its sub-classes than 
> > the accidental code reuse in its methods.
>
>
> Totally. Something like a `VarRegionBase` but with a more clever name.


We can even call it `VarRegion` and have sub-classes be `ParamVarRegion` and 
`NonParamVarRegion` or something like that.


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

https://reviews.llvm.org/D79704



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

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

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

Szelethus added a comment.

Alright, I'm up to speed I think, cheers!

In D79704#2037100 , @NoQ wrote:

> > The code changes make me feel like we're doing a lot of chore (and make it 
> > super easy to forget checking for parameters explicitly).
>
> I wouldn't mind having a common base class for these regions. `DeclRegion` 
> should probably be dropped in this case, in order to avoid dealing with 
> multiple inheritance. And it's definitely the one that needs to be dropped 
> because it currently does nothing except "inheritance for code reuse": there 
> are basically no other common properties between its sub-classes than the 
> accidental code reuse in its methods.


Totally. Something like a `VarRegionBase` but with a more clever name.


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

https://reviews.llvm.org/D79704



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

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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

In D79704#2036581 , @Szelethus wrote:

> If something tricky like this came up, the `getDecl` function should just 
> return with a nullptr, shouldn't it?


How far are you willing to push it? For instance, are you ok with, say, 
`TypedValueRegion::getValueType()` return a null `QualType` in this single rare 
cornercase? Because that's what we'll also have to do if a `Decl` isn't 
available in `VarRegion`. Unless we add more stuff into `VarRegion`, which is 
basically what we're doing (and it just suddenly turns out that we don't need 
the `Decl` anymore).

> The code changes make me feel like we're doing a lot of chore (and make it 
> super easy to forget checking for parameters explicitly).

I wouldn't mind having a common base class for these regions. `DeclRegion` 
should probably be dropped in this case, in order to avoid dealing with 
multiple inheritance. And it's definitely the one that needs to be dropped 
because it currently does nothing except "inheritance for code reuse": there 
are basically no other common properties between its sub-classes than the 
accidental code reuse in its methods.

> The gain, which is I guess correctness, seems negligible, and I'm not sure 
> this is actually correct, as I mentioned in D79704#inline-730731 
> .

The gain is that we finally have a way to express some regions that we 
previously could not express at all. This gain isn't huge; i agree that 
@baloghadamsoftware could most likely get away without it. Apart from the 
example with no decl at all, i'm pretty excited about eliminating a lot of 
annoying ambiguities with respect to virtual method calls and function 
redeclarations that i've been struggling in D49443 
.

More importantly, this change is correct because that's how programs actually 
behave. You don't need to know everything (and a `Decl` is literally 
everything) about the function you're calling in order to call it. The calling 
convention only requires you to put arguments into certain places in memory and 
these places are entirely determined by the indices and types of the arguments. 
And that's exactly what we're trying to mimic. We technically don't even need 
the `OriginExpr` itself but it's a convenient (and so far seemingly harmless) 
way of capturing all the information that we actually need (which is, well, 
indices and types of arguments).




Comment at: clang/lib/StaticAnalyzer/Core/MemRegion.cpp:191
+const ParmVarDecl *ParamRegion::getDecl() const {
+  const Decl *D = getStackFrame()->getDecl();
+

baloghadamsoftware wrote:
> NoQ wrote:
> > baloghadamsoftware wrote:
> > > NoQ wrote:
> > > > This doesn't work when the callee is unknown.
> > > Please give me an example where the callee is unknown. As I wrote, 
> > > originally I put a `getExpr()` method here as well and `getType()` fell 
> > > back to it if it could not find the `Decl()`. However, it was never 
> > > invoked on the whole test suite. (I put an `assert(false)` into it, and 
> > > did not get a crash.
> > > Please give me an example where the callee is unknown.
> > 
> > >>! In D79704#2034571, @NoQ wrote:
> > >>>! In D79704#2032947, @Szelethus wrote:
> > >> Could you give a specific code example? 
> > > 
> > > ```lang=c++
> > > struct S {
> > >   S() {
> > > this; // What region does 'this' point to...
> > >   }
> > > };
> > > 
> > > void foo(void (*bar)(S)) {
> > >   bar(S()); // ...in this invocation?
> > > }
> > > ```
> OK, but it still does not crash the analyzer, even if I enable creation of 
> stack frames for all the callees, even for those without definition. What 
> else should I do to enforce the crash (null pointer dereference)? Try to use 
> `getParameterLocation()` in a unit test?
Yes. I demonstrated how you can get the region without a decl but you should 
turn this into a test that actually calls one of the problematic functions. 
Like, `clang_analyzer_dump()` it or something.


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

https://reviews.llvm.org/D79704



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

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

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

Szelethus added a comment.

In D79704#2034571 , @NoQ wrote:

> In D79704#2032947 , @Szelethus wrote:
>
> > In D79704#2032271 , @NoQ wrote:
> >
> > > Blanket reply! `ParamRegion` is not a `DeclRegion` because it does not 
> > > necessarily have a corresponding `Decl`. For instance, when calling a 
> > > function through an unknown function pointer, you don't have the 
> > > declaration of a function, let alone its parameters.
> >
> >
> > Well hold on for a second then -- how is this, if it is, different for 
> > member pointers? Aren't they represented with a `VarRegion`?
>
>
> They aren't even `Loc`!
>
> We currently don't have a representation for an unknown pointer-to-member. A 
> known pointer-to-member is represented by a `FieldDecl` (not sure about 
> static members) but it's still a `NonLoc` and as such isn't a region at all. 
> Because, well, you can't dereference a pointer-to-member; you can only apply 
> it to a base pointer like an offset. The mental model is "an offset". 
> Therefore it's `NonLoc`.
>
> Pointers to member functions are a different thing; they're function pointers 
> so they're kinda regions.
>
> >> But for parameters of C++ object type you still need your `ParamRegion` 
> >> because arguments are constructed into it.
> > 
> > Could you give a specific code example?
>
>
>
>   struct S {
> S() {
>   this; // What region does 'this' point to...
> }
>   };
>  
>   void foo(void (*bar)(S)) {
> bar(S()); // ...in this invocation?
>   }
>


I remember this coming up so many times, and I'm an inch away from getting it 
but I'm not there just yet ^-^ Sorry if you have to repeat yourself too many 
times.

The example you have shown sounds like a case we could (should?) abstract away. 
If we model in accordance to the AST as closely as possible, `ParamRegion` 
should totally be a `VarRegion`. If something tricky like this came up, the 
`getDecl` function should just return with a nullptr, shouldn't it? The code 
changes make me feel like we're doing a lot of chore (and make it super easy to 
forget checking for parameters explicitly). The gain, which is I guess 
correctness, seems negligible, and I'm not sure this is actually correct, as I 
mentioned in D79704#inline-730731 
. I don't see why the most 
defining feature of a `DeclRegion` is supposed to be an always existing `Decl`, 
rather than a mirror of the AST.


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

https://reviews.llvm.org/D79704



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

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

[Balogh , Ádám via Phabricator via cfe-commits]

baloghadamsoftware marked an inline comment as done.
baloghadamsoftware added inline comments.



Comment at: clang/lib/StaticAnalyzer/Core/MemRegion.cpp:191
+const ParmVarDecl *ParamRegion::getDecl() const {
+  const Decl *D = getStackFrame()->getDecl();
+

NoQ wrote:
> baloghadamsoftware wrote:
> > NoQ wrote:
> > > This doesn't work when the callee is unknown.
> > Please give me an example where the callee is unknown. As I wrote, 
> > originally I put a `getExpr()` method here as well and `getType()` fell 
> > back to it if it could not find the `Decl()`. However, it was never invoked 
> > on the whole test suite. (I put an `assert(false)` into it, and did not get 
> > a crash.
> > Please give me an example where the callee is unknown.
> 
> >>! In D79704#2034571, @NoQ wrote:
> >>>! In D79704#2032947, @Szelethus wrote:
> >> Could you give a specific code example? 
> > 
> > ```lang=c++
> > struct S {
> >   S() {
> > this; // What region does 'this' point to...
> >   }
> > };
> > 
> > void foo(void (*bar)(S)) {
> >   bar(S()); // ...in this invocation?
> > }
> > ```
OK, but it still does not crash the analyzer, even if I enable creation of 
stack frames for all the callees, even for those without definition. What else 
should I do to enforce the crash (null pointer dereference)? Try to use 
`getParameterLocation()` in a unit test?


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

https://reviews.llvm.org/D79704



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

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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: clang/lib/StaticAnalyzer/Core/MemRegion.cpp:191
+const ParmVarDecl *ParamRegion::getDecl() const {
+  const Decl *D = getStackFrame()->getDecl();
+

baloghadamsoftware wrote:
> NoQ wrote:
> > This doesn't work when the callee is unknown.
> Please give me an example where the callee is unknown. As I wrote, originally 
> I put a `getExpr()` method here as well and `getType()` fell back to it if it 
> could not find the `Decl()`. However, it was never invoked on the whole test 
> suite. (I put an `assert(false)` into it, and did not get a crash.
> Please give me an example where the callee is unknown.

>>! In D79704#2034571, @NoQ wrote:
>>>! In D79704#2032947, @Szelethus wrote:
>> Could you give a specific code example? 
> 
> ```lang=c++
> struct S {
>   S() {
> this; // What region does 'this' point to...
>   }
> };
> 
> void foo(void (*bar)(S)) {
>   bar(S()); // ...in this invocation?
> }
> ```


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

https://reviews.llvm.org/D79704



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

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

[Balogh , Ádám via Phabricator via cfe-commits]

baloghadamsoftware marked an inline comment as done.
baloghadamsoftware added a comment.

Good news: I executed an analysis with all open-source checks enabled for 
several open-source projects: //BitCoint//, //CURL//, //OpenSSL//, 
//PostGreS//, //TMux//, //Xerces// using the patched and the non-patched 
version of //Clang//. There is no difference in the set of bug reports. Only 
one error message became more detailed for //BitCoin// which uses //Boost//: 
instead of `Potential memory leak` I get `Potential leak of memory pointed to 
by 'Finder.m_Pred.m_Storage.m_dynSet'` in line `135` of //Boost// header 
`algorithm/string/detail/classification.hpp` and almost the same without 
`Finder.` in line 139.




Comment at: clang/lib/StaticAnalyzer/Core/MemRegion.cpp:191
+const ParmVarDecl *ParamRegion::getDecl() const {
+  const Decl *D = getStackFrame()->getDecl();
+

NoQ wrote:
> This doesn't work when the callee is unknown.
Please give me an example where the callee is unknown. As I wrote, originally I 
put a `getExpr()` method here as well and `getType()` fell back to it if it 
could not find the `Decl()`. However, it was never invoked on the whole test 
suite. (I put an `assert(false)` into it, and did not get a crash.


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

https://reviews.llvm.org/D79704



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

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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h:1051
+
+  const Expr *OriginExpr;
+  unsigned Index;

baloghadamsoftware wrote:
> NoQ wrote:
> > baloghadamsoftware wrote:
> > > We do not use this at all. However, if I remove it then tests begin to 
> > > fail with strange reasons, some of them even crashes at strange points. 
> > > It seems that we need this for profiling the subclass.
> > `Profile` is completely essential. Without it different regions will be 
> > treated as if it's the same region.
> I know, that was not the question. It is the `OriginExpr` which I tried to 
> remove because I do not use it at all, except for profiling. It seems that 
> without this field the `Profile` does not work correctly.
Wait, how is it not used? You can't get the region type without `OriginExpr`.



Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h:1054-1055
+
+  ParamRegion(const Expr *OE, unsigned Idx, const MemRegion *SReg)
+  : TypedValueRegion(SReg, ParamRegionKind), OriginExpr(OE), Index(Idx) {}
+

baloghadamsoftware wrote:
> NoQ wrote:
> > It looks like you decided to keep `VarRegion` for the top frame.
> > 
> > I want that asserted here, in the constructor, so that never to make a 
> > mistake on this front.
> Yes, because in the top frame we neither have `CallExpr` nor `Decl`. So we 
> should assert here that we are not in the top frame, right? How do we check 
> this based on these parameters?
Yes, you did what i meant. The first assertion is redundant because `isa` is 
implied by `cast`. Let's also assert that `OE` is non-null.



Comment at: clang/lib/StaticAnalyzer/Checkers/NSErrorChecker.cpp:197-198
+if (const ParamRegion *PR = R->getAs())
+  if (const StackArgumentsSpaceRegion *
+  stackReg = dyn_cast(PR->getMemorySpace()))
+if (stackReg->getStackFrame() == SFC)

baloghadamsoftware wrote:
> NoQ wrote:
> > Yes it is a `StackArgumentsSpaceRegion`! Because we're a parameter.
> So I can remove the inner branch?
In fact you can go even further and change the type of the overridden 
`getMemorySpace()` function to return `const StackArgumentsSpaceRegion *` 
("covariant return types") so that to drop the `cast` as well.



Comment at: clang/lib/StaticAnalyzer/Core/CallEvent.cpp:255
   // correspond to the stack frame's function declaration.
   assert(VR->getStackFrame() == SFC);
 

balazske wrote:
> Is it correct to create `VR` only to have this assert?
It's a valid pattern but it should be followed by `(void)VR;` in order to avoid 
unused variable warning in no-assert builds.



Comment at: clang/lib/StaticAnalyzer/Core/MemRegion.cpp:187
+QualType ParamRegion::getValueType() const {
+  return getDecl()->getType();
+}

This doesn't work when the callee is unknown. You need to consult the origin 
expr here.



Comment at: clang/lib/StaticAnalyzer/Core/MemRegion.cpp:191
+const ParmVarDecl *ParamRegion::getDecl() const {
+  const Decl *D = getStackFrame()->getDecl();
+

This doesn't work when the callee is unknown.



Comment at: clang/lib/StaticAnalyzer/Core/MemRegion.cpp:579
+  const ParmVarDecl *PVD = getDecl();
+  if (const IdentifierInfo *ID = PVD->getIdentifier()) {
+os << ID->getName();

`PVD` may be null.



Comment at: clang/lib/StaticAnalyzer/Core/MemRegion.cpp:1236
+  const Stmt *CallSite = SFC->getCallSite();
+  if (!CallSite)
+return getVarRegion(PVD, LC);

Let's make a stronger assertion:

```lang=c++
if (SFC->inTopFrame())
  return getVarRegion(PVD, LC);
assert(CallSite && "Destructors with parameters?");
```


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

https://reviews.llvm.org/D79704



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

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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

In D79704#2032947 , @Szelethus wrote:

> In D79704#2032271 , @NoQ wrote:
>
> > Blanket reply! `ParamRegion` is not a `DeclRegion` because it does not 
> > necessarily have a corresponding `Decl`. For instance, when calling a 
> > function through an unknown function pointer, you don't have the 
> > declaration of a function, let alone its parameters.
>
>
> Well hold on for a second then -- how is this, if it is, different for member 
> pointers? Aren't they represented with a `VarRegion`?


They aren't even `Loc`!

We currently don't have a representation for an unknown pointer-to-member. A 
known pointer-to-member is represented by a `FieldDecl` (not sure about static 
members) but it's still a `NonLoc` and as such isn't a region at all. Because, 
well, you can't dereference a pointer-to-member; you can only apply it to a 
base pointer like an offset. The mental model is "an offset". Therefore it's 
`NonLoc`.

Pointers to member functions are a different thing; they're function pointers 
so they're kinda regions.

>> But for parameters of C++ object type you still need your `ParamRegion` 
>> because arguments are constructed into it.
> 
> Could you give a specific code example?



  struct S {
S() {
  this; // What region does 'this' point to...
}
  };
  
  void foo(void (*bar)(S)) {
bar(S()); // ...in this invocation?
  }


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

https://reviews.llvm.org/D79704



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

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

[Balogh , Ádám via Phabricator via cfe-commits]

baloghadamsoftware updated this revision to Diff 263748.
baloghadamsoftware added a comment.
Herald added a subscriber: mgorny.

Unit test added.


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

https://reviews.llvm.org/D79704

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/include/clang/StaticAnalyzer/Core/PathSensitive/Store.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h
  clang/lib/StaticAnalyzer/Checkers/NSErrorChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp
  
clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp
  clang/lib/StaticAnalyzer/Checkers/ValistChecker.cpp
  clang/lib/StaticAnalyzer/Core/BugReporterVisitors.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/RegionStore.cpp
  clang/lib/StaticAnalyzer/Core/Store.cpp
  clang/lib/StaticAnalyzer/Core/SymbolManager.cpp
  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,83 @@
+//===- 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 (isa(D)) {
+  const auto *ParamN = findDeclByName(D, "n");
+  const auto *ParamM = findDeclByName(D, "m");
+  const TypedValueRegion *RegN = MRMgr.getRegionForParam(ParamN, SFC);
+  const TypedValueRegion *RegM = MRMgr.getRegionForParam(ParamM, SFC);
+  assert(isa(RegN));
+  if (SFC->inTopFrame())
+assert(isa(RegM));
+  else
+assert(isa(RegM));
+}
+
+if (const auto *FD = dyn_cast(D)) {
+  if (FD->getNameAsString() == "foo") {
+const auto *ParamN = findDeclByName(D, "n");
+const TypedValueRegion *RegN = MRMgr.getRegionForParam(ParamN, SFC);
+if (SFC->inTopFrame())
+  assert(isa(RegN));
+else
+  assert(isa(RegN));
+  } else if (FD->getNameAsString() == "bar") {
+const auto *ParamL = findDeclByName(D, "l");
+const TypedValueRegion *RegL = MRMgr.getRegionForParam(ParamL, SFC);
+assert(isa(RegL));
+  }
+}
+  }
+
+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); }"));
+}
+
+} // namespace
+} // namespace ento
+} // namespace clang
Index: clang/unittests/StaticAnalyzer/CMakeLists.txt
===
--- clang/unittests/StaticAnalyzer/CMakeLists.txt
+++ clang/unittests/StaticAnalyzer/CMakeLists.txt
@@ -6,6 +6,7 @@
   AnalyzerOptionsTest.cpp
   CallDescriptionTest.cpp
   StoreTest.cpp
+  ParamRegionTest.cpp
   RegisterCustomCheckersTest.cpp
   SymbolReaperTest.cpp
   )
Index: clang/lib/StaticAnalyzer/Core/SymbolManager.cpp
===
--- clang/lib/StaticAnalyzer/Core/SymbolManager.cpp
+++ 

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

[Balogh , Ádám via Phabricator via cfe-commits]

baloghadamsoftware updated this revision to Diff 263720.
baloghadamsoftware added a comment.

Updated according to the comments from @NoQ.


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

https://reviews.llvm.org/D79704

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/include/clang/StaticAnalyzer/Core/PathSensitive/Store.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h
  clang/lib/StaticAnalyzer/Checkers/NSErrorChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp
  
clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp
  clang/lib/StaticAnalyzer/Checkers/ValistChecker.cpp
  clang/lib/StaticAnalyzer/Core/BugReporterVisitors.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/RegionStore.cpp
  clang/lib/StaticAnalyzer/Core/Store.cpp
  clang/lib/StaticAnalyzer/Core/SymbolManager.cpp

Index: clang/lib/StaticAnalyzer/Core/SymbolManager.cpp
===
--- clang/lib/StaticAnalyzer/Core/SymbolManager.cpp
+++ clang/lib/StaticAnalyzer/Core/SymbolManager.cpp
@@ -440,6 +440,9 @@
   if (const auto *VR = dyn_cast(MR))
 return isLive(VR, true);
 
+  if (const auto *PR = dyn_cast(MR))
+return isLive(PR, true);
+
   // FIXME: This is a gross over-approximation. What we really need is a way to
   // tell if anything still refers to this region. Unlike SymbolicRegions,
   // AllocaRegions don't have associated symbols, though, so we don't actually
@@ -573,3 +576,53 @@
 
   return VarContext->isParentOf(CurrentContext);
 }
+
+bool SymbolReaper::isLive(const ParamRegion *PR,
+  bool includeStoreBindings) const{
+  const StackFrameContext *ParamContext = PR->getStackFrame();
+
+  if (!ParamContext)
+return true;
+
+  if (!LCtx)
+return false;
+  const StackFrameContext *CurrentContext = LCtx->getStackFrame();
+
+  if (ParamContext == CurrentContext) {
+// If no statement is provided, everything is live.
+if (!Loc)
+  return true;
+
+// Anonymous parameters of an inheriting constructor are live for the entire
+// duration of the constructor.
+if (isa(Loc))
+  return true;
+
+if (const ParmVarDecl *PVD = PR->getDecl()) {
+  if (LCtx->getAnalysis()->isLive(Loc, PVD))
+return true;
+}
+
+if (!includeStoreBindings)
+  return false;
+
+unsigned  =
+  const_cast(this)->includedRegionCache[PR];
+
+if (cachedQuery) {
+  return cachedQuery == 1;
+}
+
+// Query the store to see if the region occurs in any live bindings.
+if (Store store = reapedStore.getStore()) {
+  bool hasRegion =
+reapedStore.getStoreManager().includedInBindings(store, PR);
+  cachedQuery = hasRegion ? 1 : 2;
+  return hasRegion;
+}
+
+return false;
+  }
+
+  return ParamContext->isParentOf(CurrentContext);
+}
Index: clang/lib/StaticAnalyzer/Core/Store.cpp
===
--- clang/lib/StaticAnalyzer/Core/Store.cpp
+++ clang/lib/StaticAnalyzer/Core/Store.cpp
@@ -138,6 +138,7 @@
 case MemRegion::CXXTempObjectRegionKind:
 case MemRegion::CXXBaseObjectRegionKind:
 case MemRegion::CXXDerivedObjectRegionKind:
+case MemRegion::ParamRegionKind:
   return MakeElementRegion(cast(R), PointeeTy);
 
 case MemRegion::ElementRegionKind: {
@@ -435,6 +436,13 @@
   return svalBuilder.dispatchCast(V, castTy);
 }
 
+Loc StoreManager::getLValueVar(const VarDecl *VD, const LocationContext *LC) {
+  if (const auto *PVD = dyn_cast(VD))
+return svalBuilder.makeLoc(MRMgr.getRegionForParam(PVD, LC));
+
+  return svalBuilder.makeLoc(MRMgr.getVarRegion(VD, LC));
+}
+
 SVal StoreManager::getLValueFieldOrIvar(const Decl *D, SVal Base) {
   if (Base.isUnknownOrUndef())
 return Base;
Index: clang/lib/StaticAnalyzer/Core/RegionStore.cpp
===
--- clang/lib/StaticAnalyzer/Core/RegionStore.cpp
+++ clang/lib/StaticAnalyzer/Core/RegionStore.cpp
@@ -569,6 +569,8 @@
 
   SVal getBindingForVar(RegionBindingsConstRef B, const VarRegion *R);
 
+  SVal getBindingForParam(RegionBindingsConstRef B, const ParamRegion *R);
+
   SVal getBindingForLazySymbol(const TypedValueRegion *R);
 
   SVal getBindingForFieldOrElementCommon(RegionBindingsConstRef B,
@@ -1510,6 +1512,16 @@
 return CastRetrievedVal(getBindingForVar(B, VR), VR, 

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

[Balogh , Ádám via Phabricator via cfe-commits]

baloghadamsoftware marked an inline comment as done.
baloghadamsoftware added inline comments.



Comment at: clang/lib/StaticAnalyzer/Core/MemRegion.cpp:1735-1736
+  const Stmt *CallSite = SFC->getCallSite();
+  if (!CallSite)
+return std::make_pair(nullptr, UINT_MAX);
+

baloghadamsoftware wrote:
> NoQ wrote:
> > Does this actually ever happen?
> I will check it.
Oh yes. If we have no `CallSite`, then we are in the top frame. Should I check 
`LC->inTopFrame()` instead?


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

https://reviews.llvm.org/D79704



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

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

[Balogh , Ádám via Phabricator via cfe-commits]

baloghadamsoftware marked 9 inline comments as done.
baloghadamsoftware added a comment.

Than you for your reviews, @NoQ and @Szelethus.

In D79704#2032271 , @NoQ wrote:

> Blanket reply! `ParamRegion` is not a `DeclRegion` because it does not 
> necessarily have a corresponding `Decl`. For instance, when calling a 
> function through an unknown function pointer, you don't have the declaration 
> of a function, let alone its parameters. But for parameters of C++ object 
> type you still need your `ParamRegion` because arguments are constructed into 
> it.


Hmm, I removed the part that tries to retrieve the parameter from the arguments 
of the `CallExpr`-like `Expr` because it was never invoked (I used an 
`assert(false)` there and executed all the tests and they passed). This means 
that we always found a `Decl`. However, this `Decl` is //not stored// but 
retrieved always based on the actual stack frame: we get the `Decl` of the 
function/method/block/etc. from the stack frame and find its parameter using 
the `Index`. This is always successful, at least in all the analyzer tests.




Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h:1051
+
+  const Expr *OriginExpr;
+  unsigned Index;

NoQ wrote:
> baloghadamsoftware wrote:
> > We do not use this at all. However, if I remove it then tests begin to fail 
> > with strange reasons, some of them even crashes at strange points. It seems 
> > that we need this for profiling the subclass.
> `Profile` is completely essential. Without it different regions will be 
> treated as if it's the same region.
I know, that was not the question. It is the `OriginExpr` which I tried to 
remove because I do not use it at all, except for profiling. It seems that 
without this field the `Profile` does not work correctly.



Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h:1054-1055
+
+  ParamRegion(const Expr *OE, unsigned Idx, const MemRegion *SReg)
+  : TypedValueRegion(SReg, ParamRegionKind), OriginExpr(OE), Index(Idx) {}
+

NoQ wrote:
> It looks like you decided to keep `VarRegion` for the top frame.
> 
> I want that asserted here, in the constructor, so that never to make a 
> mistake on this front.
Yes, because in the top frame we neither have `CallExpr` nor `Decl`. So we 
should assert here that we are not in the top frame, right? How do we check 
this based on these parameters?



Comment at: clang/lib/StaticAnalyzer/Checkers/NSErrorChecker.cpp:197-198
+if (const ParamRegion *PR = R->getAs())
+  if (const StackArgumentsSpaceRegion *
+  stackReg = dyn_cast(PR->getMemorySpace()))
+if (stackReg->getStackFrame() == SFC)

NoQ wrote:
> Yes it is a `StackArgumentsSpaceRegion`! Because we're a parameter.
So I can remove the inner branch?



Comment at: clang/lib/StaticAnalyzer/Core/MemRegion.cpp:164
+  const auto *SSR = dyn_cast(getMemorySpace());
+  return SSR ? SSR->getStackFrame() : nullptr;
+}

NoQ wrote:
> `SSR` shouldn't ever be null. Any parameter must be on the stack.
OK, I will remove this check.



Comment at: clang/lib/StaticAnalyzer/Core/MemRegion.cpp:1579-1584
+  std::tie(Call, Index) = ParamRegion::getExprAndIndexForParam(PVD, LC);
+
+  if (Call)
+OriginalVR = MemMgr.getParamRegion(Call, Index, LC);
+  else
+OriginalVR = MemMgr.getVarRegion(VD, LC);

NoQ wrote:
> I suggest making a function in `MemRegionManager` that takes a `Decl` and 
> returns a region. There should only be one place in the code that decides 
> when exactly do we produce a `ParamRegion` instead of a `VarRegion`.
Good idea!



Comment at: clang/lib/StaticAnalyzer/Core/MemRegion.cpp:1732
+ const LocationContext *LC) {
+  unsigned Index = PVD->getFunctionScopeIndex();
+  const StackFrameContext *SFC = LC->getStackFrame();

NoQ wrote:
> Does this method also suffer from the parameter vs. argument off-by-one 
> problem with operators?
Actually I removed all usage of the expression thus I got rid of the problem.



Comment at: clang/lib/StaticAnalyzer/Core/MemRegion.cpp:1735-1736
+  const Stmt *CallSite = SFC->getCallSite();
+  if (!CallSite)
+return std::make_pair(nullptr, UINT_MAX);
+

NoQ wrote:
> Does this actually ever happen?
I will check it.



Comment at: clang/lib/StaticAnalyzer/Core/MemRegion.cpp:1740
+  if (const auto *FD = dyn_cast(D)) {
+if (Index >= FD->param_size() || FD->parameters()[Index] != PVD)
+  return std::make_pair(nullptr, UINT_MAX);

NoQ wrote:
> Why would that ever happen? If it's just a sanity check, it should be an 
> assertion.
This happens all the time a lambda or a block captures a parameter of the 
enclosing 

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

[Balogh , Ádám via Phabricator via cfe-commits]

baloghadamsoftware added a comment.

In D79704#2033669 , @Szelethus wrote:

> Yeah, this patch should definitely have unit tests. All similar patches 
> should.


I wonder how I could make unit tests for this. I already looked up the unit 
tests and found no unit test for regions.


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

https://reviews.llvm.org/D79704



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

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

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

Szelethus added inline comments.



Comment at: 
clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp:435-442
+  const VarDecl *VD;
+  if (const auto *VR =
+  dyn_cast(cast(Sym)->getRegion())) {
+VD = cast(VR->getDecl());
+  } else if (const auto *PR =
+ dyn_cast(cast(Sym)->getRegion())) 
{
+VD = cast(PR->getDecl());

baloghadamsoftware wrote:
> Szelethus wrote:
> > Hmm. So, the surrounding code definitely suggests that we're definitely 
> > finishing for a parameter here, but it isn't always a parameter region? I 
> > know you struggled with this, have you gained any insight as to why?
> Not all regions for `ParmVarDecl` are `ParamRegions`. Exceptions are 
> parameters captured by a lambda or a block as well as parameters of functions 
> analyzed top-level.
That would be a lovely unit test case! :)


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

https://reviews.llvm.org/D79704



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

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

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

Szelethus requested changes to this revision.
Szelethus added a comment.
This revision now requires changes to proceed.

Yeah, this patch should definitely have unit tests. All similar patches should.


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

https://reviews.llvm.org/D79704



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

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

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

Szelethus added a comment.

In D79704#2032271 , @NoQ wrote:

> Blanket reply! `ParamRegion` is not a `DeclRegion` because it does not 
> necessarily have a corresponding `Decl`. For instance, when calling a 
> function through an unknown function pointer, you don't have the declaration 
> of a function, let alone its parameters.


Well hold on for a second then -- how is this, if it is, different for member 
pointers? Aren't they represented with a `VarRegion`?

> But for parameters of C++ object type you still need your `ParamRegion` 
> because arguments are constructed into it.

Could you give a specific code example?

Since this patch isn't WIP, lets add unit tests that shows the added interface, 
highlighting pain points such as this.


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

https://reviews.llvm.org/D79704



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

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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

Blanket reply! `ParamRegion` is not a `DeclRegion` because it does not 
necessarily have a corresponding `Decl`. For instance, when calling a function 
through an unknown function pointer, you don't have the declaration of a 
function, let alone its parameters. But for parameters of C++ object type you 
still need your `ParamRegion` because arguments are constructed into it.




Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h:1051
+
+  const Expr *OriginExpr;
+  unsigned Index;

baloghadamsoftware wrote:
> We do not use this at all. However, if I remove it then tests begin to fail 
> with strange reasons, some of them even crashes at strange points. It seems 
> that we need this for profiling the subclass.
`Profile` is completely essential. Without it different regions will be treated 
as if it's the same region.



Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h:1054-1055
+
+  ParamRegion(const Expr *OE, unsigned Idx, const MemRegion *SReg)
+  : TypedValueRegion(SReg, ParamRegionKind), OriginExpr(OE), Index(Idx) {}
+

It looks like you decided to keep `VarRegion` for the top frame.

I want that asserted here, in the constructor, so that never to make a mistake 
on this front.



Comment at: clang/lib/StaticAnalyzer/Checkers/NSErrorChecker.cpp:197-198
+if (const ParamRegion *PR = R->getAs())
+  if (const StackArgumentsSpaceRegion *
+  stackReg = dyn_cast(PR->getMemorySpace()))
+if (stackReg->getStackFrame() == SFC)

Yes it is a `StackArgumentsSpaceRegion`! Because we're a parameter.



Comment at: clang/lib/StaticAnalyzer/Core/MemRegion.cpp:164
+  const auto *SSR = dyn_cast(getMemorySpace());
+  return SSR ? SSR->getStackFrame() : nullptr;
+}

`SSR` shouldn't ever be null. Any parameter must be on the stack.



Comment at: clang/lib/StaticAnalyzer/Core/MemRegion.cpp:1579-1584
+  std::tie(Call, Index) = ParamRegion::getExprAndIndexForParam(PVD, LC);
+
+  if (Call)
+OriginalVR = MemMgr.getParamRegion(Call, Index, LC);
+  else
+OriginalVR = MemMgr.getVarRegion(VD, LC);

I suggest making a function in `MemRegionManager` that takes a `Decl` and 
returns a region. There should only be one place in the code that decides when 
exactly do we produce a `ParamRegion` instead of a `VarRegion`.



Comment at: clang/lib/StaticAnalyzer/Core/MemRegion.cpp:1732
+ const LocationContext *LC) {
+  unsigned Index = PVD->getFunctionScopeIndex();
+  const StackFrameContext *SFC = LC->getStackFrame();

Does this method also suffer from the parameter vs. argument off-by-one problem 
with operators?



Comment at: clang/lib/StaticAnalyzer/Core/MemRegion.cpp:1735-1736
+  const Stmt *CallSite = SFC->getCallSite();
+  if (!CallSite)
+return std::make_pair(nullptr, UINT_MAX);
+

Does this actually ever happen?



Comment at: clang/lib/StaticAnalyzer/Core/MemRegion.cpp:1740
+  if (const auto *FD = dyn_cast(D)) {
+if (Index >= FD->param_size() || FD->parameters()[Index] != PVD)
+  return std::make_pair(nullptr, UINT_MAX);

Why would that ever happen? If it's just a sanity check, it should be an 
assertion.



Comment at: clang/lib/StaticAnalyzer/Core/RegionStore.cpp:2031
+
+  assert (isa(MS));
+

Let's simply make a blanket assertion in `ParamRegion`'s constructor.


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

https://reviews.llvm.org/D79704



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

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

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

Szelethus added a comment.

In D79704#2029305 , 
@baloghadamsoftware wrote:

> Thank you for quickly looking into this.
>
> In D79704#2029230 , @Szelethus wrote:
>
> > - What identifies a `MemRegion`, `TypedValueRegion`s in particular? Why are 
> > parameters so special that they deserve their own region? Do you have a 
> > concrete, problematic example?
>
>
> `ParamRegion` contains different data: mainly the index of the parameter. 
> This makes it independent of the actual `Decl` of the parameter.
>
> > - Why is it an issue that we don't always use the same declaration? Does 
> > this result in a crash, incorrect modeling?
>
> See D49443#1193290 .
>
> > - Why are we making `ParamRegion` **not** a subclass of `VarRegion`?
>
> Because `VarRegion` is a `DeclRegion` which stores the `Decl` of the 
> variable. Here the main purpose is to not to store it.


To me that sounds like a technicality. Sure, inheriting from `VarRegion` would 
make you carry around a field or two you may not need/use, but is that a big 
deal? To me it feels like all the problems you're solving should be an 
implementation detail. The interface of `ParamRegion` looks like a superset of 
`VarRegion`'s. The code changes suggests that every time we're interested in a 
`VarRegion`, we're also interested in parameters.

I may not see the obvious just yet, but I invite you to correct me! :)


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

https://reviews.llvm.org/D79704



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

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

[Balázs Kéri via Phabricator via cfe-commits]

balazske added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/ValistChecker.cpp:176-180
   if (const auto *DeclReg = Reg->getAs()) {
 if (isa(DeclReg->getDecl()))
   Reg = C.getState()->getSVal(SV.castAs()).getAsRegion();
+  } else if (const auto *ParamReg = Reg->getAs()) {
+Reg = C.getState()->getSVal(SV.castAs()).getAsRegion();

baloghadamsoftware wrote:
> balazske wrote:
> > baloghadamsoftware wrote:
> > > Szelethus wrote:
> > > > This is interesting. I looked up `DeclRegion`, and it seems to be the 
> > > > region that is tied to a `ValueDecl`. `VarDecl` is a subtype of 
> > > > `ValueDecl`, and `ParmVarDecl` is a subtype of `VarDecl`, so wouldn't 
> > > > it make sense for `ParamRegion` to be a subtype of `VarRegion`?
> > > `DeclRegion` stores the `Decl`, `ParamRegion` retrieves it based on the 
> > > `Index` it stores. There is no is-a relation between them.
> > During the lifetime of a `ParamRegion` is it possible that it will return 
> > different `Decl` objects?
> @NoQ?
Purpose of the question is that if the answer is "no", the decl could be stored 
at construction of a `ParamRegion` and it could be a subclass of `DeclRegion`. 
From the code I do not see that the belonging `Decl` can change, probably yes 
if the stack frame changes somehow. So the reason to have the `ParamRegion` 
seems to be to have the expression and index available, maybe use these for 
profiling (instead of a `Decl`). Still it could be made a subclass of 
`DeclRegion` that stores additionally the expr and index and profiling works 
with these values.


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

https://reviews.llvm.org/D79704



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

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

[Balogh , Ádám via Phabricator via cfe-commits]

baloghadamsoftware marked 5 inline comments as done.
baloghadamsoftware added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/ValistChecker.cpp:176-180
   if (const auto *DeclReg = Reg->getAs()) {
 if (isa(DeclReg->getDecl()))
   Reg = C.getState()->getSVal(SV.castAs()).getAsRegion();
+  } else if (const auto *ParamReg = Reg->getAs()) {
+Reg = C.getState()->getSVal(SV.castAs()).getAsRegion();

balazske wrote:
> baloghadamsoftware wrote:
> > Szelethus wrote:
> > > This is interesting. I looked up `DeclRegion`, and it seems to be the 
> > > region that is tied to a `ValueDecl`. `VarDecl` is a subtype of 
> > > `ValueDecl`, and `ParmVarDecl` is a subtype of `VarDecl`, so wouldn't it 
> > > make sense for `ParamRegion` to be a subtype of `VarRegion`?
> > `DeclRegion` stores the `Decl`, `ParamRegion` retrieves it based on the 
> > `Index` it stores. There is no is-a relation between them.
> During the lifetime of a `ParamRegion` is it possible that it will return 
> different `Decl` objects?
@NoQ?


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

https://reviews.llvm.org/D79704



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

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

[Balogh , Ádám via Phabricator via cfe-commits]

baloghadamsoftware updated this revision to Diff 263204.
baloghadamsoftware added a comment.

Thank you for your comments, @balazske. You are completely right, I updated the 
patch now.


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

https://reviews.llvm.org/D79704

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/include/clang/StaticAnalyzer/Core/PathSensitive/Store.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h
  clang/lib/StaticAnalyzer/Checkers/NSErrorChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp
  
clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp
  clang/lib/StaticAnalyzer/Checkers/ValistChecker.cpp
  clang/lib/StaticAnalyzer/Core/BugReporterVisitors.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/RegionStore.cpp
  clang/lib/StaticAnalyzer/Core/Store.cpp
  clang/lib/StaticAnalyzer/Core/SymbolManager.cpp

Index: clang/lib/StaticAnalyzer/Core/SymbolManager.cpp
===
--- clang/lib/StaticAnalyzer/Core/SymbolManager.cpp
+++ clang/lib/StaticAnalyzer/Core/SymbolManager.cpp
@@ -440,6 +440,9 @@
   if (const auto *VR = dyn_cast(MR))
 return isLive(VR, true);
 
+  if (const auto *PR = dyn_cast(MR))
+return isLive(PR, true);
+
   // FIXME: This is a gross over-approximation. What we really need is a way to
   // tell if anything still refers to this region. Unlike SymbolicRegions,
   // AllocaRegions don't have associated symbols, though, so we don't actually
@@ -573,3 +576,53 @@
 
   return VarContext->isParentOf(CurrentContext);
 }
+
+bool SymbolReaper::isLive(const ParamRegion *PR,
+  bool includeStoreBindings) const{
+  const StackFrameContext *ParamContext = PR->getStackFrame();
+
+  if (!ParamContext)
+return true;
+
+  if (!LCtx)
+return false;
+  const StackFrameContext *CurrentContext = LCtx->getStackFrame();
+
+  if (ParamContext == CurrentContext) {
+// If no statement is provided, everything is live.
+if (!Loc)
+  return true;
+
+// Anonymous parameters of an inheriting constructor are live for the entire
+// duration of the constructor.
+if (isa(Loc))
+  return true;
+
+if (const ParmVarDecl *PVD = PR->getDecl()) {
+  if (LCtx->getAnalysis()->isLive(Loc, PVD))
+return true;
+}
+
+if (!includeStoreBindings)
+  return false;
+
+unsigned  =
+  const_cast(this)->includedRegionCache[PR];
+
+if (cachedQuery) {
+  return cachedQuery == 1;
+}
+
+// Query the store to see if the region occurs in any live bindings.
+if (Store store = reapedStore.getStore()) {
+  bool hasRegion =
+reapedStore.getStoreManager().includedInBindings(store, PR);
+  cachedQuery = hasRegion ? 1 : 2;
+  return hasRegion;
+}
+
+return false;
+  }
+
+  return ParamContext->isParentOf(CurrentContext);
+}
Index: clang/lib/StaticAnalyzer/Core/Store.cpp
===
--- clang/lib/StaticAnalyzer/Core/Store.cpp
+++ clang/lib/StaticAnalyzer/Core/Store.cpp
@@ -138,6 +138,7 @@
 case MemRegion::CXXTempObjectRegionKind:
 case MemRegion::CXXBaseObjectRegionKind:
 case MemRegion::CXXDerivedObjectRegionKind:
+case MemRegion::ParamRegionKind:
   return MakeElementRegion(cast(R), PointeeTy);
 
 case MemRegion::ElementRegionKind: {
@@ -435,6 +436,19 @@
   return svalBuilder.dispatchCast(V, castTy);
 }
 
+Loc StoreManager::getLValueVar(const VarDecl *VD, const LocationContext *LC) {
+  if (const auto *PVD = dyn_cast(VD)) {
+const Expr* Call;
+unsigned Index;
+std::tie(Call, Index) = ParamRegion::getExprAndIndexForParam(PVD, LC);
+
+if (Call)
+  return svalBuilder.makeLoc(MRMgr.getParamRegion(Call, Index, LC));
+  }
+
+  return svalBuilder.makeLoc(MRMgr.getVarRegion(VD, LC));
+}
+
 SVal StoreManager::getLValueFieldOrIvar(const Decl *D, SVal Base) {
   if (Base.isUnknownOrUndef())
 return Base;
Index: clang/lib/StaticAnalyzer/Core/RegionStore.cpp
===
--- clang/lib/StaticAnalyzer/Core/RegionStore.cpp
+++ clang/lib/StaticAnalyzer/Core/RegionStore.cpp
@@ -569,6 +569,8 @@
 
   SVal getBindingForVar(RegionBindingsConstRef B, const VarRegion *R);
 
+  SVal getBindingForParam(RegionBindingsConstRef B, const ParamRegion *R);
+
   SVal 

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

[Balázs Kéri via Phabricator via cfe-commits]

balazske added inline comments.



Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h:403
   /// frame. May fail; returns null on failure.
-  const VarRegion *getParameterLocation(unsigned Index,
-unsigned BlockCount) const;
+  const TypedValueRegion *getParameterLocation(unsigned Index,
+   unsigned BlockCount) const;

I would expect that this returns a `ParamRegion` (from the name of the 
function). (The implementation shows that the type can just be changed.)



Comment at: clang/lib/StaticAnalyzer/Checkers/ValistChecker.cpp:176-180
   if (const auto *DeclReg = Reg->getAs()) {
 if (isa(DeclReg->getDecl()))
   Reg = C.getState()->getSVal(SV.castAs()).getAsRegion();
+  } else if (const auto *ParamReg = Reg->getAs()) {
+Reg = C.getState()->getSVal(SV.castAs()).getAsRegion();

baloghadamsoftware wrote:
> Szelethus wrote:
> > This is interesting. I looked up `DeclRegion`, and it seems to be the 
> > region that is tied to a `ValueDecl`. `VarDecl` is a subtype of 
> > `ValueDecl`, and `ParmVarDecl` is a subtype of `VarDecl`, so wouldn't it 
> > make sense for `ParamRegion` to be a subtype of `VarRegion`?
> `DeclRegion` stores the `Decl`, `ParamRegion` retrieves it based on the 
> `Index` it stores. There is no is-a relation between them.
During the lifetime of a `ParamRegion` is it possible that it will return 
different `Decl` objects?



Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:745
 
 // Can only reasonably pretty-print DeclRegions.
+if (const auto *DR = dyn_cast(R)) {

This comment is not fully true any more.



Comment at: clang/lib/StaticAnalyzer/Core/CallEvent.cpp:255
   // correspond to the stack frame's function declaration.
   assert(VR->getStackFrame() == SFC);
 

Is it correct to create `VR` only to have this assert?



Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:357
   V = *OptV;
-else
-  break;
+else break;
 State = addObjectUnderConstruction(State, {CE, Idx}, LCtx, V);

This `break` is at wrong place.


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

https://reviews.llvm.org/D79704



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

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

[Balogh , Ádám via Phabricator via cfe-commits]

baloghadamsoftware updated this revision to Diff 263154.
baloghadamsoftware added a comment.

Minor fix to get rid of a warning, some comments added.


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

https://reviews.llvm.org/D79704

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/include/clang/StaticAnalyzer/Core/PathSensitive/Store.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h
  clang/lib/StaticAnalyzer/Checkers/NSErrorChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp
  
clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp
  clang/lib/StaticAnalyzer/Checkers/ValistChecker.cpp
  clang/lib/StaticAnalyzer/Core/BugReporterVisitors.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/RegionStore.cpp
  clang/lib/StaticAnalyzer/Core/Store.cpp
  clang/lib/StaticAnalyzer/Core/SymbolManager.cpp

Index: clang/lib/StaticAnalyzer/Core/SymbolManager.cpp
===
--- clang/lib/StaticAnalyzer/Core/SymbolManager.cpp
+++ clang/lib/StaticAnalyzer/Core/SymbolManager.cpp
@@ -440,6 +440,9 @@
   if (const auto *VR = dyn_cast(MR))
 return isLive(VR, true);
 
+  if (const auto *PR = dyn_cast(MR))
+return isLive(PR, true);
+
   // FIXME: This is a gross over-approximation. What we really need is a way to
   // tell if anything still refers to this region. Unlike SymbolicRegions,
   // AllocaRegions don't have associated symbols, though, so we don't actually
@@ -573,3 +576,53 @@
 
   return VarContext->isParentOf(CurrentContext);
 }
+
+bool SymbolReaper::isLive(const ParamRegion *PR,
+  bool includeStoreBindings) const{
+  const StackFrameContext *ParamContext = PR->getStackFrame();
+
+  if (!ParamContext)
+return true;
+
+  if (!LCtx)
+return false;
+  const StackFrameContext *CurrentContext = LCtx->getStackFrame();
+
+  if (ParamContext == CurrentContext) {
+// If no statement is provided, everything is live.
+if (!Loc)
+  return true;
+
+// Anonymous parameters of an inheriting constructor are live for the entire
+// duration of the constructor.
+if (isa(Loc))
+  return true;
+
+if (const ParmVarDecl *PVD = PR->getDecl()) {
+  if (LCtx->getAnalysis()->isLive(Loc, PVD))
+return true;
+}
+
+if (!includeStoreBindings)
+  return false;
+
+unsigned  =
+  const_cast(this)->includedRegionCache[PR];
+
+if (cachedQuery) {
+  return cachedQuery == 1;
+}
+
+// Query the store to see if the region occurs in any live bindings.
+if (Store store = reapedStore.getStore()) {
+  bool hasRegion =
+reapedStore.getStoreManager().includedInBindings(store, PR);
+  cachedQuery = hasRegion ? 1 : 2;
+  return hasRegion;
+}
+
+return false;
+  }
+
+  return ParamContext->isParentOf(CurrentContext);
+}
Index: clang/lib/StaticAnalyzer/Core/Store.cpp
===
--- clang/lib/StaticAnalyzer/Core/Store.cpp
+++ clang/lib/StaticAnalyzer/Core/Store.cpp
@@ -138,6 +138,7 @@
 case MemRegion::CXXTempObjectRegionKind:
 case MemRegion::CXXBaseObjectRegionKind:
 case MemRegion::CXXDerivedObjectRegionKind:
+case MemRegion::ParamRegionKind:
   return MakeElementRegion(cast(R), PointeeTy);
 
 case MemRegion::ElementRegionKind: {
@@ -435,6 +436,19 @@
   return svalBuilder.dispatchCast(V, castTy);
 }
 
+Loc StoreManager::getLValueVar(const VarDecl *VD, const LocationContext *LC) {
+  if (const auto *PVD = dyn_cast(VD)) {
+const Expr* Call;
+unsigned Index;
+std::tie(Call, Index) = ParamRegion::getExprAndIndexForParam(PVD, LC);
+
+if (Call)
+  return svalBuilder.makeLoc(MRMgr.getParamRegion(Call, Index, LC));
+  }
+
+  return svalBuilder.makeLoc(MRMgr.getVarRegion(VD, LC));
+}
+
 SVal StoreManager::getLValueFieldOrIvar(const Decl *D, SVal Base) {
   if (Base.isUnknownOrUndef())
 return Base;
Index: clang/lib/StaticAnalyzer/Core/RegionStore.cpp
===
--- clang/lib/StaticAnalyzer/Core/RegionStore.cpp
+++ clang/lib/StaticAnalyzer/Core/RegionStore.cpp
@@ -569,6 +569,8 @@
 
   SVal getBindingForVar(RegionBindingsConstRef B, const VarRegion *R);
 
+  SVal getBindingForParam(RegionBindingsConstRef B, const ParamRegion *R);
+
   SVal getBindingForLazySymbol(const 

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

[Balogh , Ádám via Phabricator via cfe-commits]

baloghadamsoftware marked an inline comment as done.
baloghadamsoftware added inline comments.



Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h:1051
+
+  const Expr *OriginExpr;
+  unsigned Index;

We do not use this at all. However, if I remove it then tests begin to fail 
with strange reasons, some of them even crashes at strange points. It seems 
that we need this for profiling the subclass.


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

https://reviews.llvm.org/D79704



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

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

[Balogh , Ádám via Phabricator via cfe-commits]

baloghadamsoftware marked 2 inline comments as done.
baloghadamsoftware added a comment.

Thank you for quickly looking into this.

In D79704#2029230 , @Szelethus wrote:

> - What identifies a `MemRegion`, `TypedValueRegion`s in particular? Why are 
> parameters so special that they deserve their own region? Do you have a 
> concrete, problematic example?


`ParamRegion` contains different data: mainly the index of the parameter. This 
makes it independent of the actual `Decl` of the parameter.

> - Why is it an issue that we don't always use the same declaration? Does this 
> result in a crash, incorrect modeling?

See D49443#1193290 .

> - Why are we making `ParamRegion` **not** a subclass of `VarRegion`?

Because `VarRegion` is a `DeclRegion` which stores the `Decl` of the variable. 
Here the main purpose is to not to store it.

> - The patch includes some code duplication, which may be okay, but do we need 
> it?

Yes, and this comes from my previous answers.




Comment at: 
clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp:435-442
+  const VarDecl *VD;
+  if (const auto *VR =
+  dyn_cast(cast(Sym)->getRegion())) {
+VD = cast(VR->getDecl());
+  } else if (const auto *PR =
+ dyn_cast(cast(Sym)->getRegion())) 
{
+VD = cast(PR->getDecl());

Szelethus wrote:
> Hmm. So, the surrounding code definitely suggests that we're definitely 
> finishing for a parameter here, but it isn't always a parameter region? I 
> know you struggled with this, have you gained any insight as to why?
Not all regions for `ParmVarDecl` are `ParamRegions`. Exceptions are parameters 
captured by a lambda or a block as well as parameters of functions analyzed 
top-level.



Comment at: clang/lib/StaticAnalyzer/Checkers/ValistChecker.cpp:176-180
   if (const auto *DeclReg = Reg->getAs()) {
 if (isa(DeclReg->getDecl()))
   Reg = C.getState()->getSVal(SV.castAs()).getAsRegion();
+  } else if (const auto *ParamReg = Reg->getAs()) {
+Reg = C.getState()->getSVal(SV.castAs()).getAsRegion();

Szelethus wrote:
> This is interesting. I looked up `DeclRegion`, and it seems to be the region 
> that is tied to a `ValueDecl`. `VarDecl` is a subtype of `ValueDecl`, and 
> `ParmVarDecl` is a subtype of `VarDecl`, so wouldn't it make sense for 
> `ParamRegion` to be a subtype of `VarRegion`?
`DeclRegion` stores the `Decl`, `ParamRegion` retrieves it based on the `Index` 
it stores. There is no is-a relation between them.


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

https://reviews.llvm.org/D79704



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

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

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

Szelethus added a comment.

> 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 introduce a new kind 
> of region called ParamRegion which 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.

I know vaguely of what you're talking about because I've been trying to follow 
your works so far, but I'd prefer to include a bit more context for those not 
so familiar with MemRegions. Such as, "Usually, we identify a MemRegion with 
..., but for parameters, this is difficult because ..., as seen on this example 
... . So, this patch introduces a new region, etcetc."

Some immediate questions:

- What identifies a `MemRegion`, `TypedValueRegion`s in particular? Why are 
parameters so special that they deserve their own region? Do you have a 
concrete, problematic example?
- Why is it an issue that we don't always use the same declaration? Does this 
result in a crash, incorrect modeling?
- Why are we making `ParamRegion` **not** a subclass of `VarRegion`?
- The patch includes some code duplication, which may be okay, but do we need 
it?

I haven't read every line thoroughly just yet, so I'll catch up with this patch 
later! In any case, thank you so much for your investment in the health of the 
codebase!




Comment at: 
clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp:435-442
+  const VarDecl *VD;
+  if (const auto *VR =
+  dyn_cast(cast(Sym)->getRegion())) {
+VD = cast(VR->getDecl());
+  } else if (const auto *PR =
+ dyn_cast(cast(Sym)->getRegion())) 
{
+VD = cast(PR->getDecl());

Hmm. So, the surrounding code definitely suggests that we're definitely 
finishing for a parameter here, but it isn't always a parameter region? I know 
you struggled with this, have you gained any insight as to why?



Comment at: clang/lib/StaticAnalyzer/Checkers/ValistChecker.cpp:176-180
   if (const auto *DeclReg = Reg->getAs()) {
 if (isa(DeclReg->getDecl()))
   Reg = C.getState()->getSVal(SV.castAs()).getAsRegion();
+  } else if (const auto *ParamReg = Reg->getAs()) {
+Reg = C.getState()->getSVal(SV.castAs()).getAsRegion();

This is interesting. I looked up `DeclRegion`, and it seems to be the region 
that is tied to a `ValueDecl`. `VarDecl` is a subtype of `ValueDecl`, and 
`ParmVarDecl` is a subtype of `VarDecl`, so wouldn't it make sense for 
`ParamRegion` to be a subtype of `VarRegion`?


Repository:
  rC Clang

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

https://reviews.llvm.org/D79704



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

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

[Balogh , Ádám via Phabricator via cfe-commits]

baloghadamsoftware created this revision.
baloghadamsoftware added reviewers: NoQ, Szelethus.
baloghadamsoftware added a project: clang.
Herald added subscribers: ASDenysPetrov, martong, steakhal, Charusso, 
gamesh411, dkrupp, donat.nagy, mikhail.ramalho, a.sidorin, rnkovacs, szepet, 
xazax.hun, whisperity.

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 introduce a new kind of 
region called `ParamRegion` which 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.


Repository:
  rC Clang

https://reviews.llvm.org/D79704

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/include/clang/StaticAnalyzer/Core/PathSensitive/Store.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h
  clang/lib/StaticAnalyzer/Checkers/NSErrorChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp
  
clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp
  clang/lib/StaticAnalyzer/Checkers/ValistChecker.cpp
  clang/lib/StaticAnalyzer/Core/BugReporterVisitors.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/RegionStore.cpp
  clang/lib/StaticAnalyzer/Core/Store.cpp
  clang/lib/StaticAnalyzer/Core/SymbolManager.cpp

Index: clang/lib/StaticAnalyzer/Core/SymbolManager.cpp
===
--- clang/lib/StaticAnalyzer/Core/SymbolManager.cpp
+++ clang/lib/StaticAnalyzer/Core/SymbolManager.cpp
@@ -440,6 +440,9 @@
   if (const auto *VR = dyn_cast(MR))
 return isLive(VR, true);
 
+  if (const auto *PR = dyn_cast(MR))
+return isLive(PR, true);
+
   // FIXME: This is a gross over-approximation. What we really need is a way to
   // tell if anything still refers to this region. Unlike SymbolicRegions,
   // AllocaRegions don't have associated symbols, though, so we don't actually
@@ -573,3 +576,53 @@
 
   return VarContext->isParentOf(CurrentContext);
 }
+
+bool SymbolReaper::isLive(const ParamRegion *PR,
+  bool includeStoreBindings) const{
+  const StackFrameContext *ParamContext = PR->getStackFrame();
+
+  if (!ParamContext)
+return true;
+
+  if (!LCtx)
+return false;
+  const StackFrameContext *CurrentContext = LCtx->getStackFrame();
+
+  if (ParamContext == CurrentContext) {
+// If no statement is provided, everything is live.
+if (!Loc)
+  return true;
+
+// Anonymous parameters of an inheriting constructor are live for the entire
+// duration of the constructor.
+if (isa(Loc))
+  return true;
+
+if (const ParmVarDecl *PVD = PR->getDecl()) {
+  if (LCtx->getAnalysis()->isLive(Loc, PVD))
+return true;
+}
+
+if (!includeStoreBindings)
+  return false;
+
+unsigned  =
+  const_cast(this)->includedRegionCache[PR];
+
+if (cachedQuery) {
+  return cachedQuery == 1;
+}
+
+// Query the store to see if the region occurs in any live bindings.
+if (Store store = reapedStore.getStore()) {
+  bool hasRegion =
+reapedStore.getStoreManager().includedInBindings(store, PR);
+  cachedQuery = hasRegion ? 1 : 2;
+  return hasRegion;
+}
+
+return false;
+  }
+
+  return ParamContext->isParentOf(CurrentContext);
+}
Index: clang/lib/StaticAnalyzer/Core/Store.cpp
===
--- clang/lib/StaticAnalyzer/Core/Store.cpp
+++ clang/lib/StaticAnalyzer/Core/Store.cpp
@@ -138,6 +138,7 @@
 case MemRegion::CXXTempObjectRegionKind:
 case MemRegion::CXXBaseObjectRegionKind:
 case MemRegion::CXXDerivedObjectRegionKind:
+case MemRegion::ParamRegionKind:
   return MakeElementRegion(cast(R), PointeeTy);
 
 case MemRegion::ElementRegionKind: {
@@ -435,6 +436,19 @@
   return svalBuilder.dispatchCast(V, castTy);
 }
 
+Loc StoreManager::getLValueVar(const VarDecl *VD, const LocationContext *LC) {
+  if (const auto *PVD = dyn_cast(VD)) {
+const Expr* Call;
+unsigned Index;
+std::tie(Call, Index) = ParamRegion::getExprAndIndexForParam(PVD, LC);
+
+if (Call)
+  return svalBuilder.makeLoc(MRMgr.getParamRegion(Call, Index, LC));
+  }
+
+  return