<html><head><meta http-equiv="Content-Type" content="text/html charset=us-ascii"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><div>Per offline discussion, we've come up with some things that should be done about this:</div><div><br></div><div>1) In -Asserts builds, we should be more conservative and report that there is no available definition of the member function.</div><div>2) The assert should remain present so that we find out about these issues in +Asserts builds.</div><div>3) We should have DynamicTypePropagation record DynamicTypeInfo as "invalid" in some way if a region passes through a reinterpret_cast, and</div><div>4) We should then whitelist cases where we can believe the reinterpret_cast (e.g. casting from void * or char *).</div><div>5) We should probably be believing other types of casts as well.</div><div>6) There should be warnings for unsafe uses of reinterpret_cast, which is what caused this particular crash.</div><div><br></div><div>1 will happen after I send this e-mail. I'll try to get to 2, 3, 4, and 5 soon (unfortunately, if we just add the assert back we'll just get back to the failure right now). I've filed <<a href="rdar://problem/12287087">rdar://problem/12287087</a>> about 3, 4, and 5, and <a href="http://llvm.org/bugs/show_bug.cgi?id=13824">PR13824</a> for 6.</div><div><br></div><div>Jordan</div><div><br></div><br><div><div>On Sep 12, 2012, at 9:42 , Anna Zaks <<a href="mailto:ganna@apple.com">ganna@apple.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><meta http-equiv="Content-Type" content="text/html charset=us-ascii"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; ">I think this logic should go into DynamicTypePropagation checker - you would specifically implement a rule on what should be done when dealing with a cast (or any other) statement. Similarly to how it's done for ObjC: <div><span style="font-family: Monaco; font-size: 11px; "><br></span></div><div><span style="font-family: Monaco; font-size: 11px; ">DynamicTypePropagation::checkPostStmt(</span><span style="font-family: Monaco; font-size: 11px; color: rgb(147, 26, 104); ">const</span><span style="font-family: Monaco; font-size: 11px; "> </span><span style="font-family: Monaco; font-size: 11px; color: rgb(0, 97, 65); ">ImplicitCastExpr</span><span style="font-family: Monaco; font-size: 11px; "> *CastE..</span></div><div><span style="font-family: Monaco; font-size: 11px; "><br></span></div><div><div style="margin: 0px; font-size: 11px; font-family: Monaco; color: rgb(78, 144, 114); ">// Return a better dynamic type if one can be derived from the cast.</div><div style="margin: 0px; font-size: 11px; font-family: Monaco; color: rgb(78, 144, 114); ">// Compare the current dynamic type of the region and the new type to which we</div><div style="margin: 0px; font-size: 11px; font-family: Monaco; color: rgb(78, 144, 114); ">// are casting. If the new type is lower in the inheritance hierarchy, pick it.</div></div><div><div style="margin: 0px; font-size: 11px; font-family: Monaco; ">DynamicTypePropagation::getBetterObjCType(..)</div><div><br></div><div>The purpose of that checker is to keep the logic for determining the type info in one place.</div><div><br></div><div>Cheers,</div><div>Anna.</div><div><br><div><div>On Sep 11, 2012, at 11:47 AM, Jordan Rose <<a href="mailto:jordan_rose@apple.com">jordan_rose@apple.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite">Author: jrose<br>Date: Tue Sep 11 13:47:13 2012<br>New Revision: 163644<br><br>URL: <a href="http://llvm.org/viewvc/llvm-project?rev=163644&view=rev">http://llvm.org/viewvc/llvm-project?rev=163644&view=rev</a><br>Log:<br>[analyzer] Use the static type for a virtual call if the dynamic type is worse.<br><br>reinterpret_cast does not provide any of the usual type information that<br>static_cast or dynamic_cast provide -- only the new type. This can get us<br>in a situation where the dynamic type info for an object is actually a<br>superclass of the static type, which does not match what CodeGen does at all.<br>In these cases, just fall back to the static type as the best possible type<br>for devirtualization.<br><br>Should fix the crashes on our internal buildbot.<br><br>Modified:<br> cfe/trunk/lib/StaticAnalyzer/Core/CallEvent.cpp<br> cfe/trunk/test/Analysis/inlining/dyn-dispatch-bifurcate.cpp<br><br>Modified: cfe/trunk/lib/StaticAnalyzer/Core/CallEvent.cpp<br>URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/CallEvent.cpp?rev=163644&r1=163643&r2=163644&view=diff">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/CallEvent.cpp?rev=163644&r1=163643&r2=163644&view=diff</a><br>==============================================================================<br>--- cfe/trunk/lib/StaticAnalyzer/Core/CallEvent.cpp (original)<br>+++ cfe/trunk/lib/StaticAnalyzer/Core/CallEvent.cpp Tue Sep 11 13:47:13 2012<br>@@ -433,14 +433,21 @@<br> if (!RD || !RD->hasDefinition())<br> return RuntimeDefinition();<br><br>- // Find the decl for this method in that class.<br>- const CXXMethodDecl *Result = MD->getCorrespondingMethodInClass(RD, true);<br>+ const CXXMethodDecl *Result;<br>+ if (MD->getParent()->isDerivedFrom(RD)) {<br>+ // If our static type info is better than our dynamic type info, don't<br>+ // bother doing a search. Just use the static method.<br>+ Result = MD;<br>+ } else {<br>+ // Otherwise, find the decl for the method in the dynamic class.<br>+ Result = MD->getCorrespondingMethodInClass(RD, true);<br>+ }<br>+<br> if (!Result) {<br> // We might not even get the original statically-resolved method due to<br> // some particularly nasty casting (e.g. casts to sister classes).<br> // However, we should at least be able to search up and down our own class<br> // hierarchy, and some real bugs have been caught by checking this.<br>- assert(!MD->getParent()->isDerivedFrom(RD) && "Bad DynamicTypeInfo");<br> assert(!RD->isDerivedFrom(MD->getParent()) && "Couldn't find known method");<br> return RuntimeDefinition();<br> }<br><br>Modified: cfe/trunk/test/Analysis/inlining/dyn-dispatch-bifurcate.cpp<br>URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/inlining/dyn-dispatch-bifurcate.cpp?rev=163644&r1=163643&r2=163644&view=diff">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/inlining/dyn-dispatch-bifurcate.cpp?rev=163644&r1=163643&r2=163644&view=diff</a><br>==============================================================================<br>--- cfe/trunk/test/Analysis/inlining/dyn-dispatch-bifurcate.cpp (original)<br>+++ cfe/trunk/test/Analysis/inlining/dyn-dispatch-bifurcate.cpp Tue Sep 11 13:47:13 2012<br>@@ -15,3 +15,19 @@<br> A a;<br> clang_analyzer_eval(a.get() == 0); // expected-warning{{TRUE}}<br> }<br>+<br>+<br>+namespace ReinterpretDisruptsDynamicTypeInfo {<br>+ class Parent {};<br>+<br>+ class Child : public Parent {<br>+ public:<br>+ virtual int foo() { return 42; }<br>+ };<br>+<br>+ void test(Parent *a) {<br>+ Child *b = reinterpret_cast<Child *>(a);<br>+ if (!b) return;<br>+ clang_analyzer_eval(b->foo() == 42); // expected-warning{{TRUE}} expected-warning{{UNKNOWN}}<br>+ }<br>+}<br><br><br>_______________________________________________<br>cfe-commits mailing list<br><a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br><a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br></blockquote></div><br></div></div></div></blockquote></div><br></body></html>