This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] new checker cppcoreguidelines-pro-lifetime
AbandonedPublic

Authored by mgehre on Nov 26 2015, 4:00 PM.

Details

Summary

This checker implements the lifetime rules presented in the paper
by Herb Sutter and Neil MacIntosh paper [1].
Basically, for each "Pointer" we track a set of possible "Owners" to which it
it directly or transitivly points, called its pset (points-to set).
If an Owner is invalidated, all Pointers that contain that Owner
in their pset get an "invalid" pset. For an in-depth explanation, please refer to the paper.

"Pointers" are not only variable with pointer type, but also references,
and objects that contain Pointers (e.g. iterators).

A pset can contain "null", "invalid", "static" (for static duration Owners that cannot
be invalidated), and tuples (Owner, order). Order 0 means that the
Pointer points to the object, e.g. int* p = &i => pset(p) = {(i,0)}.
Order 1 means that the Pointer points to something owned by the object,
e.g. int* p = o.getp() => pset(p) = {(o,1)}.

The paper, and thus the checker, are implemented in a path-independent
way. That leads to some false positives, but increases speed and
allows e.g. to reason about loops without unrolling.

Function bodies are analyzed separately. When calling a function,
the caller can assume how the psets of the Pointers change.
Those assumptions are checked for each analyzed function. (Just like for
const methods; callers assume that the object does not change; bodies of
const methods are not allowed to change the object).

The checker assumes that the code to be checked is valid
according to the C++ Core Guidelines type and bounds profile.
If "forbidden" expressions (such as pointer arithmetic) are used,
or if an unimplemented expression is encountered, the resulting pset
will be "unknown". Psets containing "unknown" will not cause any
warnings by the checker.

The checker will emit the pset of an variable if it finds a call to
"clang_analyzer_pset" in the C++ code. This is used by the tests to
display the pset of a Pointer.

For now, the checker is split into ProLifetimeCheck.cpp and
ProLifetimeVisitor.cpp. The only reason is that it speeds up the
compile time for me. ProLifetimeCheck.cpp is quite slow to compile
(I guess due to ASTMatchers.h). I can happily merge both cpp files before
the commiting, if required.

This checker is not complete. I would like to hear your comments
about high level stuff (e.g. architecture) and what is the best way
forward to get this upstream.

Currently state:

  • Pointers are only variables of pointer type or reference type (no

objects containing Pointers, like iterators).

  • Psets are computed through all expressions and conditionals.
  • "(null)" will be removed from a pset in a branch

if the pointer appears in a conditional guarding that branch. It does
not handle all types of conditionals yet.

  • Pointers are invalidated when the Owner they point to goes out of scope.
  • Dereferencing invalid pointers is flagged.
  • Pointers with static duration will be checked on assignment to only

have a pset of (null) and/or (static). Anything else is flagged.

  • CFGs with loops are supported and lead to an iterative refinement
  • I have run this on the llvm code base, and the diagnosed issues are mostly related to null pointer dereferencing. I would not call them

false-positives, because this checker assumes that pointer
arguments can be null, and otherwise gsl::not_null should be used.

If you like to see what it can diagnose, look at

test/clang-tidy/cppcoreguidelines-pro-lifetime.cpp

For the internal pset propagation, look at

test/clang-tidy/cppcoreguidelines-pro-lifetime-psets.cpp

(incomplete) TODO:

  • track psets for objects containing Pointers
  • compute pset of return values and out-parameters in function calls
  • check correct pset in 'return' statments
  • support annotations for non-standard lifetime behavior

[1] https://github.com/isocpp/CppCoreGuidelines/blob/master/docs/Lifetimes%20I%20and%20II%20-%20v0.9.1.pdf

Depends on D15031

Diff Detail

Event Timeline

mgehre updated this revision to Diff 41279.Nov 26 2015, 4:00 PM
mgehre retitled this revision from to [clang-tidy] new checker cppcoreguidelines-pro-lifetime.
mgehre updated this object.
mgehre added a subscriber: cfe-commits.
zaks.anna resigned from this revision.Jan 29 2016, 11:32 PM
zaks.anna removed a reviewer: zaks.anna.
alexfh requested changes to this revision.Apr 1 2016, 7:33 PM
alexfh edited edge metadata.

FYI, I'm waiting with reviewing this change until D15031 is landed, since it can affect this patch.

This revision now requires changes to proceed.Apr 1 2016, 7:33 PM
mgehre updated this revision to Diff 53097.Apr 8 2016, 3:11 PM
mgehre edited edge metadata.

Update for renaming in http://reviews.llvm.org/D15031

aaron.ballman edited edge metadata.Apr 11 2016, 7:35 AM

Thank you for working on this! A few initial nits (I don't have the chance to complete the review yet, but wanted to get you some early feedback).

clang-tidy/cppcoreguidelines/ProLifetimeCheck.cpp
34–35

These can be combined into a single matcher using anyOf() (should be slightly more efficient than registering two separate matchers).

clang-tidy/cppcoreguidelines/ProLifetimeCheck.h
18

Should write that short description. :-)

clang-tidy/cppcoreguidelines/ProLifetimeVisitor.cpp
94–97

I don't believe there's a need to explicitly default these (here and elsewhere), so they should just be removed.

99–101

Should be O instead of o per our usual conventions, here and elsewhere.

103–104

I would feel more comfortable using std::les<> for this to avoid comparisons between unrelated pointers not having a total ordering.

121

Comments should be properly punctuated (here and elsewhere).

153

Should remove this instead of leaving it commented out.

178

Don't use all caps for enumerators (that's generally reserved for just macros by our conventions), here and elsewhere.

229

I think this should return None instead.

548–549

Reformat the comment to fit a bit better?

552

Method can be const qualified.

alexfh requested changes to this revision.Jan 5 2018, 6:28 AM

D15031 has landed quite a while ago. Could you rebase the patch?

This revision now requires changes to proceed.Jan 5 2018, 6:28 AM
mgehre abandoned this revision.Jul 21 2019, 2:55 PM

Upstreaming of this checker will happen with D63954 and following revisions.