[Analyzer] Don’t invalidate CXXThis when conservatively evaluating const methods (PR 21606)

[seaneveson]

Summary

Dear All,

I would like to propose a patch that prevents the invalidation of ‘this’ when a method is const; fixing the test case given below, taken from PR 21606 (https://llvm.org/bugs/show_bug.cgi?id=21606).

struct s1 {
    void g(const int *i) const;
};

struct s2 {
    void f(int *i) {
        m_i = i;
        m_s.g(m_i); // Diagnostic goes away if you remove this line.
        if (m_i)
            *i = 42;
    }
    int *m_i;
    s1 m_s;
};

int main() {
    s2().f(0);
}

Mutable members of the object are a special case and are still invalidated; if a mutable member is invalidated the entire object will be invalidated because invalidateRegions invalidates the base region.

Whilst the patch fixes the test case from PR 21606, the same false-positive occurs when the method ‘s1::g’ isn’t const; i.e. when ‘s2::f’ is called, subsequently calling ‘s1::g’, the memory region for the instance of s1 is (correctly) invalidated. However, the containing memory region (the instance of s2) is also invalidated, which I think is overly conservative.

Why is the base region (in this case: S2) invalidated? Would it be acceptable to change invalidation to modify the given region and not the base region when

1. invalidating only the mutable members for a const method call?
2. invalidating an object as a result of conservative ‘method call’ evaluations?

Regards,

Sean Eveson
SN Systems - Sony Computer Entertainment Group

[xazax.hun]

Hi!

Thanks for this patch! I think this would be a great addition! I have some comments inline.

lib/StaticAnalyzer/Core/CallEvent.cpp

409

Do we need to get the cannonical decl here? When one declaration is const, all of them supposed to be const.

412

Is it possible to fail to get ThisRegion? Should this be an assert?

415

What about the mutable fields of base classes? Are they covered here?

[xazax.hun]

One more note. Do we want to support const_cast for this? A possible way to do that is to invalidate this, when a const cast appears in the body of the function. (However the body might not be available. It is only my opinion, but I would be ok to accept this patch without const cast support, but it is something that we might want to document somewhere as a caveat.

[seaneveson]

Thank you for looking at the patch and all your comments.

In D13099#252492, @xazax.hun wrote:

One more note. Do we want to support const_cast for this? A possible way to do that is to invalidate this, when a const cast appears in the body of the function. (However the body might not be available. It is only my opinion, but I would be ok to accept this patch without const cast support, but it is something that we might want to document somewhere as a caveat.

IMO it is reasonable to assume that a const method wont modify non-mutable members. While it is possible to use casts or other pointers to make changes, I'm not sure it is worth checking for these cases.

lib/StaticAnalyzer/Core/CallEvent.cpp

409

I was getting a strange case where the PR21606 test didn't work without getCanonical, but when testing it now it seems to be fine. Perhaps something was fixed elsewhere? Regardless I'll remove the call. Thanks.

412

I put the test in because getCXXThisVal can return an UnknownVal, but on closer inspection I don't think this will ever happen for a CXXMemberCall, so I will change this to an assert. Thanks.

415

Good point, I don't think they are so I'll work on that.

[seaneveson]

Removed unnecessary call to getCanonical.
Changed if statement checking getting ThisRegion to an assert.
Added code to handle mutable members of base classes.

[xazax.hun]

A general style comment: you could decrease the level of indentation using early returns. I have one more comment inline, otherwise it looks good to me.

lib/StaticAnalyzer/Core/CallEvent.cpp

408

Does this check work for member operators? I wonder what is the reason that getDecl returns a FunctionDecl instead of CXXMethodDecl.

[zaks.anna]

The analyzer has a notion of ConstPointerEscape, see checkConstPointerEscape callback.
All the pointers to const parameters are escaped this way. The implementation for that is in CallEvent::invalidateRegions, right below the code you've added:

for (unsigned Idx = 0, Count = getNumArgs(); Idx != Count; ++Idx) {

  // Mark this region for invalidation.  We batch invalidate regions
  // below for efficiency.
  if (PreserveArgs.count(Idx))
    if (const MemRegion *MR = getArgSVal(Idx).getAsRegion())
      ETraits.setTrait(MR->StripCasts(), 
                      RegionAndSymbolInvalidationTraits::TK_PreserveContents);
      // TODO: Factor this out + handle the lower level const pointers.

  ValuesToInvalidate.push_back(getArgSVal(Idx));
}

I think we should const escape all non-mutable fields as well as 'this'.

(A motivation behind this callback is that one can call delete on pointers of const *void type.)

[seaneveson]

I've realized that the patch doesn't handle pointers correctly, since a const method can modify the memory pointed at by a member. While pointer members should not be invalidated by const methods (if they are not mutable), the memory they point to should still be invalidated. I'll address this in the next version.

Regarding ConstPointerEscape:

In D13099#253111, @zaks.anna wrote:

The analyzer has a notion of ConstPointerEscape, see checkConstPointerEscape callback.
All the pointers to const parameters are escaped this way. The implementation for that is in CallEvent::invalidateRegions, right below the code you've added:

...

I think we should const escape all non-mutable fields as well as 'this'.

(A motivation behind this callback is that one can call delete on pointers of const *void type.)

I think I understand, but to clarify:
The fields that shouldn't be invalidated should still be added to ValuesToInvalidate, but with RegionAndSymbolInvalidationTraits::TK_PreserveContents set. This will result in checkConstPointerEscape being called properly.

Is that correct?

[zaks.anna]

I think I understand, but to clarify:
The fields that shouldn't be invalidated should still be added to ValuesToInvalidate, but with
RegionAndSymbolInvalidationTraits::TK_PreserveContents set. This will result in
checkConstPointerEscape being called properly.

Is that correct?

Yes, I think that is exactly how that works.

[seaneveson]

Updated to latest revision.
Moved tests into their own file.
Added (RegionAndSymbolInvalidationTraits *ETraits) parameter to CallEvent::getExtraInvalidatedValues and overrides.
Prevent invalidation by using TK_PreserveContents (Which also fixes the pointer issue).
Simplified the patch so it falls back on invalidating the entire object if there is a mutable field, since invalidating a single field invalidated the entire object anyway.

[xazax.hun]

test/Analysis/const-method-call.cpp

26 ↗

(On Diff #36600)

Please add a test case, where only base have a mutable field.

[seaneveson]

There is an issue where pointers to the object (this) should cause it to be invalidated, but don't since TK_PreserveContents has been set.

For example:

class B;
class A {
  B b;
  const foo();
};
class B {
  A *ptr_a;
}

A a;
a.b.ptr_a = &a;
a.foo();

The method foo might modify 'this' via the object b, but 'this' will not be invalidated as foo is const.

Again I'm not sure this is worth checking for, based on the assumption that a reasonable const method won't modify the relevant object. What do people think?

[seaneveson]

Removed mutable field from derived class in inheritance test case.

[zaks.anna]

Again I'm not sure this is worth checking for, based on the assumption that a reasonable const method won't modify the relevant object. What do people think?

These are heuristics. I think dealing with this case should not be a pre-requisite for the patch. We can see how much this is an issue in practice through evaluation on existing codebases as well as getting user feedback once this lands.

test/Analysis/PR21606.cpp

3

Can this be moved into some other test file?
You can have the PR name in a comment or a method name.

test/Analysis/const-method-call.cpp

72 ↗

(On Diff #36604)

Maybe we should add a test case where the const method is inherited?

[seaneveson]

Move PR21606 test into const-method-call.cpp.
Added test for const calls on member objects.
Added tests for inherited const methods.
Changed TK_PreserveContents to be set for the base region. This is so memory is still preserved when 'this' is a member of another object.
Added code to get the instance's class, rather than the method's class. This solves issues where the method's class has no mutable members, but the class which inherits the method does.

[xazax.hun]

In D13099#260531, @seaneveson wrote:

There is an issue where pointers to the object (this) should cause it to be invalidated, but don't since TK_PreserveContents has been set.

For example:

class B;
class A {
  B b;
  const foo();
};
class B {
  A *ptr_a;
}

A a;
a.b.ptr_a = &a;
a.foo();

The method foo might modify 'this' via the object b, but 'this' will not be invalidated as foo is const.

Again I'm not sure this is worth checking for, based on the assumption that a reasonable const method won't modify the relevant object. What do people think?

I think, this can happen every time, when tehere is a cycle of pointers. Even if you do not deal with this case I think it would be great to have an XFAIL test with a fixme that describes this limitation.

In case we would like to fix this issue in the future, I think TK_PreserveContents is too weak. It would be great to have more control, when to preserve the content. E.g. a trait which describes that, this region can not be invalidated by that pointer. It is a good question whether a more precise solution can be efficient enough though.

[zaks.anna]

Even if you do not deal with this case I think it would be great to have an XFAIL test with a fixme that describes this limitation.

Yes! Every time we have a test case that shows known limitations, we should add it to the regression tests with a TODO explaining the issue. (I would not add it as a separate test that is XFAILed but rather add to an existing test file at the very end in the TODO or FIXME section. )

[seaneveson]

Updated to latest trunk.
Added FIXME test for circular reference issue.

[zaks.anna]

lib/StaticAnalyzer/Core/CallEvent.cpp

438

Use IgnoreParenImpCasts()?

[seaneveson]

Updated to latest trunk
Replaced some code with an existing method: ignoreParenBaseCasts()

Could someone commit this for me as I don't have SVN access?
Thank you.

[dcoughlin]

This patch seems to have caused a regression. https://llvm.org/bugs/show_bug.cgi?id=25392

lib/StaticAnalyzer/Core/CallEvent.cpp

const FunctionDecl *CXXInstanceCall::getDecl() const {
   const FunctionDecl *D = CE->getDirectCallee();
   if (D)
     return D;
 
   return getSVal(CE->getCallee()).getAsFunctionDecl();
 }
 
 void CXXInstanceCall::getExtraInvalidatedValues(ValueList &Values) const {
+  // Check if this is a call to a const method.
+  if (const CXXMethodDecl *D = cast_or_null<CXXMethodDecl>(getDecl())) {
+    if(D->getCanonicalDecl()->isConst()) {
+      // Check if the containing class/struct has mutable members
+      const MemRegion *ThisRegion = getCXXThisVal().getAsRegion();
+      if (ThisRegion) {
+        MemRegionManager *MemMgr = ThisRegion->getMemRegionManager();
+        const CXXRecordDecl *Parent = D->getParent();
+        for (const auto *I : Parent->fields()) {
+          if (I->isMutable()) {
+            const FieldRegion *FR = MemMgr->getFieldRegion(I, ThisRegion);
+            Values.push_back(loc::MemRegionVal(FR));
+          }
+        }
+      }
+      return;
+    }
+  }
   Values.push_back(getCXXThisVal());
 }
 
 SVal CXXInstanceCall::getCXXThisVal() const {
   const Expr *Base = getCXXThisExpr();
   // FIXME: This doesn't handle an overloaded ->* operator.
   if (!Base)
     return UnknownVal();
 
   SVal ThisVal = getSVal(Base);
   assert(ThisVal.isUnknownOrUndef() || ThisVal.getAs<Loc>());
   return ThisVal;
 }
 
 
 RuntimeDefinition CXXInstanceCall::getRuntimeDefinition() const {
   // Do we have a decl at all?
   const Decl *D = getDecl();
   if (!D)
     return RuntimeDefinition();
 
   // If the method is non-virtual, we know we can inline it.

test/Analysis/PR21606.cpp

+// RUN: %clang_cc1 -analyze -analyzer-checker=core
+// PR21606
+
+struct s1 {
+    void g(const int *i) const;
+};
+
+struct s2 {
+    void f(int *i) {
+        m_i = i;
+        m_s.g(m_i);
+        if (m_i)
+            *i = 42; // no-warning
+    }
+
+    int *m_i;
+    s1 m_s;
+};
+
+int main()
+{
+    s2().f(0);
+}
 No newline at end of file

test/Analysis/method-call.cpp

 // RUN: %clang_cc1 -analyze -analyzer-checker=core,debug.ExprInspection -analyzer-config c++-inlining=constructors -verify %s
 
 void clang_analyzer_eval(bool);
 
 
 struct A {
   int x;
   A(int a) { x = a; }
   int getx() const { return x; }
 };
 
 struct B{
   int x;
 };
 
+struct C {
+  int x;
+  void foo() const;
+  void bar();
+};
+
+struct D {
+  mutable int x;
+  void foo() const;
+};
+
 void testNullObject(A *a) {
   clang_analyzer_eval(a); // expected-warning{{UNKNOWN}}
   (void)a->getx(); // assume we know what we're doing
   clang_analyzer_eval(a); // expected-warning{{TRUE}}
 }
 
 void f1() {
   A x(3);

 }
 
 void checkThatCopyConstructorDoesNotInvalidateObjectBeingCopied() {
   B t;
   t.x = 0;
   B t2(t);
   clang_analyzer_eval(t.x == 0); // expected-warning{{TRUE}}
 }
+
+void checkThatConstMethodWithoutDefinitionDoesNotInvalidateObject() {
+  C t;
+  t.x = 3;
+  t.foo();
+  clang_analyzer_eval(t.x == 3); // expected-warning{{TRUE}}
+  // Test non-const does invalidate
+  t.bar();
+  clang_analyzer_eval(t.x); // expected-warning{{UNKNOWN}}
+}
+
+void checkThatConstMethodDoesInvalidateMutableFields() {
+  D t;
+  t.x = 3;
+  t.foo();
+  clang_analyzer_eval(t.x); // expected-warning{{UNKNOWN}}
+}
+