[PATCH] D43149: [analyzer] Fix a crash on destroying a temporary array.

Artem Dergachev via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Fri Feb 9 16:07:32 PST 2018


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

Tried to actually run the analyzer with temporary destructor support on some real code, found two crashes for now - https://reviews.llvm.org/D43144 and this one. In this example, a temporary of type `C[2]` is constructed in the context of `InitListExpr` within `CXXBindTemporaryExpr`:

  `-ExprWithCleanups 0x7f82b9020c28 <line:872:3, col:36> 'std::initializer_list<C>':'std::initializer_list<temporary_list_crash::C>'
    `-CXXFunctionalCastExpr 0x7f82b9020c00 <col:3, col:36> 'std::initializer_list<C>':'std::initializer_list<temporary_list_crash::C>' functional cast to std::initializer_list<C> <NoOp>
      `-CXXStdInitializerListExpr 0x7f82ba0258f8 <col:27, col:36> 'std::initializer_list<C>':'std::initializer_list<temporary_list_crash::C>'
        `-MaterializeTemporaryExpr 0x7f82ba0258e0 <col:27, col:36> 'const temporary_list_crash::C [2]' xvalue
          `-CXXBindTemporaryExpr 0x7f82ba0258c0 <col:27, col:36> 'const temporary_list_crash::C [2]' (CXXTemporary 0x7f82ba0258b8)    *** <=== THIS ONE ***
            `-InitListExpr 0x7f82ba025748 <col:27, col:36> 'const temporary_list_crash::C [2]'
              |-CXXConstructExpr 0x7f82ba025818 <col:28, col:30> 'const temporary_list_crash::C' 'void (const temporary_list_crash::C &) noexcept' elidable
              | `-MaterializeTemporaryExpr 0x7f82ba0257b0 <col:28, col:30> 'const temporary_list_crash::C' lvalue
              |   `-ImplicitCastExpr 0x7f82ba025798 <col:28, col:30> 'const temporary_list_crash::C' <NoOp>
              |     `-CXXBindTemporaryExpr 0x7f82ba024ae8 <col:28, col:30> 'temporary_list_crash::C' (CXXTemporary 0x7f82ba024ae0)
              |       `-CXXTemporaryObjectExpr 0x7f82ba024aa8 <col:28, col:30> 'temporary_list_crash::C' 'void ()'
              `-CXXConstructExpr 0x7f82ba025880 <col:33, col:35> 'const temporary_list_crash::C' 'void (const temporary_list_crash::C &) noexcept' elidable
                `-MaterializeTemporaryExpr 0x7f82ba025868 <col:33, col:35> 'const temporary_list_crash::C' lvalue
                  `-ImplicitCastExpr 0x7f82ba025850 <col:33, col:35> 'const temporary_list_crash::C' <NoOp>
                    `-CXXBindTemporaryExpr 0x7f82ba024b58 <col:33, col:35> 'temporary_list_crash::C' (CXXTemporary 0x7f82ba024b50)
                      `-CXXTemporaryObjectExpr 0x7f82ba024b18 <col:33, col:35> 'temporary_list_crash::C' 'void ()'

We'd have to actually support this case eventually, but for now do the trick that we've always did: unwrap the array type and pretend we're destroying a single object. Once we have the construction context provided for this constructor, we'd have the region here, and in this case we'd also need to construct `ElementRegion` as part of this trick. And even then, calling a destructor of a single array element does not invalidate the whole array for us, because destructors are `const` (unless there are mutable members). So we'd have to do this manually later as well.


Repository:
  rC Clang

https://reviews.llvm.org/D43149

Files:
  lib/StaticAnalyzer/Core/ExprEngine.cpp
  test/Analysis/temporaries.cpp


Index: test/Analysis/temporaries.cpp
===================================================================
--- test/Analysis/temporaries.cpp
+++ test/Analysis/temporaries.cpp
@@ -6,6 +6,8 @@
 extern bool clang_analyzer_warnIfReached();
 void clang_analyzer_checkInlined(bool);
 
+#include "Inputs/system-header-simulator-cxx.h";
+
 struct Trivial {
   Trivial(int x) : value(x) {}
   int value;
@@ -857,3 +859,17 @@
   }
 }
 } // namespace test_match_constructors_and_destructors
+
+#if __cplusplus >= 201103L
+namespace temporary_list_crash {
+class C {
+public:
+  C() {}
+  ~C() {}
+};
+
+void test() {
+  std::initializer_list<C>{C(), C()}; // no-crash
+}
+} // namespace temporary_list_crash
+#endif // C++11
Index: lib/StaticAnalyzer/Core/ExprEngine.cpp
===================================================================
--- lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ lib/StaticAnalyzer/Core/ExprEngine.cpp
@@ -957,18 +957,37 @@
   }
   StmtBldr.generateNode(D.getBindTemporaryExpr(), Pred, State);
 
-  QualType varType = D.getBindTemporaryExpr()->getSubExpr()->getType();
+  QualType T = D.getBindTemporaryExpr()->getSubExpr()->getType();
   // FIXME: Currently CleanDtorState can be empty here due to temporaries being
   // bound to default parameters.
   assert(CleanDtorState.size() <= 1);
   ExplodedNode *CleanPred =
       CleanDtorState.empty() ? Pred : *CleanDtorState.begin();
 
   EvalCallOptions CallOpts;
   CallOpts.IsTemporaryCtorOrDtor = true;
-  if (!MR)
+  if (!MR) {
     CallOpts.IsCtorOrDtorWithImproperlyModeledTargetRegion = true;
-  VisitCXXDestructor(varType, MR, D.getBindTemporaryExpr(),
+
+    // If we have no MR, we still need to unwrap the array to avoid destroying
+    // the whole array at once. Regardless, we'd eventually need to model array
+    // destructors properly, element-by-element.
+    while (const ArrayType *AT = getContext().getAsArrayType(T)) {
+      T = AT->getElementType();
+      CallOpts.IsArrayCtorOrDtor = true;
+    }
+  } else {
+    // We'd eventually need to makeZeroElementRegion() trick here,
+    // but for now we don't have the respective construction contexts,
+    // so MR would always be null in this case. Do nothing for now.
+    // Note that destructors are const, and therefore they don't invalidate
+    // their storage when no mutable members are present. It means that
+    // we cannot model array destructors by simply calling a destructor
+    // for a single element and expect it to invalidate the whole array for us.
+    // We'd have to invalidate it manually somehow. Escaping should work
+    // just fine though.
+  }
+  VisitCXXDestructor(T, MR, D.getBindTemporaryExpr(),
                      /*IsBase=*/false, CleanPred, Dst, CallOpts);
 }
 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D43149.133711.patch
Type: text/x-patch
Size: 2742 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20180210/4670ac5d/attachment.bin>


More information about the cfe-commits mailing list