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;
}
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;
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
