This is an archive of the discontinued LLVM Phabricator instance.

[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

Event Timeline

wangxindsb created this revision.Jun 16 2017, 6:31 AM
xazax.hun edited edge metadata.Jun 20 2017, 1:38 AM

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.

Add test case for the patch

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.

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.

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 ↗(On Diff #103180)

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

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.

Add test case for the virtual call checker.

Add run line in the test case.

wangxindsb marked 5 inline comments as done.Jun 20 2017, 9:01 AM
wangxindsb added inline comments.
VirtualCallChecker.cpp
260 ↗(On Diff #102812)

Sorry, can you explain what this means?

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

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

Please add the license back.

> 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

Add license to VirtualCallChecker.cpp

wangxindsb marked an inline comment as done.

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

xazax.hun added inline comments.Jun 29 2017, 12:44 AM
lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp
105

Could you find more descriptive names for these BugTypes?

150–152

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

151

Variables should start with capital letters.

xazax.hun added inline comments.Jun 29 2017, 12:44 AM
lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp
106

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.

156

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

175

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

199

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.

207–212

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

235

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.

236

Shouldn't this be const?

250

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

265

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.

277

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

285

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.

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
277

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
134

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

138

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

195

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

208

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

227

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.

255–256

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.

258

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

272

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
157

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

158

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

263

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

280

Do not use braces for single statement blocks.

281

Braces for single statement blocks.

284

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
236

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?

250

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

255–256

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

271

You could eliminate this local variable.

279

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
236

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

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

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

161

I wish for camelCase.

233

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.

264

"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
16

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

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.

233
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
111

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
233

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.

246–249

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
3–4

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–16

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

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.