[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 ↗

(On Diff #35496)

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

412 ↗

(On Diff #35496)

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

415 ↗

(On Diff #35496)

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 ↗

(On Diff #35496)

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 ↗

(On Diff #35496)

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 ↗

(On Diff #35496)

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

422 ↗

(On Diff #35617)

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

2 ↗

(On Diff #36604)

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

419 ↗

(On Diff #36927)

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

cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h

SVal getSVal(const Stmt *S) const {
     return getState()->getSVal(S, getLocationContext());
   }
 
 
   typedef SmallVectorImpl<SVal> ValueList;
 
   /// \brief Used to specify non-argument regions that will be invalidated as a
   /// result of this call.
-  virtual void getExtraInvalidatedValues(ValueList &Values) const {}
+  virtual void getExtraInvalidatedValues(ValueList &Values,
+                 RegionAndSymbolInvalidationTraits *ETraits) const {}
 
 public:
   virtual ~CallEvent() {}
 
   /// \brief Returns the kind of call this is.
   virtual Kind getKind() const = 0;
 
   /// \brief Returns the declaration of the function or method that will be

 protected:
   BlockCall(const CallExpr *CE, ProgramStateRef St,
             const LocationContext *LCtx)
     : CallEvent(CE, St, LCtx) {}
 
   BlockCall(const BlockCall &Other) : CallEvent(Other) {}
   void cloneTo(void *Dest) const override { new (Dest) BlockCall(*this); }
 
-  void getExtraInvalidatedValues(ValueList &Values) const override;
+  void getExtraInvalidatedValues(ValueList &Values,
+         RegionAndSymbolInvalidationTraits *ETraits) const override;
 
 public:
   virtual const CallExpr *getOriginExpr() const {
     return cast<CallExpr>(CallEvent::getOriginExpr());
   }
 
   unsigned getNumArgs() const override { return getOriginExpr()->getNumArgs(); }
 
static bool classof(const CallEvent *CA) {
     return CA->getKind() == CE_Block;
   }
 };
 
 /// \brief Represents a non-static C++ member function call, no matter how
 /// it is written.
 class CXXInstanceCall : public AnyFunctionCall {
 protected:
-  void getExtraInvalidatedValues(ValueList &Values) const override;
+  void getExtraInvalidatedValues(ValueList &Values, 
+         RegionAndSymbolInvalidationTraits *ETraits) const override;
 
   CXXInstanceCall(const CallExpr *CE, ProgramStateRef St,
                   const LocationContext *LCtx)
     : AnyFunctionCall(CE, St, LCtx) {}
   CXXInstanceCall(const FunctionDecl *D, ProgramStateRef St,
                   const LocationContext *LCtx)
     : AnyFunctionCall(D, St, LCtx) {}
 
CXXConstructorCall(const CXXConstructExpr *CE, const MemRegion *Target,
                      ProgramStateRef St, const LocationContext *LCtx)
     : AnyFunctionCall(CE, St, LCtx) {
     Data = Target;
   }
 
   CXXConstructorCall(const CXXConstructorCall &Other) : AnyFunctionCall(Other){}
   void cloneTo(void *Dest) const override { new (Dest) CXXConstructorCall(*this); }
 
-  void getExtraInvalidatedValues(ValueList &Values) const override;
+  void getExtraInvalidatedValues(ValueList &Values,
+         RegionAndSymbolInvalidationTraits *ETraits) const override;
 
 public:
   virtual const CXXConstructExpr *getOriginExpr() const {
     return cast<CXXConstructExpr>(AnyFunctionCall::getOriginExpr());
   }
 
   const CXXConstructorDecl *getDecl() const override {
     return getOriginExpr()->getConstructor();
ObjCMethodCall(const ObjCMessageExpr *Msg, ProgramStateRef St,
                  const LocationContext *LCtx)
     : CallEvent(Msg, St, LCtx) {
     Data = nullptr;
   }
 
   ObjCMethodCall(const ObjCMethodCall &Other) : CallEvent(Other) {}
   void cloneTo(void *Dest) const override { new (Dest) ObjCMethodCall(*this); }
 
-  void getExtraInvalidatedValues(ValueList &Values) const override;
+  void getExtraInvalidatedValues(ValueList &Values,
+         RegionAndSymbolInvalidationTraits *ETraits) const override;
 
   /// Check if the selector may have multiple definitions (may have overrides).
   virtual bool canBeOverridenInSubclass(ObjCInterfaceDecl *IDecl,
                                         Selector Sel) const;
 
 public:
   virtual const ObjCMessageExpr *getOriginExpr() const {
     return cast<ObjCMessageExpr>(CallEvent::getOriginExpr());

cfe/trunk/lib/StaticAnalyzer/Core/CallEvent.cpp

ProgramStateRef CallEvent::invalidateRegions(unsigned BlockCount,
   // Don't invalidate anything if the callee is marked pure/const.
   if (const Decl *callee = getDecl())
     if (callee->hasAttr<PureAttr>() || callee->hasAttr<ConstAttr>())
       return Result;
 
   SmallVector<SVal, 8> ValuesToInvalidate;
   RegionAndSymbolInvalidationTraits ETraits;
 
-  getExtraInvalidatedValues(ValuesToInvalidate);
+  getExtraInvalidatedValues(ValuesToInvalidate, &ETraits);
 
   // Indexes of arguments whose values will be preserved by the call.
   llvm::SmallSet<unsigned, 4> PreserveArgs;
   if (!argumentsMayEscape())
     findPtrToConstParams(PreserveArgs, *this);
 
   for (unsigned Idx = 0, Count = getNumArgs(); Idx != Count; ++Idx) {
     // Mark this region for invalidation.  We batch invalidate regions
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 {
-  Values.push_back(getCXXThisVal());
+void CXXInstanceCall::getExtraInvalidatedValues(ValueList &Values,
+                        RegionAndSymbolInvalidationTraits *ETraits) const {
+  SVal ThisVal = getCXXThisVal();
+  Values.push_back(ThisVal);
+
+  // Don't invalidate if the method is const and there are no mutable fields
+  if (const CXXMethodDecl *D = cast_or_null<CXXMethodDecl>(getDecl())) {
+    if (!D->isConst())
+      return;
+    // Get the record decl for the class of 'This'. D->getParent() may return a
+    // base class decl, rather than the class of the instance which needs to be
+    // checked for mutable fields.
+    const Expr *Ex = getCXXThisExpr()->ignoreParenBaseCasts();
+    const CXXRecordDecl *ParentRecord = Ex->getType()->getAsCXXRecordDecl();
+    if (!ParentRecord || ParentRecord->hasMutableFields())
+      return;
+    // Preserve CXXThis.
+    const MemRegion *ThisRegion = ThisVal.getAsRegion();
+    assert(ThisRegion && "ThisValue was not a memory region");
+    ETraits->setTrait(ThisRegion->getBaseRegion(),
+      RegionAndSymbolInvalidationTraits::TK_PreserveContents);
+  }
 }
 
 SVal CXXInstanceCall::getCXXThisVal() const {
   const Expr *Base = getCXXThisExpr();
   // FIXME: This doesn't handle an overloaded ->* operator.
   if (!Base)
     return UnknownVal();
 

 
 ArrayRef<ParmVarDecl*> BlockCall::parameters() const {
   const BlockDecl *D = getDecl();
   if (!D)
     return nullptr;
   return D->parameters();
 }
 
-void BlockCall::getExtraInvalidatedValues(ValueList &Values) const {
+void BlockCall::getExtraInvalidatedValues(ValueList &Values,
+                  RegionAndSymbolInvalidationTraits *ETraits) const {
   // FIXME: This also needs to invalidate captured globals.
   if (const MemRegion *R = getBlockRegion())
     Values.push_back(loc::MemRegionVal(R));
 }
 
 void BlockCall::getInitialStackFrameContents(const StackFrameContext *CalleeCtx,
                                              BindingsTy &Bindings) const {
   const BlockDecl *D = cast<BlockDecl>(CalleeCtx->getDecl());
   SValBuilder &SVB = getState()->getStateManager().getSValBuilder();
   addParameterValuesToBindings(CalleeCtx, Bindings, SVB, *this,
                                D->parameters());
 }
 
 
 SVal CXXConstructorCall::getCXXThisVal() const {
   if (Data)
     return loc::MemRegionVal(static_cast<const MemRegion *>(Data));
   return UnknownVal();
 }
 
-void CXXConstructorCall::getExtraInvalidatedValues(ValueList &Values) const {
+void CXXConstructorCall::getExtraInvalidatedValues(ValueList &Values,
+                           RegionAndSymbolInvalidationTraits *ETraits) const {
   if (Data)
     Values.push_back(loc::MemRegionVal(static_cast<const MemRegion *>(Data)));
 }
 
 void CXXConstructorCall::getInitialStackFrameContents(
                                              const StackFrameContext *CalleeCtx,
                                              BindingsTy &Bindings) const {
   AnyFunctionCall::getInitialStackFrameContents(CalleeCtx, Bindings);

 ArrayRef<ParmVarDecl*> ObjCMethodCall::parameters() const {
   const ObjCMethodDecl *D = getDecl();
   if (!D)
     return None;
   return D->parameters();
 }
 
 void
-ObjCMethodCall::getExtraInvalidatedValues(ValueList &Values) const {
+ObjCMethodCall::getExtraInvalidatedValues(ValueList &Values,
+                  RegionAndSymbolInvalidationTraits *ETraits) const {
   Values.push_back(getReceiverSVal());
 }
 
 SVal ObjCMethodCall::getSelfSVal() const {
   const LocationContext *LCtx = getLocationContext();
   const ImplicitParamDecl *SelfDecl = LCtx->getSelfDecl();
   if (!SelfDecl)
     return SVal();

cfe/trunk/test/Analysis/const-method-call.cpp

+// RUN: %clang_cc1 -analyze -analyzer-checker=core,debug.ExprInspection -verify %s
+
+void clang_analyzer_eval(bool);
+
+struct A {
+  int x;
+  void foo() const;
+  void bar();
+};
+
+struct B {
+  mutable int mut;
+  void foo() const;
+};
+
+struct C {
+  int *p;
+  void foo() const;
+};
+
+struct MutBase {
+  mutable int b_mut;
+};
+
+struct MutDerived : MutBase {
+  void foo() const;
+};
+
+struct PBase {
+  int *p;
+};
+
+struct PDerived : PBase {
+  void foo() const;
+};
+
+struct Inner {
+  int x;
+  int *p;
+  void bar() const;
+};
+
+struct Outer {
+  int x;
+  Inner in;
+  void foo() const;
+};
+
+void checkThatConstMethodWithoutDefinitionDoesNotInvalidateObject() {
+  A 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() {
+  B t;
+  t.mut = 4;
+  t.foo();
+  clang_analyzer_eval(t.mut); // expected-warning{{UNKNOWN}}
+}
+
+void checkThatConstMethodDoesInvalidatePointedAtMemory() {
+  int x = 1;
+  C t;
+  t.p = &x;
+  t.foo();
+  clang_analyzer_eval(x); // expected-warning{{UNKNOWN}}
+  clang_analyzer_eval(t.p == &x); // expected-warning{{TRUE}}
+}
+
+void checkThatConstMethodDoesInvalidateInheritedMutableFields() {
+  MutDerived t;
+  t.b_mut = 4;
+  t.foo();
+  clang_analyzer_eval(t.b_mut); // expected-warning{{UNKNOWN}}
+}
+
+void checkThatConstMethodDoesInvalidateInheritedPointedAtMemory() {
+  int x = 1;
+  PDerived t;
+  t.p = &x;
+  t.foo();
+  clang_analyzer_eval(x); // expected-warning{{UNKNOWN}}
+  clang_analyzer_eval(t.p == &x); // expected-warning{{TRUE}}
+}
+
+void checkThatConstMethodDoesInvalidateContainedPointedAtMemory() {
+  int x = 1;
+  Outer t;
+  t.x = 2;
+  t.in.p = &x;
+  t.foo();
+  clang_analyzer_eval(x); // expected-warning{{UNKNOWN}}
+  clang_analyzer_eval(t.x == 2); // expected-warning{{TRUE}}
+  clang_analyzer_eval(t.in.p == &x); // expected-warning{{TRUE}}
+}
+
+void checkThatContainedConstMethodDoesNotInvalidateObjects() {
+  Outer t;
+  t.x = 1;
+  t.in.x = 2;
+  t.in.bar();
+  clang_analyzer_eval(t.x == 1); // expected-warning{{TRUE}}
+  clang_analyzer_eval(t.in.x == 2); // expected-warning{{TRUE}}
+}
+
+// --- Versions of the above tests where the const method is inherited --- //
+
+struct B1 {
+  void foo() const;
+};
+
+struct D1 : public B1 {
+  int x;
+};
+
+struct D2 : public B1 {
+  mutable int mut;
+};
+
+struct D3 : public B1 {
+  int *p;
+};
+
+struct DInner : public B1 {
+  int x;
+  int *p;
+};
+
+struct DOuter : public B1 {
+  int x;
+  DInner in;
+};
+
+void checkThatInheritedConstMethodDoesNotInvalidateObject() {
+  D1 t;
+  t.x = 1;
+  t.foo();
+  clang_analyzer_eval(t.x == 1); // expected-warning{{TRUE}}
+}
+
+void checkThatInheritedConstMethodDoesInvalidateMutableFields() {
+  D2 t;
+  t.mut = 1;
+  t.foo();
+  clang_analyzer_eval(t.mut); // expected-warning{{UNKNOWN}}
+}
+
+void checkThatInheritedConstMethodDoesInvalidatePointedAtMemory() {
+  int x = 1;
+  D3 t;
+  t.p = &x;
+  t.foo();
+  clang_analyzer_eval(x); // expected-warning{{UNKNOWN}}
+  clang_analyzer_eval(t.p == &x); // expected-warning{{TRUE}}
+}
+
+void checkThatInheritedConstMethodDoesInvalidateContainedPointedAtMemory() {
+  int x = 1;
+  DOuter t;
+  t.x = 2;
+  t.in.x = 3;
+  t.in.p = &x;
+  t.foo();
+  clang_analyzer_eval(x); // expected-warning{{UNKNOWN}}
+  clang_analyzer_eval(t.x == 2); // expected-warning{{TRUE}}
+  clang_analyzer_eval(t.in.x == 3); // expected-warning{{TRUE}}
+  clang_analyzer_eval(t.in.p == &x); // expected-warning{{TRUE}}
+}
+
+void checkThatInheritedContainedConstMethodDoesNotInvalidateObjects() {
+  DOuter t;
+  t.x = 1;
+  t.in.x = 2;
+  t.in.foo();
+  clang_analyzer_eval(t.x == 1); // expected-warning{{TRUE}}
+  clang_analyzer_eval(t.in.x == 2); // expected-warning{{TRUE}}
+}
+
+// --- 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;
+};
+
+void PR21606()
+{
+    s2().f(0);
+}
+
+// FIXME
+// When there is a circular reference to an object and a const method is called
+// the object is not invalidated because TK_PreserveContents has already been
+// set.
+struct Outer2;
+
+struct InnerWithRef {
+  Outer2 *ref;
+};
+
+struct Outer2 {
+  int x;
+  InnerWithRef in;
+  void foo() const;
+};
+
+void checkThatConstMethodCallDoesInvalidateObjectForCircularReferences() {
+  Outer2 t;
+  t.x = 1;
+  t.in.ref = &t;
+  t.foo();
+  // FIXME: Should be UNKNOWN.
+  clang_analyzer_eval(t.x); // expected-warning{{TRUE}}
+}