Page MenuHomePhabricator

[analyzer] CastValueChecker: Model castAs(), getAs()
ClosedPublic

Authored by Charusso on Wed, Aug 7, 8:56 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

Charusso created this revision.Wed, Aug 7, 8:56 AM
Charusso marked 4 inline comments as done.Wed, Aug 7, 9:01 AM

This is a little-bit WIP as the symbol conjuring is very naive.

clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp
111 ↗(On Diff #213919)

That is a very lame way to conjure a symbol, but with a cool wrapper I believe this would be okay. Do we have a better way to create a non-null symbol?

clang/lib/StaticAnalyzer/Core/CallEvent.cpp
718 ↗(On Diff #213919)

This is the only necessary addition to make it usable.

clang/test/Analysis/cast-value.cpp
113 ↗(On Diff #213919)

I am not sure what is going on here,

131 ↗(On Diff #213919)

and here.

193 ↗(On Diff #213919)

Known-value printing fails, but at least it is working well.

NoQ added inline comments.Wed, Aug 7, 10:39 AM
clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp
111 ↗(On Diff #213919)

This creates a concrete 1 rather than a symbol. That's the right behavior for isa, but it's not the right behavior for getAs. In case of getAs we need to return our this.

Charusso added inline comments.Wed, Aug 7, 10:47 AM
clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp
111 ↗(On Diff #213919)

Could you explain what is this, please? At this case we are working with a CXXMemberCallExpr. Do you mean its getImplicitObjectArgument()?

NoQ added inline comments.Wed, Aug 7, 10:48 AM
clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp
111 ↗(On Diff #213919)

Yeah, its value.

Charusso updated this revision to Diff 213952.Wed, Aug 7, 11:07 AM
Charusso marked 5 inline comments as done.
  • Remove symbol-conjuring.
  • Add a forgotten test case.
clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp
111 ↗(On Diff #213919)

Hm, I have not understand why it is not working as I definitely wanted to use this. I believe I had forgotten an early-return at a wrong place. Thanks!

NoQ added inline comments.Wed, Aug 7, 1:53 PM
clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp
51 ↗(On Diff #213952)

I'd rather have only one map from call descriptions to (callback, kind) pairs. This should make the evalCall code much more readable.

215 ↗(On Diff #213952)

Use Call.getResultType() instead.

And please add a test with references, as getResultType behaves more correctly for references:

C &c = ...;
D &d = dyn_cast<D>(c);
223 ↗(On Diff #213952)

*Call.parameters()[0]->getType(). It should also help with references.

231 ↗(On Diff #213952)
const auto *InstanceCall = dyn_cast<CXXInstanceCall>(&Call);
...
DV = InstanceCall->getCXXThisVal();
clang/lib/StaticAnalyzer/Core/CallEvent.cpp
718 ↗(On Diff #213952)

I don't think you need this anymore.

clang/test/Analysis/cast-value.cpp
186 ↗(On Diff #213952)

To think: this note isn't very useful because hard casts aren't really supposed to fail anyway.

Charusso updated this revision to Diff 214037.Wed, Aug 7, 5:14 PM
Charusso marked 9 inline comments as done.
  • Make it usable with references.
  • Test references.
  • Better messages on simple cast<>.
clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp
51 ↗(On Diff #213952)

The problem with that I really want to use something like that:

auto &&[Check, Kind, MoreData] = *Lookup;

but I cannot. Until that time it equally non-readable and difficult to scale sadly. For now it is fine, but that C++17 feature would be more awesome.

231 ↗(On Diff #213952)

Hm, not bad. Thanks!

clang/test/Analysis/cast-value.cpp
186 ↗(On Diff #213952)

I believe it is better than the generic Assuming... piece.

Charusso updated this revision to Diff 214038.Wed, Aug 7, 5:15 PM
  • Fix a comment.
Charusso updated this revision to Diff 214041.Wed, Aug 7, 5:27 PM
  • May it is better to also check for getAsCXXRecordDecl() for obtaining a class.
Charusso updated this revision to Diff 214186.Thu, Aug 8, 11:52 AM
  • The call as getAsCXXRecordDecl() sometimes run into an assertion failure, so we need to avoid it:
clang/lib/StaticAnalyzer/Core/ExprEngine.cpp:333:
ExprEngine::createTemporaryRegionIfNeeded():
Assertion `!InitValWithAdjustments.getAs<Loc>() || Loc::isLocType(Result->getType()) || Result->getType()->isMemberPointerType()' failed.
NoQ added a comment.Thu, Aug 8, 12:58 PM

Looking good now! I still recommend eventually adding tests with casts of references.

  • May it is better to also check for getAsCXXRecordDecl() for obtaining a class.
  • The call as getAsCXXRecordDecl() sometimes run into an assertion failure, so we need to avoid it

Well, yeah, i was just about to say "good thing that you don't do this because we really don't want to get into business of modeling return-by-value getAs calls such as SVal::getAs".

clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp
51 ↗(On Diff #213952)

I think it turned out pretty pretty (no pun intended).

26 ↗(On Diff #214038)

Please explain "Checking" and "Sugar". Checking what? Sugar... you mean syntactic sugar? For what?

220 ↗(On Diff #214186)

What's the point of making this a raw pointer? (no pun intended)

NoQ added inline comments.Thu, Aug 8, 1:08 PM
clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp
94 ↗(On Diff #214186)

IsDynamicCast.

98 ↗(On Diff #214186)

More suggestions for the :-branch: "Hard cast" (a bit too jargon-y), "Safe cast", "Checked cast".

clang/test/Analysis/cast-value.cpp
156–167 ↗(On Diff #214186)

Mmm, wait a sec. That's a false positive. cast<> doesn't accept null pointers. We have cast_or_null for this.

Charusso updated this revision to Diff 214234.Thu, Aug 8, 2:29 PM
Charusso marked 7 inline comments as done.
  • Added a test case for casting *to* a reference.
  • Better naming.
clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp
26 ↗(On Diff #214038)

I have no idea what devs mean by those names:

  • Checking kind:

The cast<> operator is a “checked cast” operation.
The dyn_cast<> operator is a “checking cast” operation.

from http://llvm.org/docs/ProgrammersManual.html#the-isa-cast-and-dyn-cast-templates

  • Sugar kind:

Member-template castAs<specific type>. Look through sugar for the underlying instance of <specific type>.
Member-template getAs<specific type>'. Look through sugar for an instance of <specific type>.

from https://clang.llvm.org/doxygen/classclang_1_1Type.html#a436b8b08ae7f2404b4712d37986194ce
and https://clang.llvm.org/doxygen/classclang_1_1Type.html#adae68e1f4c85ede2d36da45fbefc48a2

  • isa() would be Instance kind:

The isa<> operator works exactly like the Java “instanceof” operator.

from http://llvm.org/docs/ProgrammersManual.html#the-isa-cast-and-dyn-cast-templates

If you could imagine better names, please let me know. I have tried to use the definitions.

94 ↗(On Diff #214186)

It is not dyn_cast but simple cast. Because we have decided to use Checked for that cast, I believe IsCheckedCast the one.

98 ↗(On Diff #214186)

Well, let us pick Checked then as that is the proper definition for cast(), but that other sugar-based castAs() definition makes me mad.

220 ↗(On Diff #214186)

I just wanted to make it usable as it being used since the first review as (*Check) and emphasize it is not a given function-call but a pointer to one. If you think as a nude Check() it is fine, then I have no idea which is better and it is fine for me as well.

clang/test/Analysis/cast-value.cpp
156–167 ↗(On Diff #214186)

This Assuming pointer value is null note is very random. I believe it is not made by me and my code is fine, so I have printed a graph:


Do you see any problem?

NoQ accepted this revision.Thu, Aug 8, 6:31 PM

Aha, ok!

clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp
26 ↗(On Diff #214038)

{ Function, Method }?

clang/test/Analysis/cast-value.cpp
156–167 ↗(On Diff #214186)

Whoops, sorry, i didn't notice the !. Seems fine.

Yeah, the note is broken. I have another interesting reproducer for a problem with the same note: https://bugs.llvm.org/show_bug.cgi?id=42938

This revision is now accepted and ready to land.Thu, Aug 8, 6:31 PM
Charusso updated this revision to Diff 214277.Thu, Aug 8, 7:02 PM
Charusso marked 6 inline comments as done.
  • Better naming.
clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp
26 ↗(On Diff #214038)

These are cool identifiers, thanks!

clang/test/Analysis/cast-value.cpp
156–167 ↗(On Diff #214186)

I will leave that not "Done" as FIXME: Broken notes.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptThu, Aug 8, 7:25 PM