[PATCH] D61545: [analyzer] Fix a crash in RVO from within blocks.
This revision was automatically updated to reflect the committed changes.
Closed by commit rC360202: [analyzer] Fix a crash when doing RVO from within
blocks. (authored by dergachev, committed by ).
Changed prior to commit:
https://reviews.llvm.org/D61545?vs=198535&id=198555#toc
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D61545/new/
https://reviews.llvm.org/D61545
Files:
lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
test/Analysis/copy-elision.mm
Index: lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
===
--- lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
+++ lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
@@ -196,6 +196,12 @@
// able to find construction context at all.
break;
}
+if (isa(CallerLCtx)) {
+ // Unwrap block invocation contexts. They're mostly part of
+ // the current stack frame.
+ CallerLCtx = CallerLCtx->getParent();
+ assert(!isa(CallerLCtx));
+}
return prepareForObjectConstruction(
cast(SFC->getCallSite()), State, CallerLCtx,
RTC->getConstructionContext(), CallOpts);
Index: test/Analysis/copy-elision.mm
===
--- test/Analysis/copy-elision.mm
+++ test/Analysis/copy-elision.mm
@@ -0,0 +1,18 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core -fblocks -verify %s
+
+// expected-no-diagnostics
+
+namespace block_rvo_crash {
+struct A {};
+
+A getA();
+void use(A a) {}
+
+void foo() {
+ // This used to crash when finding construction context for getA()
+ // (which is use()'s argument due to RVO).
+ use(^{
+return getA(); // no-crash
+ }());
+}
+} // namespace block_rvo_crash
Index: lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
===
--- lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
+++ lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
@@ -196,6 +196,12 @@
// able to find construction context at all.
break;
}
+if (isa(CallerLCtx)) {
+ // Unwrap block invocation contexts. They're mostly part of
+ // the current stack frame.
+ CallerLCtx = CallerLCtx->getParent();
+ assert(!isa(CallerLCtx));
+}
return prepareForObjectConstruction(
cast(SFC->getCallSite()), State, CallerLCtx,
RTC->getConstructionContext(), CallOpts);
Index: test/Analysis/copy-elision.mm
===
--- test/Analysis/copy-elision.mm
+++ test/Analysis/copy-elision.mm
@@ -0,0 +1,18 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core -fblocks -verify %s
+
+// expected-no-diagnostics
+
+namespace block_rvo_crash {
+struct A {};
+
+A getA();
+void use(A a) {}
+
+void foo() {
+ // This used to crash when finding construction context for getA()
+ // (which is use()'s argument due to RVO).
+ use(^{
+return getA(); // no-crash
+ }());
+}
+} // namespace block_rvo_crash
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D61545: [analyzer] Fix a crash in RVO from within blocks.
dcoughlin accepted this revision. dcoughlin added a comment. Looks good to me. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61545/new/ https://reviews.llvm.org/D61545 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D61545: [analyzer] Fix a crash in RVO from within blocks.
NoQ updated this revision to Diff 198535.
NoQ added a comment.
Remove unused declaration from the test.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D61545/new/
https://reviews.llvm.org/D61545
Files:
clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
clang/test/Analysis/copy-elision.mm
Index: clang/test/Analysis/copy-elision.mm
===
--- /dev/null
+++ clang/test/Analysis/copy-elision.mm
@@ -0,0 +1,18 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core -fblocks -verify %s
+
+// expected-no-diagnostics
+
+namespace block_rvo_crash {
+struct A {};
+
+A getA();
+void use(A a) {}
+
+void foo() {
+ // This used to crash when finding construction context for getA()
+ // (which is use()'s argument due to RVO).
+ use(^{
+return getA(); // no-crash
+ }());
+}
+} // namespace block_rvo_crash
Index: clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
===
--- clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
+++ clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
@@ -196,6 +196,12 @@
// able to find construction context at all.
break;
}
+if (isa(CallerLCtx)) {
+ // Unwrap block invocation contexts. They're mostly part of
+ // the current stack frame.
+ CallerLCtx = CallerLCtx->getParent();
+ assert(!isa(CallerLCtx));
+}
return prepareForObjectConstruction(
cast(SFC->getCallSite()), State, CallerLCtx,
RTC->getConstructionContext(), CallOpts);
Index: clang/test/Analysis/copy-elision.mm
===
--- /dev/null
+++ clang/test/Analysis/copy-elision.mm
@@ -0,0 +1,18 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core -fblocks -verify %s
+
+// expected-no-diagnostics
+
+namespace block_rvo_crash {
+struct A {};
+
+A getA();
+void use(A a) {}
+
+void foo() {
+ // This used to crash when finding construction context for getA()
+ // (which is use()'s argument due to RVO).
+ use(^{
+return getA(); // no-crash
+ }());
+}
+} // namespace block_rvo_crash
Index: clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
===
--- clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
+++ clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
@@ -196,6 +196,12 @@
// able to find construction context at all.
break;
}
+if (isa(CallerLCtx)) {
+ // Unwrap block invocation contexts. They're mostly part of
+ // the current stack frame.
+ CallerLCtx = CallerLCtx->getParent();
+ assert(!isa(CallerLCtx));
+}
return prepareForObjectConstruction(
cast(SFC->getCallSite()), State, CallerLCtx,
RTC->getConstructionContext(), CallOpts);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D61545: [analyzer] Fix a crash in RVO from within blocks.
NoQ updated this revision to Diff 198342.
NoQ added a comment.
No, this is all wrong. We shouldn't unwrap to the parent stack frame here. We
should end up exactly in the location context in which the call has happened.
But we should still unwrap the block invocation context, as an exception,
because it's just a weird location context that goes before the stack frame but
still has its corresponding `AnalysisDeclContext` tied to the *callee* stack
frame.
These block invocation contexts are used, like, once, to help modeling
captures. I should probably try to get rid of them entirely.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D61545/new/
https://reviews.llvm.org/D61545
Files:
clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
clang/test/Analysis/copy-elision.mm
Index: clang/test/Analysis/copy-elision.mm
===
--- /dev/null
+++ clang/test/Analysis/copy-elision.mm
@@ -0,0 +1,19 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core -fblocks -verify %s
+
+// expected-no-diagnostics
+
+namespace block_rvo_crash {
+struct A {};
+struct B {};
+
+A getA();
+void use(A a) {}
+
+void foo() {
+ // This used to crash when finding construction context for getA()
+ // (which is use()'s argument due to RVO).
+ use(^{
+return getA(); // no-crash
+ }());
+}
+} // namespace block_rvo_crash
Index: clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
===
--- clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
+++ clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
@@ -196,6 +196,12 @@
// able to find construction context at all.
break;
}
+if (isa(CallerLCtx)) {
+ // Unwrap block invocation contexts. They're mostly part of
+ // the current stack frame.
+ CallerLCtx = CallerLCtx->getParent();
+ assert(!isa(CallerLCtx));
+}
return prepareForObjectConstruction(
cast(SFC->getCallSite()), State, CallerLCtx,
RTC->getConstructionContext(), CallOpts);
Index: clang/test/Analysis/copy-elision.mm
===
--- /dev/null
+++ clang/test/Analysis/copy-elision.mm
@@ -0,0 +1,19 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core -fblocks -verify %s
+
+// expected-no-diagnostics
+
+namespace block_rvo_crash {
+struct A {};
+struct B {};
+
+A getA();
+void use(A a) {}
+
+void foo() {
+ // This used to crash when finding construction context for getA()
+ // (which is use()'s argument due to RVO).
+ use(^{
+return getA(); // no-crash
+ }());
+}
+} // namespace block_rvo_crash
Index: clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
===
--- clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
+++ clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
@@ -196,6 +196,12 @@
// able to find construction context at all.
break;
}
+if (isa(CallerLCtx)) {
+ // Unwrap block invocation contexts. They're mostly part of
+ // the current stack frame.
+ CallerLCtx = CallerLCtx->getParent();
+ assert(!isa(CallerLCtx));
+}
return prepareForObjectConstruction(
cast(SFC->getCallSite()), State, CallerLCtx,
RTC->getConstructionContext(), CallOpts);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D61545: [analyzer] Fix a crash in RVO from within blocks.
Charusso accepted this revision.
Charusso added a comment.
This revision is now accepted and ready to land.
I think the entire `LocationContext` stuff is a huge design issue, and you used
its functionality without any hack. If you would rename the `getStackFrame` to
`getNextStackFrame` or something, it is clear it is not a getter and traversing
every frame until the top-frame.
Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:203
+cast(SFC->getCallSite()), State, CallerSFC,
RTC->getConstructionContext(), CallOpts);
} else {
The smallest the scope between the definition and its usage, the better the
code. Could you put the new code immediately before the return statement?
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D61545/new/
https://reviews.llvm.org/D61545
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D61545: [analyzer] Fix a crash in RVO from within blocks.
NoQ created this revision.
NoQ added reviewers: dcoughlin, xazax.hun, a_sidorin, rnkovacs,
mikhail.ramalho, Szelethus, baloghadamsoftware, Charusso.
Herald added subscribers: cfe-commits, dkrupp, donat.nagy, a.sidorin, szepet.
Herald added a project: clang.
While blocks are an Apple extension to C, i'm adding everybody because my
mistake that i'm fixing here may hurt us more when we add more kinds of
location contexts, so we shouldn't be making it anywhere.
In the attached test case, `getA()` returns an object by value, and therefore
requires a construction context. Its construction context is that of a value
that's immediately* returned (by value, without any sort of conversion), so
copy elision (namely, RVO) applies. This means that we unwind the stack of
`LocationContext`s in order to see where is the current callee called from.
This way we obtain the construction context for the call site, and from that we
can figure out what object is constructed. In this case it's going to be the
first-and-only argument of `use()`. This is all good.
In this case RVO is applied to a return value of an anonymous block that's
declared only to be immediately called and discarded. The Static Analyzer
models block calls by putting two location contexts on the stack: a
`BlockInvocationContext` followed by the actual `StackFrameContext`. I don't
really know why it does that :) but that's not important, because if we
introduce more kinds of location contexts, it'll look similarly anyway.
Therefore the correct idiom for obtaining the parent stack frame is to first
obtain the parent, and then don't forget to obtain its stack frame, which is
not necessarily the parent itself. This is the mistake that i made in my RVO
code that i'm fixing here.
The code was crashing because it was looking up the call site in a `CFGStmtMap`
for the wrong CFG (obtained from a wrong location context). This was happening
in `CallEvent::getCalleeStackFrame()`.
Repository:
rC Clang
https://reviews.llvm.org/D61545
Files:
clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
clang/test/Analysis/copy-elision.mm
Index: clang/test/Analysis/copy-elision.mm
===
--- /dev/null
+++ clang/test/Analysis/copy-elision.mm
@@ -0,0 +1,19 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core -fblocks -verify %s
+
+// expected-no-diagnostics
+
+namespace block_rvo_crash {
+struct A {};
+struct B {};
+
+A getA();
+void use(A a) {}
+
+void foo() {
+ // This used to crash when finding construction context for getA()
+ // (which is use()'s argument due to RVO).
+ use(^{
+return getA(); // no-crash
+ }());
+}
+} // namespace block_rvo_crash
Index: clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
===
--- clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
+++ clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
@@ -188,6 +188,8 @@
// top frame of the analysis.
const StackFrameContext *SFC = LCtx->getStackFrame();
if (const LocationContext *CallerLCtx = SFC->getParent()) {
+// Skip non-stack-frame contexts.
+const StackFrameContext *CallerSFC = CallerLCtx->getStackFrame();
auto RTC = (*SFC->getCallSiteBlock())[SFC->getIndex()]
.getAs();
if (!RTC) {
@@ -197,7 +199,7 @@
break;
}
return prepareForObjectConstruction(
-cast(SFC->getCallSite()), State, CallerLCtx,
+cast(SFC->getCallSite()), State, CallerSFC,
RTC->getConstructionContext(), CallOpts);
} else {
// We are on the top frame of the analysis. We do not know where is the
Index: clang/test/Analysis/copy-elision.mm
===
--- /dev/null
+++ clang/test/Analysis/copy-elision.mm
@@ -0,0 +1,19 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core -fblocks -verify %s
+
+// expected-no-diagnostics
+
+namespace block_rvo_crash {
+struct A {};
+struct B {};
+
+A getA();
+void use(A a) {}
+
+void foo() {
+ // This used to crash when finding construction context for getA()
+ // (which is use()'s argument due to RVO).
+ use(^{
+return getA(); // no-crash
+ }());
+}
+} // namespace block_rvo_crash
Index: clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
===
--- clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
+++ clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
@@ -188,6 +188,8 @@
// top frame of the analysis.
const StackFrameContext *SFC = LCtx->getStackFrame();
if (const LocationContext *CallerLCtx = SFC->getParent()) {
+// Skip non-stack-frame contexts.
+const StackFrameContext *CallerSFC = CallerLCtx->getStackFrame();
auto RTC = (*SFC->getCallSiteBlock())[SFC->getIndex()]
.getAs();
if (!RTC) {
@@ -197,7 +199,7 @@
