[CTU] Add support for virtual functions

[martong]

clang/test/Analysis/ctu-main.cpp

112 ↗

(On Diff #207012)

In case you wonder why we don't use expected-warning{{UNKNOWN}} then please refer to this non-ctu bug report about virtual functions:
https://bugs.llvm.org/show_bug.cgi?id=42423

[baloghadamsoftware]

Try to set analyzer option IPAMode to something different from its default value which is dynamic-bifurcate in the test file.

[martong]

• Use -analyzer-config ipa=inlining in the test

[martong]

In D63920#1566035, @baloghadamsoftware wrote:

Try to set analyzer option IPAMode to something different from its default value which is dynamic-bifurcate in the test file.

Ok I set it to ipa=inlining and then we receive the expected "UNKNOWN" warning.

[Szelethus]

In D63920#1568031, @martong wrote:

In D63920#1566035, @baloghadamsoftware wrote:

Try to set analyzer option IPAMode to something different from its default value which is dynamic-bifurcate in the test file.

Ok I set it to ipa=inlining and then we receive the expected "UNKNOWN" warning.

Alright, what are testing here exactly? I think its fine to see how the new virtual functions are inlined in different inlining mode, but we should definitely have a RUN: line/test file with the default mode.

[martong]

In D63920#1568049, @Szelethus wrote:

In D63920#1568031, @martong wrote:

In D63920#1566035, @baloghadamsoftware wrote:

Try to set analyzer option IPAMode to something different from its default value which is dynamic-bifurcate in the test file.

Ok I set it to ipa=inlining and then we receive the expected "UNKNOWN" warning.

Alright, what are testing here exactly? I think its fine to see how the new virtual functions are inlined in different inlining mode, but we should definitely have a RUN: line/test file with the default mode.

So, we would like to have a test which indicates that we can inline a virtual function from another TU. These tests are at line 110 and 111.

At line 113 we would like to have a test which verifies that if the dynamic type is unknown there is not much we can gain with inlining.
So, either we use the default ipa mode and then we will have

clang_analyzer_eval(obj->fvcl(1));           // expected-warning{{TRUE}} expected-warning{{UNKOWN}}

Or we use` ipa=inlining` and then we have

clang_analyzer_eval(obj->fvcl(1));           // expected-warning{{UNKOWN}}

The latter seems to be more meaningful for me because it is hard to explain why we expect {{TRUE}} in the first case.
Still I vote for a third alternative when we use the default ipa mode:

clang_analyzer_eval(obj->fvcl(1) == 8);           // expected-warning{{TRUE}} expected-warning{{FALSE}}

This seems to be clear for me , it is obvious from the warnings that the dynamic type is unknown.

Or perhaps there is no need to have a check for the case when the dynamic type is unknown?

[Szelethus]

In D63920#1568269, @martong wrote:

So, we would like to have a test which indicates that we can inline a virtual function from another TU. These tests are at line 110 and 111.

At line 113 we would like to have a test which verifies that if the dynamic type is unknown there is not much we can gain with inlining.
So, either we use the default ipa mode and then we will have

clang_analyzer_eval(obj->fvcl(1));           // expected-warning{{TRUE}} expected-warning{{UNKOWN}}

Or we use` ipa=inlining` and then we have

clang_analyzer_eval(obj->fvcl(1));           // expected-warning{{UNKOWN}}

The latter seems to be more meaningful for me because it is hard to explain why we expect {{TRUE}} in the first case.
Still I vote for a third alternative when we use the default ipa mode:

clang_analyzer_eval(obj->fvcl(1) == 8);           // expected-warning{{TRUE}} expected-warning{{FALSE}}

This seems to be clear for me , it is obvious from the warnings that the dynamic type is unknown.

Or perhaps there is no need to have a check for the case when the dynamic type is unknown?

Okay, so changing the inlining mode for any other reason then a test case sounds like a super risky idea. I don't think we're going to change it for our use cases anytime soon. Could you please add a RUN: line where with default inlining?

[martong]

• Remove ipa mode from the test

[Szelethus]

LGTM

[martong]

Thanks for the review!

cfe/trunk/lib/StaticAnalyzer/Core/CallEvent.cpp

if (!Result) {
     // this is fixed. <rdar://problem/12287087>
     //assert(!MD->getParent()->isDerivedFrom(RD) && "Bad DynamicTypeInfo");
 
     return {};
   }
 
   // Does the decl that we found have an implementation?
   const FunctionDecl *Definition;
-  if (!Result->hasBody(Definition))
+  if (!Result->hasBody(Definition)) {
+    if (!DynType.canBeASubClass())
+      return AnyFunctionCall::getRuntimeDefinition();
     return {};
+  }
 
   // We found a definition. If we're not sure that this devirtualization is
   // actually what will happen at runtime, make sure to provide the region so
   // that ExprEngine can decide what to do with it.
   if (DynType.canBeASubClass())
     return RuntimeDefinition(Definition, R->StripCasts());
   return RuntimeDefinition(Definition, /*DispatchRegion=*/nullptr);
 }

cfe/trunk/test/Analysis/Inputs/ctu-other.cpp

 int embed_cls::fecl(int x) {
   return x - 7;
 }
 } // namespace myns
 
 class mycls {
 public:
   int fcl(int x);
+  virtual int fvcl(int x);
   static int fscl(int x);
 
   class embed_cls2 {
   public:
     int fecl2(int x);
   };
 };
 
 int mycls::fcl(int x) {
   return x + 5;
 }
+int mycls::fvcl(int x) {
+  return x + 7;
+}
 int mycls::fscl(int x) {
   return x + 6;
 }
 int mycls::embed_cls2::fecl2(int x) {
   return x - 11;
 }
 
