[PATCH] D49656: [analyzer] Add support for more pointer invalidating functions in InnerPointerChecker

[Artem Dergachev via Phabricator via cfe-commits]

NoQ accepted this revision.
NoQ added a comment.

Looks great!



================
Comment at: lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp:192
 
-  if (Call.isCalled(CStrFn) || Call.isCalled(DataFn)) {
-    SVal RawPtr = Call.getReturnValue();
-    if (SymbolRef Sym = RawPtr.getAsSymbol(/*IncludeBaseRegions=*/true)) {
-      // Start tracking this raw pointer by adding it to the set of symbols
-      // associated with this container object in the program state map.
-      PtrSet::Factory &F = State->getStateManager().get_context<PtrSet>();
-      const PtrSet *SetPtr = State->get<RawPtrMap>(ObjRegion);
-      PtrSet Set = SetPtr ? *SetPtr : F.getEmptySet();
-      assert(C.wasInlined || !Set.contains(Sym));
-      Set = F.add(Set, Sym);
-      State = State->set<RawPtrMap>(ObjRegion, Set);
-      C.addTransition(State);
+      SVal Arg = FC->getArgSVal(ArgI);
+      const auto *ArgRegion =
----------------
xazax.hun wrote:
> While I cannot recall examples in the STL the number of arguments and parameters might differ. We might have ellipsis in the parameters and this way we may pass more arguments. Since we do not have the parameter type for this case, I think it is ok to not handle it. But it might be worth to have a comment. The other case, when we have default arguments. I do not really know how the analyzer behaves with default arguments but maybe it would be worth to add a test case to ensure we do not crash on that?
The analyzer behaves pretty badly with default arguments; we don't really model them unless they are like plain integer literals. But that's not an issue here because a default parameter/argument is still both a parameter and an argument (where the argument expression takes the form of `CXXDefaultArgExpr`).


================
Comment at: lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp:207-208
+
+    for (unsigned I = 0, E = FD->getNumParams(); I != E; ++I) {
+      QualType ParamTy = FD->getParamDecl(I)->getType();
+      if (!ParamTy->isReferenceType() ||
----------------
rnkovacs wrote:
> NoQ wrote:
> > NoQ wrote:
> > > We need tests for operators here, due to the problem that i recently encountered in D49627: there's different numbering for arguments and parameters within in-class operators. Namely, this-argument is counted as an argument (shifting other argument indices by 1) but not as a parameter.
> > (i.e., tests and most likely some actual code branch)
> I did the coding part, but for the tests, I'm struggling to find an example of a member operator in the STL that takes a non-const string reference as an argument. Could you perhaps help me out here?
Mmm mmmmm right, dunno. I thought `operator>>` would be our main example, but it's apparently non-member for a string, even though it is defined as a member for primitive types.

I guess let's skip the test until we find an example. We did our best to work around potential future problems, that's good.


================
Comment at: test/Analysis/inner-pointer.cpp:41-42
 
+template< class T >
+void func_ref(T& a);
+
----------------
rnkovacs wrote:
> NoQ wrote:
> > Without a definition for this thing, we won't be able to test much. I suggest you to take a pointer to a function directly, i.e. define a `std::swap<T>(x, y)` somewhere and take a `&std::swap<std::string>` directly and call it (hope it actually works this way).
> I think I am missing something. This is meant to test that the checker recognizes standard library function calls taking a non-const reference to a string as an argument. At L314, `func_ref` is called with a string argument, and the checker seems to do all the things it should after the current patch, emitting the new kind of note and such.
> 
> I can surely add the `std::swap<T>(x, y)` definition too, but then the analyzer will see that the pointer was reallocated at the assignment inside the function, and place the note there. I think that would test the previous string-member-function patch more than this one.
Mm, aha, ok, i misunderstood this test. I thought you're trying to define something like `std::function` and try to see if calling a function pointer wrapped into that thing works. No idea why i was thinking that, sry.


https://reviews.llvm.org/D49656