[PATCH] D43497: [analyzer] Introduce correct lifetime extension behavior in simple cases.

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC326236: [analyzer] Introduce correct lifetime extension 
behavior in simple cases. (authored by dergachev, committed by ).

Repository:
  rC Clang

https://reviews.llvm.org/D43497

Files:
  include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
  lib/StaticAnalyzer/Core/ExprEngine.cpp
  lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
  test/Analysis/lifetime-extension.cpp
  test/Analysis/temporaries.cpp

Index: lib/StaticAnalyzer/Core/ExprEngine.cpp
===
--- lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ lib/StaticAnalyzer/Core/ExprEngine.cpp
@@ -65,6 +65,17 @@
 REGISTER_TRAIT_WITH_PROGRAMSTATE(InitializedTemporaries,
  InitializedTemporariesMap)
 
+typedef llvm::ImmutableMap
+TemporaryMaterializationMap;
+
+// Keeps track of temporaries that will need to be materialized later.
+// The StackFrameContext assures that nested calls due to inlined recursive
+// functions do not interfere.
+REGISTER_TRAIT_WITH_PROGRAMSTATE(TemporaryMaterializations,
+ TemporaryMaterializationMap)
+
 typedef llvm::ImmutableMap
@@ -255,17 +266,35 @@
   const Expr *Init = InitWithAdjustments->skipRValueSubobjectAdjustments(
   CommaLHSs, Adjustments);
 
+  // Take the region for Init, i.e. for the whole object. If we do not remember
+  // the region in which the object originally was constructed, come up with
+  // a new temporary region out of thin air and copy the contents of the object
+  // (which are currently present in the Environment, because Init is an rvalue)
+  // into that region. This is not correct, but it is better than nothing.
+  bool FoundOriginalMaterializationRegion = false;
   const TypedValueRegion *TR = nullptr;
   if (const MaterializeTemporaryExpr *MT =
   dyn_cast(Result)) {
-StorageDuration SD = MT->getStorageDuration();
-// If this object is bound to a reference with static storage duration, we
-// put it in a different region to prevent "address leakage" warnings.
-if (SD == SD_Static || SD == SD_Thread)
-  TR = MRMgr.getCXXStaticTempObjectRegion(Init);
-  }
-  if (!TR)
+auto Key = std::make_pair(MT, LC->getCurrentStackFrame());
+if (const CXXTempObjectRegion *const *TRPtr =
+State->get(Key)) {
+  FoundOriginalMaterializationRegion = true;
+  TR = *TRPtr;
+  assert(TR);
+  State = State->remove(Key);
+} else {
+  StorageDuration SD = MT->getStorageDuration();
+  // If this object is bound to a reference with static storage duration, we
+  // put it in a different region to prevent "address leakage" warnings.
+  if (SD == SD_Static || SD == SD_Thread) {
+TR = MRMgr.getCXXStaticTempObjectRegion(Init);
+  } else {
+TR = MRMgr.getCXXTempObjectRegion(Init, LC);
+  }
+}
+  } else {
 TR = MRMgr.getCXXTempObjectRegion(Init, LC);
+  }
 
   SVal Reg = loc::MemRegionVal(TR);
   SVal BaseReg = Reg;
@@ -287,41 +316,46 @@
 }
   }
 
