[analyzer] Re-implemente current virtual calls checker in a path-sensitive way

[wangxindsb]

Summary

This implement a path-sensitive checker that warns if virtual calls are made from constructors and destructors.

The checker use the GDM (generic data map) to store three integers in the program state for constructors, destructors and objects. Once enter one of these is entered, increase the corresponding integer, once leave, decrease the corresponding integer. In a PreCall callback, the checker first check if a virtual method is called and the GDM meets the conditions, if yes, a warning will be issued.

The checker also include a bug reporter visitor to add an extra note when the last constructor/destructor was called before the call to the virtual function.

[xazax.hun]

I do not see any test cases for this patch. Could you add them as well?

Are you sure that this representation is ok?
How do you handle the following case?

struct A {
  A() {
    X x;
    x.virtualMethod(); // this virtual call is ok
  }
}
int main() {
  A a;
}

VirtualCallChecker.cpp

44 ↗

(On Diff #102812)

The name of this flag is not descriptive enough. Please choose a name that refers to what are you using this value for.

79 ↗

(On Diff #102812)

Variable names should start with uppercase letter.

83 ↗

(On Diff #102812)

Are you sure that the LCtx->getDecl can not return null?

114 ↗

(On Diff #102812)

Do you need this cast here?

119 ↗

(On Diff #102812)

Querying the state is not free, I think you should also query something from the state once you are sure that you will need that.

158 ↗

(On Diff #102812)

I don't think this is the right approach. Once you increased the ObjectFlag on a path, you will never report anything on that path anymore. I think it might be better to have a map from Symbols (representing this) to ctor/dtors or some other approach.

260 ↗

(On Diff #102812)

The formatting seems to be off here I recommend using clang format.

[wangxindsb]

Add test case for the patch

[wangxindsb]

I do not see any test cases for this patch. Could you add them as well?

I add the test case just now.

How do you handle the following case?

struct A {
  A() {
    X x;
    x.virtualMethod(); // this virtual call is ok
  }
}
int main() {
  A a;
}

I use the checker to check the code above, the checker works as expect and doesn't throw the warning.

[xazax.hun]

Note that when you update the differential revision you need to upload the whole diff. Your diff now only contains the tests but not the code.

In D34275#785189, @wangxindsb wrote:

How do you handle the following case?

struct A {
  A() {
    X x;
    x.virtualMethod(); // this virtual call is ok
  }
}
int main() {
  A a;
}

I use the checker to check the code above, the checker works as expect and doesn't throw the warning.

What about:

struct A {
  A() {
    X x;
    x.virtualMethod(); // this virtual call is ok
    foo(); // should warn here
  }
  virtual foo();
}
int main() {
  A a;
}

Does the checker warn on the second call?

virtualcall.cpp

1

Please add the appropriate run lines so you can run the tests using make check.

[wangxindsb]

What about:

struct A {
  A() {
    X x;
    x.virtualMethod(); // this virtual call is ok
    foo(); // should warn here
  }
  virtual foo();
}
int main() {
  A a;
}

Does the checker warn on the second call?

Yes, the checker warn on the second call.

[wangxindsb]

Add test case for the virtual call checker.

[wangxindsb]

Add run line in the test case.

[wangxindsb]

VirtualCallChecker.cpp

260 ↗

(On Diff #102812)

Sorry, can you explain what this means?

[wangxindsb]

Correct some error in VirtualCallChecker.cpp.
Complete the test case.

[xazax.hun]

In D34275#785294, @wangxindsb wrote:

What about:

struct A {
  A() {
    X x;
    x.virtualMethod(); // this virtual call is ok
    foo(); // should warn here
  }
  virtual foo();
}
int main() {
  A a;
}

Does the checker warn on the second call?

Yes, the checker warn on the second call.

Oh, I think I see how it works now. How about:

struct A;
struct X {
   void callFooOfA(A*);
};
struct A {
  A() {
    X x;
    x.virtualMethod(); // this virtual call is ok
    x.callFooOfA(this)
  }
  virtual foo();
};
void X::callFooOfA(A* a) {
   a->foo(); // Would be good to warn here. 
}
int main() {
  A a;
}

lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp

1 ↗

(On Diff #103214)

Please add the license back.

[wangxindsb]

> Oh, I think I see how it works now. How about:

struct A;
struct X {
   void callFooOfA(A*);
};
struct A {
  A() {
    X x;
    x.virtualMethod(); // this virtual call is ok
    x.callFooOfA(this)
  }
  virtual foo();
};
void X::callFooOfA(A* a) {
   a->foo(); // Would be good to warn here. 
}
int main() {
  A a;
}

Yes, you are right, the checker doesn't warn on the a->foo();. I will fix this error soon.

Thanks,
Xin

[wangxindsb]

Add license to VirtualCallChecker.cpp

[wangxindsb]

-Fix the bug of the virtual call during a function call of the object.
-Add flag for the PUREONLY.

[xazax.hun]

lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp

31 ↗

(On Diff #104201)

Could you find more descriptive names for these BugTypes?

35 ↗

(On Diff #104201)

Comments should be full sentences, this means they should be terminated with a period.

36 ↗

(On Diff #104201)

Variables should start with capital letters.

[xazax.hun]

lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp

32 ↗

(On Diff #104201)

I'd rather have one bug type, describing a call to a virtual function from ctor/dtor. The actual error message can clarify whether this is a call from ctor or from dtor and whether it is pure virtual. You do not need to reflect every detail in the bug type.

40 ↗

(On Diff #104201)

The names of the arguments should start with an uppercase letter.

72 ↗

(On Diff #104201)

Do you need these traits above after having the maps bellow?

96 ↗

(On Diff #104201)

The formatting here seems to be off, could you run clang-format on the code? This is a tool that can format the code to comply with the LLVM formatting style guide.

129 ↗

(On Diff #104201)

I think you do not need the or_null suffix here since the argument of this cast will never be null.

141 ↗

(On Diff #104201)

If you do not use the result of dyn_cast, you could use isa instead. Even better, you could reuse CC or DC in this check.

168 ↗

(On Diff #104201)

Shouldn't this be const?

192 ↗

(On Diff #104201)

In LLVM we do not write the braces for conditionals that guarding a single statement.

218 ↗

(On Diff #104201)

In case of calling a pure virtual function maybe it would be better to stop the analysis, i.e.: creating a fatal error node.
Also, these functions might fail to give you back an ExplodedNode, so I think you should have a check here for that case.

314 ↗

(On Diff #104201)

Can getParent ever return null? Maybe you do not need the or_null suffix in the cast.
Moreover are you sure that this method works? In case the function was a method, the parent should be the CXXRecordDecl, not a CXXMethodDecl.

332 ↗

(On Diff #104201)

Maybe instead of callExpr, you should get CXXInstanceCall, which has a getCXXThisVal method.

[wangxindsb]

• Rename the BugType.
• Correct the code style to be clang format.
• Rename the arguments to start with uppercase letter.
• Add the support to check for the constructor called by the New expr.

You don't need to put effort to review the code, because I will make some changes to the bug report system and the GDM of the checker which has not been done now.

[wangxindsb]

lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp

332 ↗

(On Diff #104201)

I tried to use the CXXInstanceCall, but I can't use it to get the expr which call the function.

[wangxindsb]

• Change the two bugtype to one.
• Use the generateErrorNode() for the pure virtual call.
• Change the test case for the new expression.

[wangxindsb]

• Change the two bugtype to one.
• Use the generateErrorNode() for the pure virtual call.
• Change the test case for the new expression.

[wangxindsb]

• Use the map of object to ctor/dtor to check the virtual call.
• Use CXXInstanceCall and getCXXThisVal method to get the 'this' instead of getThisSVal().
• Correct some format errors.

[wangxindsb]

Look forward to your review.

[xazax.hun]

You are making a pretty good progress!
I think right now there is some code duplication that could be reduced, but otherwise, the checker starts to look really good.

lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp

43 ↗

(On Diff #105760)

This could be maybe a free static function instead of a method?

47 ↗

(On Diff #105760)

Maybe instead of Region the name could be more specific? I.e. the ThisRegion? Or ObjectRegion?

101 ↗

(On Diff #105760)

In the warning messages we usually enclose the source code snippets (e.g.: declaration names) in single quotes.

135 ↗

(On Diff #105760)

getting the SValBuilder is cheap, maybe you could hoist it from the if statements.

146 ↗

(On Diff #105760)

The code of this function is almost completely identical with the checkBegin. I was thinking about abstracting this into a separate function with a bool argument to determine whether we would like to add something to the GDM or remove it.

186 ↗

(On Diff #105760)

Maybe you could cast this to CXXMemberCall instead. If that cast succeed, than you do not need to check the Call.getDecl() and also do not need to check for ctor/dtor calls.

199 ↗

(On Diff #105760)

Maybe it would be cleaner to early return if the call is not virtual and remove it from these two if statements.

213 ↗

(On Diff #105760)

Maybe you could extract the error reporting into a separate function with two arguments: the error message, and whether it should generate an error node.

[wangxindsb]

• Change IsVirtualCall(const CallExpr *CE) to be a free static function.
• Rename some variables.
• Improve the BugReporterVisitor, enclose the declaration names in single quotes.
• Hoist getSValBuilder() from the if statements.
• Fix some code duplications.
• Use the CXXMemberCall instead CXXInstanceCall in the CheckPreCall.
• Remove IsVirtualCall(CE) from if statements.
• Fix the error of the visitnode() method which may throw a wrong call graph for the code blow.

class Y {
public:
  virtual void foobar();
  Y() {
    F f1;
    foobar();
  }
};

Previous visitnode() will issue the virtual function called from the F(); Current visitnode() fix this bug.

[xazax.hun]

Thank you! I think we can start to run this check on real world code bases and evaluate the results.

lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp

41 ↗

(On Diff #106204)

Function names should start with a lower case letter. These two could be private. I'd prefer another name for this function like registerCtorDtorCallInState.

42 ↗

(On Diff #106204)

Use StringRef instead of const char *. It is more idiomatic in LLVM code.

179 ↗

(On Diff #106204)

I'd omit the Msg local variable. After that, we do not need to use braces for single statement blocks.

210 ↗

(On Diff #106204)

Braces for single statement blocks.

224 ↗

(On Diff #106204)

Braces for single statement blocks.

238 ↗

(On Diff #106204)

Do not use braces for single statement blocks.

[wangxindsb]

• Change function name to start with a lower case letter.
• Use StringRef instead of `const char * ' for ReportBug() const.
• Correct the braces for single statement blocks.

[xazax.hun]

lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp

72 ↗

(On Diff #106243)

I was wondering if there is another way to represent the state.
We could have a two element (bool based) enum class like:

enum class ObjectState : bool {
  CtorCalled,
  DtorCalled
};

Se we could have only one map in the GDM. Either the destructor is called for an object or the constructor. Or in case none of them is called on a path, the state is empty. What do you think?

86 ↗

(On Diff #106243)

You could move the declarations of PSM and SVB after the next early return.

107 ↗

(On Diff #106243)

You could eliminate this local variable.

169 ↗

(On Diff #106243)

These two checks could be moved before you query the CXXThisVal.

247 ↗

(On Diff #106243)

This return is redundant.

[wangxindsb]

lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp

72 ↗

(On Diff #106243)

Yes, it's better than the previous two maps.

[wangxindsb]

• Change two maps to one map.
• Move the declarations of PSM and SVB after the next early return.
• Delete the variable std::string DeclName.
• Delete last return in reportbug().

[wangxindsb]

• Change the bugtype, just like the older checker.

[wangxindsb]

Fix the Assertion Failed when run the checker to check the building of LibreOffice.

[xazax.hun]

@NoQ , @dcoughlin could either of you review this patch as well? The end of GSoC is closing and it would be awesome to be able to commit this before it ends. :)

lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp

30 ↗

(On Diff #106654)

Missing namespace closing comment.

[wangxindsb]

+enum class ObjectState : bool { CtorCalled, DtorCalled };
+} // end namespace

Add namespace closing comment.

[NoQ]

First of all, thanks everybody for working on this. I'd see what i can do.

lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp

29 ↗

(On Diff #111806)

Please remind me what was wrong with ascending over StackFrameContexts to see if we're inside a constructor or destructor call? Why did we need a state trait here? Was it too hard to obtain this-values? (Not sure if there's time to fix this, but it might be worth it to leave a FIXME around).

56 ↗

(On Diff #111806)

I wish for camelCase.

110 ↗

(On Diff #111806)

I don't think this is working. CXXThisRegion is never a region of a C++ object; it's the segment of the stack where the pointer is passed, you need to dereference its value to get the actual object region.

Probably tests for the visitor might make things more clear.

194 ↗

(On Diff #111806)

"Pure function" doesn't mean what we want to say here (https://en.wikipedia.org/wiki/Pure_function), i think we should stick with "pure virtual function" anyways.

test/Analysis/virtualcall.cpp

15 ↗

(On Diff #111806)

Tests need fixing: expected-warning requires {{ and needs to be on the correct line.

[wangxindsb]

lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp

29 ↗

(On Diff #111806)

I have implement this method before, the code is on this link, virtualcallchecker using stackframe. I thought it was harder and slower than the state trait, so I gave up this method. Also, I thought this method maybe hard to add visitor. I will work on this soon.

110 ↗

(On Diff #111806)

class Y {
public:
  virtual void foobar();
  void fooY() {  
    F f1;
    foobar();
  }
  Y() {
    fooY();
  }
};

I thought this test is for this situation. If the visitor is correct, it will issued "Called from this constructor 'Y'", else it will issued "Called from this constructor 'F'".

[wangxindsb]

• Rename reportbug();
• Change message "Pure function" to "pure virtual function";
• Fixing: expected-warning;

[wangxindsb]

lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp

31 ↗

(On Diff #111862)

Add the FIXME

[NoQ]

Tests are still not working - they pass now, but they don't actually test anything. Please make sure that tests actually work. Which means that

1. Tests pass when you run make -j4 check-clang-analysis;
2. Tests start failing when you change your code or expected-warning directives.

lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp

124–127 ↗

(On Diff #111862)

Typo: "constructor", "destructor".

Also i guess we need to think about this warning message more carefully anyway. Like, we already have an "entering function..." event diagnostic piece. Because the warning always happens inside the function in question, this event would never be pruned, so it'd always be there. So what do we expect the visitor's diagnostic piece to say?

I suggest "This [constructor|destructor] of an object of type 'Foo' has not returned when the virtual method was called". With a probable future improvement of specifying the name of the object (when eg. it's a variable).

It might even be that later we'd decide that the visitor is not necessary for this checker. I'm not sure, i guess i'd have to see how it works in practice.

Also, right now the visitor's piece is above the "entering function..." event. Not sure if having it below, or even inside the constructor, would be better.

110 ↗

(On Diff #111806)

Right, i didn't notice getSVal(); seems correct.

This test doesn't verify anything though, because it has no expected-warnings. Even if it had, it wouldn't verify where the visitor diagnostic appears, because without a more specific -analyzer-output option (eg. =text), the analyzer wouldn't emit visitor notes, so the -verify option would not be able to verify them.

So by visitor tests i mean adding -analyzer-output=text to the run-line and then adding expected-note directives wherever notes are expected.

test/Analysis/virtualcall.cpp

1–6 ↗

(On Diff #111862)

Tests are still not working because your auto-format tool has inserted weird line breaks.

[NoQ]

Most importantly, do you think we can enable the checker by default now? Was this checker evaluated on any large codebase, and if so, how many true/false positives did it find, probably compared to the old checker?

[wangxindsb]

• Fix the errors of the tests;
• Add -analyzer-output=text to the run-line;
• Change the message of the visitor's diagnostic piece.

[wangxindsb]

@NoQ

In D34275#847434, @NoQ wrote:

Most importantly, do you think we can enable the checker by default now? Was this checker evaluated on any large codebase, and if so, how many true/false positives did it find, probably compared to the old checker?

I am now runing the checker to check the build of firefox, llvm and libreoffice. There are no alarms on the building of firefox and llvm. When use scan-build to check the building of libreoffice, the scan-build failed (this may because the scan-build. Enable or not enable the virtualcallchecker, the build failed all the same, but when I just build the libreoffice and not run the scan-build, the build worked well) and there are 105 alarms about the virtual function in ctors/dtors, I am evaluating these alarms now.

[NoQ]

Thanks, works now! Apart from the minor comment on the hanging header file in the tests, this looks good and i have no further nits :)

*summons @dcoughlin to have a look at English in the warning messages*

test/Analysis/virtualcall.cpp

15–21 ↗

(On Diff #112110)

I think it might be worth it to highlight that the function is pure virtual even in non-pure-only mode (if you have time for that).

137–141 ↗

(On Diff #112110)

There used to be a test case in this header; we should restore the test or remove the file if it's no longer relevant.

[wangxindsb]

• Highlight pure virtual even in non-pure-only mode;
• Add change to the header.

[wangxindsb]

There are 105 alarms running the checker on the LibreOffice, 92 True positive, 13 not sure.

[NoQ]

All right then, i approve!

There are 105 alarms running the checker on the LibreOffice, 92 True positive, 13 not sure.

That's impressively loud. I guess you can try reporting some of the bugs you've found and seeing their reactions, if you enjoy this sort of stuff (it's always the ultimate test for bug-finding tools, and it'd also hopefully make libreoffice better).

virtualcall.cpp

-// RUN: %clang_analyze_cc1 -analyzer-checker=optin.cplusplus.VirtualCall -analyzer-store region -verify -std=c++11 %s
-// RUN: %clang_analyze_cc1 -analyzer-checker=optin.cplusplus.VirtualCall -analyzer-store region -analyzer-config optin.cplusplus.VirtualCall:Interprocedural=true -DINTERPROCEDURAL=1 -verify -std=c++11 %s
-// RUN: %clang_analyze_cc1 -analyzer-checker=optin.cplusplus.VirtualCall -analyzer-store region -analyzer-config optin.cplusplus.VirtualCall:PureOnly=true -DPUREONLY=1 -verify -std=c++11 %s
-
-/* When INTERPROCEDURAL is set, we expect diagnostics in all functions reachable
-   from a constructor or destructor. If it is not set, we expect diagnostics
-   only in the constructor or destructor.
-
-   When PUREONLY is set, we expect diagnostics only for calls to pure virtual
-   functions not to non-pure virtual functions.
-*/
-
 class A {
 public:
   A();
   A(int i);
 
   ~A() {};
   
-  virtual int foo() = 0; // from Sema: expected-note {{'foo' declared here}}
-  virtual void bar() = 0;
+  virtual int foo()=0; // from Sema: expected-note {{'foo' declared here}}
+  virtual void bar()=0;
   void f() {
     foo();
-#if INTERPROCEDURAL
-        // expected-warning-re@-2 {{{{^}}Call Path : foo <-- fCall to pure virtual function during construction has undefined behavior}}
-#endif
+        // expected-warning:Call to virtual function during construction
   }
 };
 
 class B : public A {
 public:
   B() {
     foo();
-#if !PUREONLY
-#if INTERPROCEDURAL
-        // expected-warning-re@-3 {{{{^}}Call Path : fooCall to virtual function during construction will not dispatch to derived class}}
-#else
-        // expected-warning-re@-5 {{{{^}}Call to virtual function during construction will not dispatch to derived class}}
-#endif
-#endif
-
+        // expected-warning:Call to virtual function during construction
   }
   ~B();
   
   virtual int foo();
   virtual void bar() { foo(); }
-#if INTERPROCEDURAL
-      // expected-warning-re@-2 {{{{^}}Call Path : foo <-- barCall to virtual function during destruction will not dispatch to derived class}}
-#endif
+      // expected-warning:Call to virtual function during destruction
 };
 
 A::A() {
   f();
 }
 
 A::A(int i) {
   foo(); // From Sema: expected-warning {{call to pure virtual member function 'foo' has undefined behavior}}
-#if INTERPROCEDURAL
-      // expected-warning-re@-2 {{{{^}}Call Path : fooCall to pure virtual function during construction has undefined behavior}}
-#else
-      // expected-warning-re@-4 {{{{^}}Call to pure virtual function during construction has undefined behavior}}
-#endif
+      // expected-warning:Call to virtual function during construction
 }
 
 B::~B() {
   this->B::foo(); // no-warning
   this->B::bar();
   this->foo();
-#if !PUREONLY
-#if INTERPROCEDURAL
-      // expected-warning-re@-3 {{{{^}}Call Path : fooCall to virtual function during destruction will not dispatch to derived class}}
-#else
-      // expected-warning-re@-5 {{{{^}}Call to virtual function during destruction will not dispatch to derived class}}
-#endif
-#endif
-
+      // expected-warning:Call to virtual function during destruction
 }
 
 class C : public B {
 public:
   C();
   ~C();
   
   virtual int foo();
   void f(int i);
 };
 
 C::C() {
   f(foo());
-#if !PUREONLY
-#if INTERPROCEDURAL
-      // expected-warning-re@-3 {{{{^}}Call Path : fooCall to virtual function during construction will not dispatch to derived class}}
-#else
-      // expected-warning-re@-5 {{{{^}}Call to virtual function during construction will not dispatch to derived class}}
-#endif
-#endif
+      // expected-warning:Call to virtual function during construction
 }
 
 class D : public B {
 public:
   D() {
     foo(); // no-warning
   }
   ~D() { bar(); }
   int foo() final;
   void bar() final { foo(); } // no-warning
 };
 
 class E final : public B {
 public:
   E() {
     foo(); // no-warning
   }
   ~E() { bar(); }
   int foo() override;
 };
 
-// Regression test: don't crash when there's no direct callee.
 class F {
 public:
   F() {
     void (F::* ptr)() = &F::foo;
     (this->*ptr)();
   }
   void foo();
 };
 
-int main() {
-  A *a;
-  B *b;
-  C *c;
-  D *d;
-  E *e;
-  F *f;
+class G {
+public:
+  virtual void bar();
+  void foo() {
+    bar();
+      // no warning
+  }
+};
+
+class H{
+public:
+  H() : initState(0) { init(); }
+  int initState;
+  virtual void f() const;
+  void init() {
+    if (initState)
+      f();
+      // no warning
   }
 
-#include "virtualcall.h"
-
-#define AS_SYSTEM
-#include "virtualcall.h"
-#undef AS_SYSTEM
+  H(int i) {
+    G g;
+    g.foo();
+    g.bar();
+      // no warning
+    f();
+      // expected-warning:Call to virtual function during construction
+    H& h = *this;
+    h.f();
+      // expected-warning:Call to virtual function during construction
+  }
+};
+
+class X {
+public:
+  X() {
+    g();
+      // expected-warning:Call to virtual function during construction
+  }
+  X(int i) {
+    if (i > 0) {
+      X x(i-1);
+      x.g();
+      // no warning
+    }
+    g();
+      // expected-warning:Call to virtual function during construction
+  }
+  virtual void g();
+};
+
+int main() {
+  
+  B b;
+  C c;
+  D d;
+  E e;
+  F f;
+  G g;
+  H h;
+  X x;
+}