This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by seaneveson on Sep 23 2015, 7:25 AM.

Details

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

Diff Detail

Repository
rL LLVM

Event Timeline

seaneveson updated this revision to Diff 35496.Sep 23 2015, 7:25 AM
seaneveson retitled this revision from to [Analyzer] Don’t invalidate CXXThis when conservatively evaluating const methods (PR 21606).
seaneveson updated this object.
seaneveson added a subscriber: cfe-commits.

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?

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.

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

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 updated this revision to Diff 35617.Sep 24 2015, 6:18 AM

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

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 edited edge metadata.Sep 24 2015, 3:34 PM

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

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:

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?

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 this revision to Diff 36600.Oct 6 2015, 2:27 AM
seaneveson edited edge metadata.

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 added inline comments.Oct 6 2015, 2:38 AM
test/Analysis/const-method-call.cpp
26 ↗(On Diff #36600)

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

seaneveson marked 6 inline comments as done.Oct 6 2015, 2:47 AM

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 updated this revision to Diff 36604.Oct 6 2015, 2:58 AM

Removed mutable field from derived class in inheritance test case.

seaneveson marked an inline comment as done.Oct 6 2015, 3:00 AM

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 updated this revision to Diff 36845.Oct 8 2015, 5:48 AM

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.

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.

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 this revision to Diff 36927.Oct 9 2015, 1:35 AM

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

zaks.anna accepted this revision.Oct 9 2015, 10:49 AM
zaks.anna edited edge metadata.
zaks.anna added inline comments.
lib/StaticAnalyzer/Core/CallEvent.cpp
419 ↗(On Diff #36927)

Use IgnoreParenImpCasts()?

This revision is now accepted and ready to land.Oct 9 2015, 10:49 AM
seaneveson updated this revision to Diff 37084.Oct 12 2015, 1:50 AM
seaneveson edited edge metadata.

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.

This revision was automatically updated to reflect the committed changes.
dcoughlin edited edge metadata.Nov 3 2015, 12:58 PM

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