This is an archive of the discontinued LLVM Phabricator instance.

[analyzer] Track leaking object through stores
ClosedPublic

Authored by vsavchenko on Apr 20 2021, 7:12 AM.

Details

Summary

Since we can report memory leaks on one variable, while the originally
allocated object was stored into another one, we should explain
how did it get there.

rdar://76645710

Diff Detail

Event Timeline

vsavchenko created this revision.Apr 20 2021, 7:12 AM
vsavchenko requested review of this revision.Apr 20 2021, 7:12 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 20 2021, 7:12 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
NoQ added a comment.Apr 21 2021, 1:08 AM

I think this is already way better than before. I see that OSDynamicCast isn't supported yet and we seem to hit the same wall as D97183 because inter-operation between trackExpressionValue and the checkers isn't implemented yet.

clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp
636

Because we already know that Val.getAsLocSymbol() is equal to Sym, we can be certain that Val is either a &SymRegion{Sym} (i.e., literally this symbol in its pristine representation) or, in some rare cases, a LocAsInteger over that (which is a case i'm not sure we even want to handle). I dunno if we really need to return it here. Maybe it's easier to re-construct it in place as SValBuilder.makeLoc(Sym).

978

In particular, this check is always true and you can use castAs.

vsavchenko marked an inline comment as done.

Add a comment and replace getAs with castAs

I think this is already way better than before. I see that OSDynamicCast isn't supported yet and we seem to hit the same wall as D97183 because inter-operation between trackExpressionValue and the checkers isn't implemented yet.

Let's say that OSDynamicCast isn't fully supported. If we have a chain of bindings var1 -> var2 -> ... -> varN and varM -> varM+1 is a dynamic cast assignment, we will create a note that varM+1 is assigned/initialized here (as you can see in tests), but not further down the chain. check_dynamic_cast_alias_intermediate is a good example here.
We still have work to do, but in the most common case where var1 -> var2 is the whole chain, we work as expected.

clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp
636

Oh, that was my original solution, but it doesn't work for dynamic casts.
This check compares symbols, but FindLastStore compares SVals. &Derived{&SymRegion} is not the same as &SymRegion and we fail.
So, instead of hacking on the result from SValBuilder trying to make something that will indeed match the last stored value, I save that last stored value here, so we're guaranteed that FindLastStore will work as intended.

978

Yep, makes sense!

NoQ accepted this revision.Apr 25 2021, 11:34 PM
NoQ added inline comments.
clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp
636

Oh, C++. Indeed. Our pointer cast representation strikes again. Ok then!

This revision is now accepted and ready to land.Apr 25 2021, 11:34 PM
This revision was automatically updated to reflect the committed changes.