+class derived : public mycls {
+public:
+  virtual int fvcl(int x) override;
+};
+
+int derived::fvcl(int x) {
+  return x + 8;
+}
+
 namespace chns {
 int chf2(int x);
 
 class chcls {
 public:
   int chf4(int x);
 };
 

cfe/trunk/test/Analysis/Inputs/ctu-other.cpp.externalDefMap.txt

 c:@N@chns@F@chf1#I# ctu-other.cpp.ast
 c:@N@myns@N@embed_ns@F@fens#I# ctu-other.cpp.ast
 c:@F@g#I# ctu-other.cpp.ast
 c:@S@mycls@F@fscl#I#S ctu-other.cpp.ast
 c:@S@mycls@F@fcl#I# ctu-other.cpp.ast
+c:@S@mycls@F@fvcl#I# ctu-other.cpp.ast
 c:@N@myns@S@embed_cls@F@fecl#I# ctu-other.cpp.ast
 c:@S@mycls@S@embed_cls2@F@fecl2#I# ctu-other.cpp.ast
+c:@S@derived@F@fvcl#I# ctu-other.cpp.ast
 c:@F@f#I# ctu-other.cpp.ast
 c:@N@myns@F@fns#I# ctu-other.cpp.ast
 c:@F@h#I# ctu-other.cpp.ast
 c:@F@h_chain#I# ctu-chain.cpp.ast
 c:@N@chns@S@chcls@F@chf4#I# ctu-chain.cpp.ast
 c:@N@chns@F@chf2#I# ctu-chain.cpp.ast
 c:@F@fun_using_anon_struct#I# ctu-other.cpp.ast
 c:@F@other_macro_diag#I# ctu-other.cpp.ast

cfe/trunk/test/Analysis/ctu-main.cpp

 public:
   int fecl(int x);
 };
 }
 
 class mycls {
 public:
   int fcl(int x);
+  virtual int fvcl(int x);
   static int fscl(int x);
 
   class embed_cls2 {
   public:
     int fecl2(int x);
   };
 };
 
+class derived : public mycls {
+public:
+  virtual int fvcl(int x) override;
+};
+
 namespace chns {
 int chf1(int x);
 }
 
 int fun_using_anon_struct(int);
 int other_macro_diag(int);
 
 extern const int extInt;

 };
 extern SCC extSCC;
 union U {
   const int a;
   const unsigned int b;
 };
 extern U extU;
 
+void test_virtual_functions(mycls* obj) {
+  // The dynamic type is known.
+  clang_analyzer_eval(mycls().fvcl(1) == 8);   // expected-warning{{TRUE}}
+  clang_analyzer_eval(derived().fvcl(1) == 9); // expected-warning{{TRUE}}
+  // We cannot decide about the dynamic type.
+  clang_analyzer_eval(obj->fvcl(1) == 8);      // expected-warning{{FALSE}} expected-warning{{TRUE}}
+}
+
 int main() {
   clang_analyzer_eval(f(3) == 2); // expected-warning{{TRUE}}
   clang_analyzer_eval(f(4) == 3); // expected-warning{{TRUE}}
   clang_analyzer_eval(f(5) == 3); // expected-warning{{FALSE}}
   clang_analyzer_eval(g(4) == 6); // expected-warning{{TRUE}}
   clang_analyzer_eval(h(2) == 8); // expected-warning{{TRUE}}
 
   clang_analyzer_eval(myns::fns(2) == 9);                   // expected-warning{{TRUE}}
   clang_analyzer_eval(myns::embed_ns::fens(2) == -1);       // expected-warning{{TRUE}}
   clang_analyzer_eval(mycls().fcl(1) == 6);                 // expected-warning{{TRUE}}
   clang_analyzer_eval(mycls::fscl(1) == 7);                 // expected-warning{{TRUE}}
   clang_analyzer_eval(myns::embed_cls().fecl(1) == -6);     // expected-warning{{TRUE}}
   clang_analyzer_eval(mycls::embed_cls2().fecl2(0) == -11); // expected-warning{{TRUE}}
 
   clang_analyzer_eval(chns::chf1(4) == 12); // expected-warning{{TRUE}}
   clang_analyzer_eval(fun_using_anon_struct(8) == 8); // expected-warning{{TRUE}}
 
   clang_analyzer_eval(other_macro_diag(1) == 1); // expected-warning{{TRUE}}
-  // expected-warning@Inputs/ctu-other.cpp:80{{REACHABLE}}
+  // expected-warning@Inputs/ctu-other.cpp:93{{REACHABLE}}
   MACRODIAG(); // expected-warning{{REACHABLE}}
 
   clang_analyzer_eval(extInt == 2); // expected-warning{{TRUE}}
   clang_analyzer_eval(intns::extInt == 3); // expected-warning{{TRUE}}
   clang_analyzer_eval(extS.a == 4); // expected-warning{{TRUE}}
   clang_analyzer_eval(extHere == 6); // expected-warning{{TRUE}}
   clang_analyzer_eval(A::a == 3); // expected-warning{{TRUE}}
   clang_analyzer_eval(extSC.a == 8); // expected-warning{{TRUE}}
   clang_analyzer_eval(ST::sc.a == 2); // expected-warning{{TRUE}}
   // clang_analyzer_eval(extSCN.scn.a == 9); // TODO
   clang_analyzer_eval(extSubSCN.a == 1); // expected-warning{{TRUE}}
   // clang_analyzer_eval(extSCC.a == 7); // TODO
   clang_analyzer_eval(extU.a == 4); // expected-warning{{TRUE}}
 }