[cfe-commits] r163644 - in /cfe/trunk: lib/StaticAnalyzer/Core/CallEvent.cpp test/Analysis/inlining/dyn-dispatch-bifurcate.cpp

[Jordan Rose]

Per offline discussion, we've come up with some things that should be done about this:

1) In -Asserts builds, we should be more conservative and report that there is no available definition of the member function.
2) The assert should remain present so that we find out about these issues in +Asserts builds.
3) We should have DynamicTypePropagation record DynamicTypeInfo as "invalid" in some way if a region passes through a reinterpret_cast, and
4) We should then whitelist cases where we can believe the reinterpret_cast (e.g. casting from void * or char *).
5) We should probably be believing other types of casts as well.
6) There should be warnings for unsafe uses of reinterpret_cast, which is what caused this particular crash.

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 <rdar://problem/12287087> about 3, 4, and 5, and PR13824 for 6.

Jordan


On Sep 12, 2012, at 9:42 , Anna Zaks <ganna at apple.com> wrote:

> 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: 
> 
> DynamicTypePropagation::checkPostStmt(const ImplicitCastExpr *CastE..
> 
> // Return a better dynamic type if one can be derived from the cast.
> // Compare the current dynamic type of the region and the new type to which we
> // are casting. If the new type is lower in the inheritance hierarchy, pick it.
> DynamicTypePropagation::getBetterObjCType(..)
> 
> The purpose of that checker is to keep the logic for determining the type info in one place.
> 
> Cheers,
> Anna.
> 
> On Sep 11, 2012, at 11:47 AM, Jordan Rose <jordan_rose at apple.com> wrote:
> 
>> Author: jrose
>> Date: Tue Sep 11 13:47:13 2012
>> New Revision: 163644
>> 
>> URL: http://llvm.org/viewvc/llvm-project?rev=163644&view=rev
>> Log:
>> [analyzer] Use the static type for a virtual call if the dynamic type is worse.
>> 
>> reinterpret_cast does not provide any of the usual type information that
>> static_cast or dynamic_cast provide -- only the new type. This can get us
>> in a situation where the dynamic type info for an object is actually a
>> superclass of the static type, which does not match what CodeGen does at all.
>> In these cases, just fall back to the static type as the best possible type
>> for devirtualization.
>> 
>> Should fix the crashes on our internal buildbot.
>> 
>> Modified:
>>    cfe/trunk/lib/StaticAnalyzer/Core/CallEvent.cpp
>>    cfe/trunk/test/Analysis/inlining/dyn-dispatch-bifurcate.cpp
>> 
>> Modified: cfe/trunk/lib/StaticAnalyzer/Core/CallEvent.cpp
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/CallEvent.cpp?rev=163644&r1=163643&r2=163644&view=diff
>> ==============================================================================
>> --- cfe/trunk/lib/StaticAnalyzer/Core/CallEvent.cpp (original)
>> +++ cfe/trunk/lib/StaticAnalyzer/Core/CallEvent.cpp Tue Sep 11 13:47:13 2012
>> @@ -433,14 +433,21 @@
>>   if (!RD || !RD->hasDefinition())
>>     return RuntimeDefinition();
>> 
>> -  // Find the decl for this method in that class.
>> -  const CXXMethodDecl *Result = MD->getCorrespondingMethodInClass(RD, true);
>> +  const CXXMethodDecl *Result;
>> +  if (MD->getParent()->isDerivedFrom(RD)) {
>> +    // If our static type info is better than our dynamic type info, don't
>> +    // bother doing a search. Just use the static method.
>> +    Result = MD;
>> +  } else {
>> +    // Otherwise, find the decl for the method in the dynamic class.
>> +    Result = MD->getCorrespondingMethodInClass(RD, true);
>> +  }
>> +
>>   if (!Result) {
>>     // We might not even get the original statically-resolved method due to
>>     // some particularly nasty casting (e.g. casts to sister classes).
>>     // However, we should at least be able to search up and down our own class
>>     // hierarchy, and some real bugs have been caught by checking this.
>> -    assert(!MD->getParent()->isDerivedFrom(RD) && "Bad DynamicTypeInfo");
>>     assert(!RD->isDerivedFrom(MD->getParent()) && "Couldn't find known method");
>>     return RuntimeDefinition();
>>   }
>> 
>> Modified: cfe/trunk/test/Analysis/inlining/dyn-dispatch-bifurcate.cpp
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/inlining/dyn-dispatch-bifurcate.cpp?rev=163644&r1=163643&r2=163644&view=diff
>> ==============================================================================
>> --- cfe/trunk/test/Analysis/inlining/dyn-dispatch-bifurcate.cpp (original)
>> +++ cfe/trunk/test/Analysis/inlining/dyn-dispatch-bifurcate.cpp Tue Sep 11 13:47:13 2012
>> @@ -15,3 +15,19 @@
>>   A a;
>>   clang_analyzer_eval(a.get() == 0); // expected-warning{{TRUE}}
>> }
>> +
>> +
>> +namespace ReinterpretDisruptsDynamicTypeInfo {
>> +  class Parent {};
>> +
>> +  class Child : public Parent {
>> +  public:
>> +    virtual int foo() { return 42; }
>> +  };
>> +
>> +  void test(Parent *a) {
>> +    Child *b = reinterpret_cast<Child *>(a);
>> +    if (!b) return;
>> +    clang_analyzer_eval(b->foo() == 42); // expected-warning{{TRUE}} expected-warning{{UNKNOWN}}
>> +  }
>> +}
>> 
>> 
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120912/c127fb8f/attachment.html>