Page MenuHomePhabricator

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

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

Diff Detail

Event Timeline

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

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

clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp
114

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
117

I am not sure what is going on here,

135

and here.

216

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

NoQ added inline comments.Aug 7 2019, 10:39 AM
clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp
114

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.Aug 7 2019, 10:47 AM
clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp
114

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.Aug 7 2019, 10:48 AM
clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp
114

Yeah, its value.

Charusso updated this revision to Diff 213952.Aug 7 2019, 11:07 AM
Charusso marked 5 inline comments as done.
  • Remove symbol-conjuring.
  • Add a forgotten test case.
clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp
114

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.Aug 7 2019, 1:53 PM
clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp
59

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

216–218

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);
226

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

234
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
211

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.Aug 7 2019, 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
59

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.

234

Hm, not bad. Thanks!

clang/test/Analysis/cast-value.cpp
211

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

Charusso updated this revision to Diff 214038.Aug 7 2019, 5:15 PM
  • Fix a comment.
Charusso updated this revision to Diff 214041.Aug 7 2019, 5:27 PM
  • May it is better to also check for getAsCXXRecordDecl() for obtaining a class.
Charusso updated this revision to Diff 214186.Aug 8 2019, 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.Aug 8 2019, 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
26

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

59

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

221–222

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

NoQ added inline comments.Aug 8 2019, 1:08 PM
clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp
95

IsDynamicCast.

99–101

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

clang/test/Analysis/cast-value.cpp
165–176

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.Aug 8 2019, 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

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.

95

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

99–101

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

221–222

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
165–176

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.Aug 8 2019, 6:31 PM

Aha, ok!

clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp
26

{ Function, Method }?

clang/test/Analysis/cast-value.cpp
165–176

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.Aug 8 2019, 6:31 PM
Charusso updated this revision to Diff 214277.Aug 8 2019, 7:02 PM
Charusso marked 6 inline comments as done.
  • Better naming.
clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp
26

These are cool identifiers, thanks!

clang/test/Analysis/cast-value.cpp
165–176

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 TranscriptAug 8 2019, 7:25 PM