[PATCH] D43689: [analyzer] Disable constructor inlining when lifetime extension through fields occurs.

2018-04-18 Thread Chih-Hung Hsieh via Phabricator via cfe-commits 
chh added a comment.

This change caused an assertion failure in ExprEngineCXX.cpp:
https://bugs.llvm.org/show_bug.cgi?id=37166


Repository:
  rC Clang

https://reviews.llvm.org/D43689



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

[PATCH] D43689: [analyzer] Disable constructor inlining when lifetime extension through fields occurs.

2018-02-27 Thread Phabricator via Phabricator via cfe-commits 
This revision was automatically updated to reflect the committed changes.
Closed by commit rC326240: [analyzer] Disable constructor inlining when 
lifetime extending through a field. (authored by dergachev, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D43689?vs=135669&id=136133#toc

Repository:
  rC Clang

https://reviews.llvm.org/D43689

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


Index: lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
===
--- lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
+++ lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
@@ -678,6 +678,11 @@
   // the fake temporary target.
   if (CallOpts.IsCtorOrDtorWithImproperlyModeledTargetRegion)
 return CIP_DisallowedOnce;
+
+  // If the temporary is lifetime-extended by binding a smaller object
+  // within it to a reference, automatic destructors don't work properly.
+  if (CallOpts.IsTemporaryLifetimeExtendedViaSubobject)
+return CIP_DisallowedOnce;
 }
 
 break;
Index: lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
===
--- lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
+++ lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
@@ -168,6 +168,18 @@
   break;
 }
 case ConstructionContext::TemporaryObjectKind: {
+  const auto *TOCC = cast(CC);
+  // See if we're lifetime-extended via our field. If so, take a note.
+  // Because automatic destructors aren't quite working in this case.
+  if (const auto *MTE = TOCC->getMaterializedTemporaryExpr()) {
+if (const ValueDecl *VD = MTE->getExtendingDecl()) {
+  assert(VD->getType()->isReferenceType());
+  if (VD->getType()->getPointeeType().getCanonicalType() !=
+  MTE->GetTemporaryExpr()->getType().getCanonicalType()) {
+CallOpts.IsTemporaryLifetimeExtendedViaSubobject = true;
+  }
+}
+  }
   // TODO: Support temporaries lifetime-extended via static references.
   // They'd need a getCXXStaticTempObjectRegion().
   CallOpts.IsTemporaryCtorOrDtor = true;
Index: include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
===
--- include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
+++ include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
@@ -65,6 +65,10 @@
 bool IsArrayCtorOrDtor = false;
 /// This call is a constructor or a destructor of a temporary value.
 bool IsTemporaryCtorOrDtor = false;
+/// This call is a constructor for a temporary that is lifetime-extended
+/// by binding a smaller object within it to a reference, for example
+/// 'const int &x = C().x;'.
+bool IsTemporaryLifetimeExtendedViaSubobject = false;
 
 EvalCallOptions() {}
   };
Index: test/Analysis/lifetime-extension.cpp
===
--- test/Analysis/lifetime-extension.cpp
+++ test/Analysis/lifetime-extension.cpp
@@ -39,18 +39,10 @@
   const int &y = A().j[1]; // no-crash
   const int &z = (A().j[1], A().j[0]); // no-crash
 
-  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
+  // 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}}
 }
 } // end namespace pr19539_crash_on_destroying_an_integer
 
@@ -144,12 +136,7 @@
 const bool &x = C(true, &after, &before).x; // no-crash
   }
   // FIXME: Should be TRUE. Should not warn about garbage value.
-  clang_analyzer_eval(after == before);
-#ifdef TEMPORARIES
-  // expected-warning@-2{{The left operand of '==' is a garbage value}}
-#else
-  // expected-warning@-4{{UNKNOWN}}
-#endif
+  clang_analyzer_eval(after == before); // expected-warning{{UNKNOWN}}
 }
 } // end namespace maintain_original_object_address_on_lifetime_extension
 


Index: lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
===
--- lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
+++ lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
@@ -678,6 +678,11 @@
   // the fake temporary target.
   if (CallOpts.IsCtorOrDtorWithImproperlyModeledTargetRegion)
 return CIP_DisallowedOnce;
+
+  // If the temporary is lifetime-extended by binding a smal

[PATCH] D43689: [analyzer] Disable constructor inlining when lifetime extension through fields occurs.

2018-02-27 Thread Artem Dergachev via Phabricator via cfe-commits 
NoQ added inline comments.



Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h:70
+/// by binding a smaller object within it to a reference.
+bool IsTemporaryLifetimeExtendedViaSubobject = false;
 

dcoughlin wrote:
> Would you be willing to add a tiny code example to the comment? I.e., `const 
> int &x = C().x`
Fixed in the commit.


Repository:
  rC Clang

https://reviews.llvm.org/D43689



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

[PATCH] D43689: [analyzer] Disable constructor inlining when lifetime extension through fields occurs.

2018-02-24 Thread Artem Dergachev via Phabricator via cfe-commits 
NoQ added inline comments.



