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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

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.



================
Comment at: lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp:110
+  auto ThiSVal =
+      State->getSVal(SVB.getCXXThis(MD, LCtx->getCurrentStackFrame()));
+  const MemRegion *Reg = ThiSVal.getAsRegion();
----------------
wangxindsb wrote:
> NoQ wrote:
> > 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.
> ```
> 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'".
> 
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.


================
Comment at: lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp:124-127
+  if (CD)
+    InfoText = "Called from this constrctor '" + CD->getNameAsString() + "'";
+  else
+    InfoText = "Called from this destrctor '" + DD->getNameAsString() + "'";
----------------
Typo: "constr//**u**//ctor", "destr//**u**//ctor".

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.




================
Comment at: test/Analysis/virtualcall.cpp:1-6
+// 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:PureOnly=true -DPUREONLY=1 -verify -std=c++11 %s
----------------
Tests are still not working because your auto-format tool has inserted weird line breaks.


https://reviews.llvm.org/D34275