Page MenuHomePhabricator

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

Authored by wangxindsb on Jun 16 2017, 6:31 AM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
xazax.hun added inline comments.Jun 29 2017, 12:44 AM
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.

36 ↗(On Diff #104201)

Variables should start with capital letters.

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 updated this revision to Diff 105440.Jul 6 2017, 8:43 AM
  • 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 added inline comments.Jul 6 2017, 8:46 AM
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 updated this revision to Diff 105737.Jul 8 2017, 4:27 AM
  • Change the two bugtype to one.
  • Use the generateErrorNode() for the pure virtual call.
  • Change the test case for the new expression.
wangxindsb updated this revision to Diff 105738.Jul 8 2017, 4:44 AM
  • Change the two bugtype to one.
  • Use the generateErrorNode() for the pure virtual call.
  • Change the test case for the new expression.
wangxindsb updated this revision to Diff 105760.Jul 9 2017, 6:05 AM
wangxindsb marked an inline comment as done.
  • 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 marked 11 inline comments as done.Jul 9 2017, 6:09 AM

Look forward to your review.

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.

  • 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.

wangxindsb marked 8 inline comments as done.Jul 12 2017, 7:31 AM

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 marked 6 inline comments as done.Jul 12 2017, 9:41 AM
  • 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 added inline comments.Jul 14 2017, 6:21 AM
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 marked 5 inline comments as done.Jul 14 2017, 10:05 AM
wangxindsb added inline comments.
lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp
72 ↗(On Diff #106243)

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

wangxindsb marked an inline comment as done.
  • 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().
  • Change the bugtype, just like the older checker.

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

xazax.hun added a subscriber: NoQ.Aug 18 2017, 11:37 PM

@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.

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

Add namespace closing comment.

wangxindsb marked an inline comment as done.Aug 19 2017, 1:02 AM
NoQ edited edge metadata.Aug 19 2017, 9:42 AM

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 marked 3 inline comments as done.Aug 19 2017, 8:16 PM
wangxindsb added inline comments.
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'".

  • Rename reportbug();
  • Change message "Pure function" to "pure virtual function";
  • Fixing: expected-warning;
wangxindsb added inline comments.Aug 19 2017, 11:36 PM
lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp
31 ↗(On Diff #111862)

Add the FIXME

NoQ added a comment.Aug 21 2017, 6:34 AM

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
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.

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.

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 added a comment.Aug 21 2017, 7:02 AM

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?

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

@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 added a comment.Aug 22 2017, 12:09 AM

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.

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

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

NoQ accepted this revision.Aug 24 2017, 7:47 AM

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).

This revision is now accepted and ready to land.Aug 24 2017, 7:47 AM
This revision was automatically updated to reflect the committed changes.