-  // What remains is to copy the value of the object to the new region.
-  // FIXME: In other words, what we should always do is copy value of the
-  // Init expression (which corresponds to the bigger object) to the whole
-  // temporary region TR. However, this value is often no longer present
-  // in the Environment. If it has disappeared, we instead invalidate TR.
-  // Still, what we can do is assign the value of expression Ex (which
-  // corresponds to the sub-object) to the TR's sub-region Reg. At least,
-  // values inside Reg would be correct.
-  SVal InitVal = State->getSVal(Init, LC);
-  if (InitVal.isUnknown()) {
-InitVal = getSValBuilder().conjureSymbolVal(Result, LC, Init->getType(),
-currBldrCtx->blockCount());
-State = State->bindLoc(BaseReg.castAs(), InitVal, LC, false);
-
-// Then we'd need to take the value that certainly exists and bind it over.
-if (InitValWithAdjustments.isUnknown()) {
-  // Try to recover some path sensitivity in case we couldn't
-  // compute the value.
-  InitValWithAdjustments = getSValBuilder().conjureSymbolVal(
-  Result, LC, InitWithAdjustments->getType(),
-  currBldrCtx->blockCount());
+  if (!FoundOriginalMaterializationRegion) {
+// What remains is to copy the value of the object to the new region.
+// FIXME: In other words, what we should always do is copy value of the
+// Init expression (which corresponds to the bigger object) to the whole
+// temporary region TR. However, this value is often no longer present
+// in the Environment. If it has disappeared, we instead invalidate TR.
+// Still, what we can do is assign the value of expression Ex (which
+// 

[PATCH] D43497: [analyzer] Introduce correct lifetime extension behavior in simple cases.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

> Add a FIXME test case that demonstrates that automatic destructors don't fire 
> after lifetime extension through a POD field, even though lifetime extension 
> itself seems to work correctly.

This has always been broken - the CFG element for the automatic destructor is 
simply missing in this case. But previously we didn't inline the constructor, 
so the whole thing was invalidated anyway. I wonder how Sema lives without it.

Now the interesting part here is https://reviews.llvm.org/rC288563 which seems 
to have removed the need to `skipRValueSubobjectAdjustments()` //three days 
after// i added that code in https://reviews.llvm.org/rC288263 ¯\_(ツ)_/¯


https://reviews.llvm.org/D43497



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

[PATCH] D43497: [analyzer] Introduce correct lifetime extension behavior in simple cases.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:396
+ProgramStateRef State, const LocationContext *FromLC,
+const LocationContext *ToLC) {
+  const LocationContext *LC = FromLC;

a.sidorin wrote:
> EndLC? (similar to iterators)
Sounds neat, i'd make another quick patch on all things together.



Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:281
+
+if (!TR) {
+  StorageDuration SD = MT->getStorageDuration();

dcoughlin wrote:
> Would it be safe for the body of the `if (!TR)` to be the else branch of `if 
> constCXXTempObjectionRegion *const *TRPtr = ...` rather then its own if 
> statement?
Yep. Fxd.



Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:289
   }
   if (!TR)
 TR = MRMgr.getCXXTempObjectRegion(Init, LC);

dcoughlin wrote:
> Would it be safe for `TR = MRMgr.getCXXTempObjectRegion(Init, LC);` to be the 
> else branch of `if (const MaterializeTemporaryExpr *MT = 
> dyn_cast(Result))` rather than its own `if` 
> statement?
> 
> Ideally the number paths through this function on which we call 
> `MRMgr.getCXXTempObjectRegion()` would be small and very clear.
> 
Nope, it also covers the case when we have an MTE but it's not static. I guess 
it's still more clear to add it twice than to have it as a fallback for an 
indefinite amount of cases.



Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:490
 
+static void printTemporaryMaterializatinosForContext(
+raw_ostream , ProgramStateRef State, const char *NL, const char *Sep,

dcoughlin wrote:
> Nit: There is a typo in the name here ("Materializatinos"). I guess this is 
> the sub-atomic particle created as a byproduct of materializing a temporary! 
> We have a lot of materializatinos in Cupertino.
Fixed to prevent further lifetime extensino of typos through autocompletinos.



Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:503
+Key.first->printPretty(Out, nullptr, PP);
+if (Value)
+  Out << " : " << Value;

a.sidorin wrote:
> As I see from line 350, `Value` is always non-null. And there is same check 
> on line 659. Am I missing something?
Yep, all of these are non-null by construction. This actually makes me wonder 
if it was correct to remove `VisitCXXBindTemporaryExpr` from `ExprEngine` - 
might have done it too early (this is what makes the similar if in 
`printInitializedTemporariesForContext` redundant). The 
`dont_forget_destructor_around_logical_op` test also suggests that - seems to 
actually be a regression. I guess i'm going to revert the 
`VisitCXXBindTemporaryExpr` change in a follow-up commit.

Also we should allow `ostream << MR` to accept null pointers. It's a global 
operator overload anyway.



Comment at: test/Analysis/lifetime-extension.cpp:45
+  clang_analyzer_eval(z == 2);
+#ifdef TEMPORARIES
+ // expected-warning@-4{{TRUE}}

dcoughlin wrote:
> Yay!
These worked before (with old lifetime extension, since temporary constructors 
were inlined), i only added the run-line to see that.


https://reviews.llvm.org/D43497



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

[PATCH] D43497: [analyzer] Introduce correct lifetime extension behavior in simple cases.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ updated this revision to Diff 135312.
NoQ marked 4 inline comments as done.
NoQ added a comment.

- Address comments.
- Add a FIXME test case that demonstrates that automatic destructors don't fire 
after lifetime extension through a POD field, even though lifetime extension 
itself seems to work correctly. I should probably also add a test case for the 
situation where sub-object adjustments actually kick in (here they suddenly 
don't), because even though they are correctly handled in 
`createTemporaryRegionIfNeeded`, they aren't implemented in 
`findConstructionContexts`.


https://reviews.llvm.org/D43497

Files:
  include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
  lib/StaticAnalyzer/Core/ExprEngine.cpp
  lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
  test/Analysis/lifetime-extension.cpp
  test/Analysis/temporaries.cpp

Index: test/Analysis/temporaries.cpp
===
--- test/Analysis/temporaries.cpp
+++ test/Analysis/temporaries.cpp
@@ -895,6 +895,33 @@
 }
 } // namespace test_match_constructors_and_destructors
 
+namespace dont_forget_destructor_around_logical_op {
+int glob;
+
+class C {
+public:
+  ~C() { glob = 1; }
+};
+
+C get();
+
+bool is(C);
+
+
+void test(int coin) {
+  // Here temporaries are being cleaned up after && is evaluated. There are two
+  // temporaries: the return value of get() and the elidable copy constructor
+  // of that return value into is(). According to the CFG, we need to cleanup
+  // both of them depending on whether the temporary corresponding to the
+  // return value of get() was initialized. However, for now we don't track
+  // temporaries returned from functions, so we take the wrong branch.
+  coin && is(get()); // no-crash
+  // FIXME: We should have called the destructor, i.e. should be TRUE,
+  // at least when we inline temporary destructors.
+  clang_analyzer_eval(glob == 1); // expected-warning{{UNKNOWN}}
+}
+} // namespace dont_forget_destructor_around_logical_op
+
 #if __cplusplus >= 201103L
 namespace temporary_list_crash {
 class C {
Index: test/Analysis/lifetime-extension.cpp
===
--- test/Analysis/lifetime-extension.cpp
+++ test/Analysis/lifetime-extension.cpp
@@ -1,4 +1,5 @@
-// RUN: %clang_analyze_cc1 -Wno-unused -std=c++11 -analyzer-checker=debug.ExprInspection -verify %s
+// RUN: %clang_analyze_cc1 -Wno-unused -std=c++11 -analyzer-checker=core,debug.ExprInspection -verify %s
+// RUN: %clang_analyze_cc1 -Wno-unused -std=c++11 -analyzer-checker=core,debug.ExprInspection -analyzer-config cfg-temporary-dtors=true -DTEMPORARIES -verify %s
 
 void clang_analyzer_eval(bool);
 
@@ -38,9 +39,157 @@
   const int  = A().j[1]; // no-crash
   const int  = (A().j[1], A().j[0]); // no-crash
 
-  // FIXME: All of these should be TRUE, but constructors aren't inlined.
-  clang_analyzer_eval(x == 1); // expected-warning{{UNKNOWN}}
-  clang_analyzer_eval(y == 3); // expected-warning{{UNKNOWN}}
-  clang_analyzer_eval(z == 2); // expected-warning{{UNKNOWN}}
+  clang_analyzer_eval(x == 1);
+  clang_analyzer_eval(y == 3);
+  clang_analyzer_eval(z == 2);
+#ifdef TEMPORARIES
+ // expected-warning@-4{{TRUE}}
+ // expected-warning@-4{{TRUE}}
+ // expected-warning@-4{{TRUE}}
+#else
+ // expected-warning@-8{{UNKNOWN}}
+ // expected-warning@-8{{UNKNOWN}}
+ // expected-warning@-8{{UNKNOWN}}
+#endif
 }
 } // end namespace pr19539_crash_on_destroying_an_integer
+
+namespace maintain_original_object_address_on_lifetime_extension {
+class C {
+  C **after, **before;
+
+public:
+  bool x;
+
+  C(bool x, C **after, C **before) : x(x), after(after), before(before) {
+*before = this;
+  }
+
+  // Don't track copies in our tests.
+  C(const C ) : x(c.x), after(nullptr), before(nullptr) {}
+
+  ~C() { if (after) *after = this; }
+
+  operator bool() const { return x; }
+};
+
+void f1() {
+  C *after, *before;
+  {
+const C  = C(true, , );
+  }
+  clang_analyzer_eval(after == before);
+#ifdef TEMPORARIES
+  // expected-warning@-2{{TRUE}}
+#else
+  // expected-warning@-4{{UNKNOWN}}
+#endif
+}
+
+void f2() {
+  C *after, *before;
+  C c = C(1, , );
+  clang_analyzer_eval(after == before);
+#ifdef TEMPORARIES
+  // expected-warning@-2{{TRUE}}
+#else
+  // expected-warning@-4{{UNKNOWN}}
+#endif
+}
+
+void f3(bool coin) {
+  C *after, *before;
+  {
+const C  = coin ? C(true, , ) : C(false, , );
+  }
+  clang_analyzer_eval(after == before);
+#ifdef TEMPORARIES
+  // expected-warning@-2{{TRUE}}
+#else
+  // expected-warning@-4{{UNKNOWN}}
+#endif
+}
+
+void f4(bool coin) {
+  C *after, *before;
+  {
+// no-crash
+const C  = C(coin, , ) ?: C(false, , );
+  }
+  // FIXME: Add support for lifetime extension through binary conditional
+  // operator. Ideally also add support for the binary conditional operator in
+  // C++. Because for now it calls the constructor for the condition twice.
+  if (coin) {
+clang_analyzer_eval(after == before);

[PATCH] D43497: [analyzer] Introduce correct lifetime extension behavior in simple cases.

[Aleksei Sidorin via Phabricator via cfe-commits]

a.sidorin accepted this revision.
a.sidorin added a comment.

Thank you! Just some of nits inline.




Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:394
+
+bool ExprEngine::areTemporaryMaterializationsClear(
+ProgramStateRef State, const LocationContext *FromLC,

```
for (const auto  : State->get()) {
  auto *LCtx = I.first.second;
  if (LCtx == FromLC || (LCtx->isParentOf(From) && (!To || 
To->isParentOf(LCtx)))
 return false;
}
return true;
```
is a bit shorter but less evident so I won't insist.



Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:396
+ProgramStateRef State, const LocationContext *FromLC,
+const LocationContext *ToLC) {
+  const LocationContext *LC = FromLC;

EndLC? (similar to iterators)



Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:400
+assert(LC && "ToLC must be a parent of FromLC!");
+for (auto I : State->get())
+  if (I.first.second == LC)

const auto &?



Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:503
+Key.first->printPretty(Out, nullptr, PP);
+if (Value)
+  Out << " : " << Value;

As I see from line 350, `Value` is always non-null. And there is same check on 
line 659. Am I missing something?


https://reviews.llvm.org/D43497



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

[PATCH] D43497: [analyzer] Introduce correct lifetime extension behavior in simple cases.

[Devin Coughlin via Phabricator via cfe-commits]

dcoughlin accepted this revision.
dcoughlin added a comment.
This revision is now accepted and ready to land.

Looks good to me. I have a comment about simplifying 
createTemporaryRegionIfNeeded() (if possible) inline.




Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:281
+
+if (!TR) {
+  StorageDuration SD = MT->getStorageDuration();

Would it be safe for the body of the `if (!TR)` to be the else branch of `if 
constCXXTempObjectionRegion *const *TRPtr = ...` rather then its own if 
statement?



Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:289
   }
   if (!TR)
 TR = MRMgr.getCXXTempObjectRegion(Init, LC);

Would it be safe for `TR = MRMgr.getCXXTempObjectRegion(Init, LC);` to be the 
else branch of `if (const MaterializeTemporaryExpr *MT = 
dyn_cast(Result))` rather than its own `if` statement?

Ideally the number paths through this function on which we call 
`MRMgr.getCXXTempObjectRegion()` would be small and very clear.




Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:490
 
+static void printTemporaryMaterializatinosForContext(
+raw_ostream , ProgramStateRef State, const char *NL, const char *Sep,

Nit: There is a typo in the name here ("Materializatinos"). I guess this is the 
sub-atomic particle created as a byproduct of materializing a temporary! We 
have a lot of materializatinos in Cupertino.



Comment at: test/Analysis/lifetime-extension.cpp:45
+  clang_analyzer_eval(z == 2);
+#ifdef TEMPORARIES
+ // expected-warning@-4{{TRUE}}

Yay!



Comment at: test/Analysis/lifetime-extension.cpp:78
+  {
+const C  = C(1, , );
+  }

Nice.


https://reviews.llvm.org/D43497



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

[PATCH] D43497: [analyzer] Introduce correct lifetime extension behavior in simple cases.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ updated this revision to Diff 135186.
NoQ added a comment.

Add one more FIXME test (`dont_forget_destructor_around_logical_op` in 
`temporaries.cpp`) which demonstrates a situation where we fail to call the 
temporary destructor after calling the temporary constructor. It should be 
fixed once we implement return value construction properly, as described at the 
"Return by value" part of 
http://lists.llvm.org/pipermail/cfe-dev/2018-February/056898.html


https://reviews.llvm.org/D43497

Files:
  include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
  lib/StaticAnalyzer/Core/ExprEngine.cpp
  lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
  test/Analysis/lifetime-extension.cpp
  test/Analysis/temporaries.cpp

Index: test/Analysis/temporaries.cpp
===
--- test/Analysis/temporaries.cpp
+++ test/Analysis/temporaries.cpp
@@ -895,6 +895,33 @@
 }
 } // namespace test_match_constructors_and_destructors
 
+namespace dont_forget_destructor_around_logical_op {
+int glob;
+
+class C {
+public:
+  ~C() { glob = 1; }
+};
+
+C get();
+
+bool is(C);
+
+
+void test(int coin) {
+  // Here temporaries are being cleaned up after && is evaluated. There are two
+  // temporaries: the return value of get() and the elidable copy constructor
+  // of that return value into is(). According to the CFG, we need to cleanup
+  // both of them depending on whether the temporary corresponding to the
+  // return value of get() was initialized. However, for now we don't track
+  // temporaries returned from functions, so we take the wrong branch.
+  coin && is(get()); // no-crash
+  // FIXME: We should have called the destructor, i.e. should be TRUE,
+  // at least when we inline temporary destructors.
+  clang_analyzer_eval(glob == 1); // expected-warning{{UNKNOWN}}
+}
+} // namespace dont_forget_destructor_around_logical_op
+
 #if __cplusplus >= 201103L
 namespace temporary_list_crash {
 class C {
Index: test/Analysis/lifetime-extension.cpp
===
--- test/Analysis/lifetime-extension.cpp
+++ test/Analysis/lifetime-extension.cpp
@@ -1,4 +1,5 @@
-// RUN: %clang_analyze_cc1 -Wno-unused -std=c++11 -analyzer-checker=debug.ExprInspection -verify %s
+// RUN: %clang_analyze_cc1 -Wno-unused -std=c++11 -analyzer-checker=core,debug.ExprInspection -verify %s
+// RUN: %clang_analyze_cc1 -Wno-unused -std=c++11 -analyzer-checker=core,debug.ExprInspection -analyzer-config cfg-temporary-dtors=true -DTEMPORARIES -verify %s
 
 void clang_analyzer_eval(bool);
 
@@ -38,9 +39,142 @@
   const int  = A().j[1]; // no-crash
   const int  = (A().j[1], A().j[0]); // no-crash
 
-  // FIXME: All of these should be TRUE, but constructors aren't inlined.
-  clang_analyzer_eval(x == 1); // expected-warning{{UNKNOWN}}
-  clang_analyzer_eval(y == 3); // expected-warning{{UNKNOWN}}
-  clang_analyzer_eval(z == 2); // expected-warning{{UNKNOWN}}
+  clang_analyzer_eval(x == 1);
+  clang_analyzer_eval(y == 3);
+  clang_analyzer_eval(z == 2);
+#ifdef TEMPORARIES
+ // expected-warning@-4{{TRUE}}
+ // expected-warning@-4{{TRUE}}
+ // expected-warning@-4{{TRUE}}
+#else
+ // expected-warning@-8{{UNKNOWN}}
+ // expected-warning@-8{{UNKNOWN}}
+ // expected-warning@-8{{UNKNOWN}}
+#endif
 }
 } // end namespace pr19539_crash_on_destroying_an_integer
+
+namespace maintain_original_object_address_on_lifetime_extension {
+class C {
+  C **after, **before;
+  bool x;
+
+public:
+  C(bool x, C **after, C **before) : x(x), after(after), before(before) {
+*before = this;
+  }
+
+  // Don't track copies in our tests.
+  C(const C ) : x(c.x), after(nullptr), before(nullptr) {}
+
+  ~C() { if (after) *after = this; }
+
+  operator bool() const { return x; }
+};
+
+void f1() {
+  C *after, *before;
+  {
+const C  = C(1, , );
+  }
+  clang_analyzer_eval(after == before);
+#ifdef TEMPORARIES
+  // expected-warning@-2{{TRUE}}
+#else
+  // expected-warning@-4{{UNKNOWN}}
+#endif
+}
+
+void f2() {
+  C *after, *before;
+  C c = C(1, , );
+  clang_analyzer_eval(after == before);
+#ifdef TEMPORARIES
+  // expected-warning@-2{{TRUE}}
+#else
+  // expected-warning@-4{{UNKNOWN}}
+#endif
+}
+
+void f3(bool coin) {
+  C *after, *before;
+  {
+const C  = coin ? C(1, , ) : C(2, , );
+  }
+  clang_analyzer_eval(after == before);
+#ifdef TEMPORARIES
+  // expected-warning@-2{{TRUE}}
+#else
+  // expected-warning@-4{{UNKNOWN}}
+#endif
+}
+
+void f4(bool coin) {
+  C *after, *before;
+  {
+// no-crash
+const C  = C(coin, , ) ?: C(false, , );
+  }
+  // FIXME: Add support for lifetime extension through binary conditional
+  // operator. Ideally also add support for the binary conditional operator in
+  // C++. Because for now it calls the constructor for the condition twice.
+  if (coin) {
+clang_analyzer_eval(after == before);
+#ifdef TEMPORARIES
+  // expected-warning@-2{{The left operand of '==' is a garbage value}}
+#else
+  // expected-warning@-4{{UNKNOWN}}

[PATCH] D43497: [analyzer] Introduce correct lifetime extension behavior in simple cases.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ created this revision.
NoQ added reviewers: dcoughlin, xazax.hun, a.sidorin, george.karpenkov, szepet.
Herald added subscribers: cfe-commits, rnkovacs.

This patch uses the reference to `MaterializeTemporaryExpr` stored in the 
construction context since https://reviews.llvm.org/D43477 in order to model 
that expression correctly, following the plan described in 
http://lists.llvm.org/pipermail/cfe-dev/2018-February/056898.html

`MaterializeTemporaryExpr` is an expression that takes an rvalue object (or one 
of its xvalue sub-objects) and transforms it into a reference to that object. 
From the analyzer's point of view, the value of the sub-expression would be a 
`NonLoc`, and the value of `MaterializeTemporaryExpr` would be a `Loc` that 
this `NonLoc` is stored in.

For now this behavior has been achieved by creating a `Loc` out of thin air and 
re-binding the `NonLoc` value into the newly created location. This, however, 
is incorrect, because the constructor that has constructed the respective 
`NonLoc` value was operating on a different memory region, so all of the 
subsequent method calls, including the destructor, should have the exact same 
"this" region that the constructor was using.

The problem with behaving correctly is that we've already forgot the original 
storage. It is technically impossible to reliably recover it from the `NonLoc`.

The patch addresses it by relying on the `ConstructionContext` to inform the 
constructor that lifetime extension is taking place, so that the constructor 
could store the current "this" region in the program state for later use. The 
region is uniquely determined by the `MaterializeTemporaryExpr` itself. Then  
when time comes to model `MaterializeTemporaryExpr`, it looks up itself in the 
program state to find the respective region, and avoids the hassle of creating 
a new region and copying the value in case it does indeed find the existing 
correct region.

The temporary region's liveness (in the sense of `removeDeadBindings`) is 
extended until the `MaterializeTemporaryExpr` is resolved, in order to keep the 
store bindings around, because it wouldn't be referenced from anywhere else in 
the program state.

Situations where correct lifetime extension behavior is needed require 
relatively awkward test cases, because most of the time you don't care about 
the object's address as long as its contents are correct.

This patch is enough to assert that for every initialized temporary 
(lifetime-extended or not), a destructor is called (with the correct address) 
on all existing test cases(!). I added a new test case (with 
`BinaryConditionalOperator`) on which this assertion still fails (due to lack 
of construction context and other problems) - i've actually seen this operator 
used on C++ objects. There are more cases that we don't handle yet; i'd see 
what i can do about them.


Repository:
  rC Clang

https://reviews.llvm.org/D43497

Files:
  include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
  lib/StaticAnalyzer/Core/ExprEngine.cpp
  lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
  test/Analysis/lifetime-extension.cpp

Index: test/Analysis/lifetime-extension.cpp
===
--- test/Analysis/lifetime-extension.cpp
+++ test/Analysis/lifetime-extension.cpp
@@ -1,4 +1,5 @@
-// RUN: %clang_analyze_cc1 -Wno-unused -std=c++11 -analyzer-checker=debug.ExprInspection -verify %s
+// RUN: %clang_analyze_cc1 -Wno-unused -std=c++11 -analyzer-checker=core,debug.ExprInspection -verify %s
+// RUN: %clang_analyze_cc1 -Wno-unused -std=c++11 -analyzer-checker=core,debug.ExprInspection -analyzer-config cfg-temporary-dtors=true -DTEMPORARIES -verify %s
 
 void clang_analyzer_eval(bool);
 
@@ -38,9 +39,142 @@
   const int  = A().j[1]; // no-crash
   const int  = (A().j[1], A().j[0]); // no-crash
 
-  // FIXME: All of these should be TRUE, but constructors aren't inlined.
-  clang_analyzer_eval(x == 1); // expected-warning{{UNKNOWN}}
-  clang_analyzer_eval(y == 3); // expected-warning{{UNKNOWN}}
-  clang_analyzer_eval(z == 2); // expected-warning{{UNKNOWN}}
+  clang_analyzer_eval(x == 1);
+  clang_analyzer_eval(y == 3);
+  clang_analyzer_eval(z == 2);
+#ifdef TEMPORARIES
+ // expected-warning@-4{{TRUE}}
+ // expected-warning@-4{{TRUE}}
+ // expected-warning@-4{{TRUE}}
+#else
+ // expected-warning@-8{{UNKNOWN}}
+ // expected-warning@-8{{UNKNOWN}}
+ // expected-warning@-8{{UNKNOWN}}
+#endif
 }
 } // end namespace pr19539_crash_on_destroying_an_integer
+
+namespace maintain_original_object_address_on_lifetime_extension {
+class C {
+  C **after, **before;
+  bool x;
+
+public:
+  C(bool x, C **after, C **before) : x(x), after(after), before(before) {
+*before = this;
+  }
+
+  // Don't track copies in our tests.
+  C(const C ) : x(c.x), after(nullptr), before(nullptr) {}
+
+  ~C() { if (after) *after = this; }
+
+  operator bool() const { return x; }
+};
+
+void f1() {
+  C *after, *before;
+  {
+const C  =