Comment at: lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:177
+  assert(VD->getType()->isReferenceType());
+  if (VD->getType()->getPointeeType().getCanonicalType() !=
+  MTE->GetTemporaryExpr()->getType().getCanonicalType()) {

dcoughlin wrote:
> I *think* this is safe. But it seems like it would be more direct to use 
> skipRValueSubobjectAdjustments() and check for sub-object adjustments since 
> ultimately that is what you care about, right?
> skipRValueSubobjectAdjustments()

This function doesn't do anything anymore (since rC288563) (in most cases, at 
least). We now have lvalue sub-objects adjustments that are outside of 
`MaterializeTemporaryExpr`, and i'm trying to see if there are any by comparing 
the variable type with the temporary type. If there are still any rvalue 
sub-object adjustments present in the AST, then i'd have to skip them, as an 
additional step.


Repository:
  rC Clang

https://reviews.llvm.org/D43689



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

[PATCH] D43689: [analyzer] Disable constructor inlining when lifetime extension through fields occurs.

2018-02-24 Thread Devin Coughlin via Phabricator via cfe-commits 
dcoughlin accepted this revision.
dcoughlin added inline comments.
This revision is now accepted and ready to land.



Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h:70
+/// by binding a smaller object within it to a reference.
+bool IsTemporaryLifetimeExtendedViaSubobject = false;
 

Would you be willing to add a tiny code example to the comment? I.e., `const 
int &x = C().x`



Comment at: lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:177
+  assert(VD->getType()->isReferenceType());
+  if (VD->getType()->getPointeeType().getCanonicalType() !=
+  MTE->GetTemporaryExpr()->getType().getCanonicalType()) {

I *think* this is safe. But it seems like it would be more direct to use 
skipRValueSubobjectAdjustments() and check for sub-object adjustments since 
ultimately that is what you care about, right?


Repository:
  rC Clang

https://reviews.llvm.org/D43689



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

[PATCH] D43689: [analyzer] Disable constructor inlining when lifetime extension through fields occurs.

2018-02-23 Thread 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.

As i mentioned in https://reviews.llvm.org/D43497, automatic destructors are 
missing in the CFG in situations like

  const int &x = C().x;

For now i'd rather disable construction inlining, because inlining constructors 
while doing nothing on destructors is very bad.


Repository:
  rC Clang

https://reviews.llvm.org/D43689

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


Index: test/Analysis/lifetime-extension.cpp
===
--- test/Analysis/lifetime-extension.cpp
+++ test/Analysis/lifetime-extension.cpp
@@ -39,18 +39,10 @@
   const int &y = A().j[1]; // no-crash
   const int &z = (A().j[1], A().j[0]); // no-crash
 
-  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
+  // 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}}
 }
 } // end namespace pr19539_crash_on_destroying_an_integer
 
@@ -144,12 +136,7 @@
 const bool &x = C(true, &after, &before).x; // no-crash
   }
   // FIXME: Should be TRUE. Should not warn about garbage value.
-  clang_analyzer_eval(after == before);
-#ifdef TEMPORARIES
-  // expected-warning@-2{{The left operand of '==' is a garbage value}}
-#else
-  // expected-warning@-4{{UNKNOWN}}
-#endif
+  clang_analyzer_eval(after == before); // expected-warning{{UNKNOWN}}
 }
 } // end namespace maintain_original_object_address_on_lifetime_extension
 
Index: lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
===
--- lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
+++ lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
@@ -678,6 +678,11 @@
   // the fake temporary target.
   if (CallOpts.IsCtorOrDtorWithImproperlyModeledTargetRegion)
 return CIP_DisallowedOnce;
+
+  // If the temporary is lifetime-extended by binding a smaller object
+  // within it to a reference, automatic destructors don't work properly.
+  if (CallOpts.IsTemporaryLifetimeExtendedViaSubobject)
+return CIP_DisallowedOnce;
 }
 
 break;
Index: lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
===
--- lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
+++ lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
@@ -168,6 +168,18 @@
   break;
 }
 case ConstructionContext::TemporaryObjectKind: {
+  const auto *TOCC = cast(CC);
+  // See if we're lifetime-extended via our field. If so, take a note.
+  // Because automatic destructors aren't quite working in this case.
+  if (const auto *MTE = TOCC->getMaterializedTemporaryExpr()) {
+if (const ValueDecl *VD = MTE->getExtendingDecl()) {
+  assert(VD->getType()->isReferenceType());
+  if (VD->getType()->getPointeeType().getCanonicalType() !=
+  MTE->GetTemporaryExpr()->getType().getCanonicalType()) {
+CallOpts.IsTemporaryLifetimeExtendedViaSubobject = true;
+  }
+}
+  }
   // TODO: Support temporaries lifetime-extended via static references.
   // They'd need a getCXXStaticTempObjectRegion().
   CallOpts.IsTemporaryCtorOrDtor = true;
Index: include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
===
--- include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
+++ include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
@@ -65,6 +65,9 @@
 bool IsArrayCtorOrDtor = false;
 /// This call is a constructor or a destructor of a temporary value.
 bool IsTemporaryCtorOrDtor = false;
+/// This call is a constructor for a temporary that is lifetime-extended
+/// by binding a smaller object within it to a reference.
+bool IsTemporaryLifetimeExtendedViaSubobject = false;
 
 EvalCallOptions() {}
   };


Index: test/Analysis/lifetime-extension.cpp
===
--- test/Analysis/lifetime-extension.cpp
+++ test/Analysis/lifetime-extension.cpp
@@ -39,18 +39,10 @@
   const int &y = A().j[1]; // no-crash
   const int &z = (A().j[1], A().j[0]); // no-crash
 
-  clang_analyzer_eval(x == 1);
-  clang_analyzer_eval(y == 3);
-  clang_analyzer_ev