[PATCH] D59573: [analyzer] C++17: PR41142: Ignore transparent InitListExprs when finding construction contexts.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ created this revision.
NoQ added reviewers: dcoughlin, xazax.hun, a_sidorin, rnkovacs, mikhail.ramalho, Szelethus, baloghadamsoftware, Charusso, alexfh.
Herald added subscribers: cfe-commits, jdoerfert, dkrupp, donat.nagy, a.sidorin, szepet.
Herald added a project: clang.

This addresses a few simplified crashes similar to the one in in https://bugs.llvm.org/show_bug.cgi?id=41142 but there's more bug synergy there, so more fixes would be necessary.

Here's the crash:

  class A {};
  class B: A {};
  B boo();
  void foo1() {
    B b { boo() }; // no-crash
  }

The analyzer evaluates `{ boo() }` (an `InitListExpr`) to a `compoundVal{X}`, where `X` is whatever `boo()` evaluates to. The new aggregate initialization facility introduced in D59054 <https://reviews.llvm.org/D59054> would now be triggered by the presence of the `compoundVal{}`, even though the object is not an aggregate (base classes are allowed in aggregates in C++17, but they aren't allowed to be private or virtual).

F8485527: x72r0730bbc11.jpg <https://reviews.llvm.org/F8485527>

Note that:

1. If we remove the assertion and proceed, what would happen is it'll try to bind the whole object `b` to the region of its base class `A` within variable `b`, which is incorrect. `RegionStore` would probably not care in this specific case, but with multiple inheritance it'll probably also lead to actual incorrect bindings.
2. It is in fact irrelevant whether the object is an aggregate - the incorrect behavior would happen anyway. The real problem is that the `compoundVal{}` doesn't mean what we think: we expected it to describe a big object that is composed of (0 or more) smaller objects, but in fact it's transparently describing the single object within it, much like the `InitListExpr` it came from. I'd prefer to declare such `compoundVal{}`s as ill-formed.
3. The general problem with compound values is that it's unobvious what sort of object does it describe. I.e., it's impossible to figure out whether `compoundVal{X}` corresponds to an object of type `A` (probably not) or an object of type `B` (that's what we've got) or of some other type that contains a `B` inside it (that's what we've expected).

This patch fixes neither of these three problems. Instead, what it does is makes sure that there's a `ConstructionContext` to the CFG for the return-by-value function call `boo()`. As a side effect for this change, even though we still produce an ill-formed `compoundVal{}` as the value of expression `{ boo() }`, we never experience a need to make a //Store// binding out of it. Instead, the call is correctly treated as a constructor that has already initialized its target region by the time it ends, so the crashing code path is no longer triggered. Another side effect of this change is that the value no longer looks like `compoundVal{conj_$2<B>}`, but more like `compoundVal{lazyCompoundVal{b}}` - but both are ill-formed anyway.

Now, the original test case in PR41142 is still not fixed. With the same definition of `B`, the original test case can be written as follows:

  B recursive() {
    B b { recursive() };
  }

The problem here is that we inevitably hit the recursion limit, and then...

4. Well, something weird happens. I'll have to have a closer look, but one weird thing i noticed is that the value for `{ boo() }` ends up being `compoundVal{Unknown}`, as if `bindReturnValue()` never gets triggered.

Also,

5. Regardless of the recursion, we're still in trouble every time we don't find a construction context, and for now there is a more or less indefinite number of situations in which this can happen. We should try to be more defensive somehow.

I'll think a bit more to see if fixing these ill-formed `compoundVal{}`s fixes the problem. In the worst case, i guess we can remove the assertion: we'd behave incorrectly incorrect and crashing from time to time (due to multiple inheritance), but it'll hopefully happen less often than before we had D59054 <https://reviews.llvm.org/D59054>.


Repository:
  rC Clang

https://reviews.llvm.org/D59573

Files:
  clang/lib/Analysis/CFG.cpp
  clang/test/Analysis/cfg-rich-constructors.cpp
  clang/test/Analysis/initializer.cpp


Index: clang/test/Analysis/initializer.cpp
===================================================================
--- clang/test/Analysis/initializer.cpp
+++ clang/test/Analysis/initializer.cpp
@@ -242,4 +242,22 @@
   B &&bcr = C({{}}); // no-crash
 #endif
 }
+} // namespace CXX17_aggregate_construction
+
+namespace CXX17_transparent_init_list_exprs {
+class A {};
+
+class B: private A {};
+
+B boo();
+void foo1() {
+  B b { boo() }; // no-crash
+}
+
+class C: virtual public A {};
+
+C coo();
+void foo2() {
+  C c { coo() }; // no-crash
 }
+} // namespace CXX17_transparent_init_list_exprs
Index: clang/test/Analysis/cfg-rich-constructors.cpp
===================================================================
--- clang/test/Analysis/cfg-rich-constructors.cpp
+++ clang/test/Analysis/cfg-rich-constructors.cpp
@@ -1043,3 +1043,23 @@
   C c(variadic(0 ? c : 0)); // no-crash
 }
 } // namespace variadic_function_arguments
+
+// CHECK: void testTransparentInitListExprs()
+// CHECK:        [B1]
+// CHECK-NEXT:     1: getC
+// CHECK-NEXT:     2: [B1.1] (ImplicitCastExpr, FunctionToPointerDecay, class transparent_init_list_exprs::C (*)(void))
+// CXX11-ELIDE-NEXT:     3: [B1.2]() (CXXRecordTypedCall, [B1.4], [B1.5])
+// CXX11-NOELIDE-NEXT:     3: [B1.2]() (CXXRecordTypedCall, [B1.4])
+// CXX11-NEXT:     4: [B1.3]
+// CXX11-NEXT:     5: {[B1.4]} (CXXConstructExpr, [B1.6], class transparent_init_list_exprs::C)
+// CXX11-NEXT:     6: transparent_init_list_exprs::C c{getC()};
+// CXX17-NEXT:     3: [B1.2]() (CXXRecordTypedCall, [B1.5])
+// CXX17-NEXT:     4: {[B1.3]}
+// CXX17-NEXT:     5: transparent_init_list_exprs::C c{getC()};
+namespace transparent_init_list_exprs {
+class C {};
+C getC();
+void testTransparentInitListExprs() {
+  C c{getC()};
+}
+} // namespace transparent_init_list_exprs
Index: clang/lib/Analysis/CFG.cpp
===================================================================
--- clang/lib/Analysis/CFG.cpp
+++ clang/lib/Analysis/CFG.cpp
@@ -1378,6 +1378,15 @@
     findConstructionContexts(Layer, CO->getRHS());
     break;
   }
+  case Stmt::InitListExprClass: {
+    auto *ILE = cast<InitListExpr>(Child);
+    if (ILE->isTransparent()) {
+      findConstructionContexts(Layer, ILE->getInit(0));
+      break;
+    }
+    // TODO: Handle other cases. For now, fail to find construction contexts.
+    break;
+  }
   default:
     break;
   }


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D59573.191423.patch
Type: text/x-patch
Size: 2378 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20190320/81d8c737/attachment.bin>