[PATCH] D64374: [analyzer] CastValueChecker: Model casts

Charusso added a comment. In D64374#1594617 , @Szelethus wrote: > I guess `getAs` and `castAs` methods could join the party too. I'm evaluating > plenty of results on LLVM, and those seem to be in the same problem category. In D64374#1618694

[PATCH] D64374: [analyzer] CastValueChecker: Model casts

Szelethus added a comment. Do you have plans to extend this checker with the modeling of `isa<>`? I might take that off your shoulder if not :) CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64374/new/ https://reviews.llvm.org/D64374 ___ cf

[PATCH] D64374: [analyzer] CastValueChecker: Model casts

Szelethus added a comment. I guess `getAs` and `castAs` methods could join the party too. I'm evaluating plenty of results on LLVM, and those seem to be in the same problem category. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64374/new/ https://reviews.llvm.org/D64374 ___

[PATCH] D64374: [analyzer] CastValueChecker: Model casts

Charusso updated this revision to Diff 208847. Charusso added a comment. - Add the llvm namespace to the test file. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64374/new/ https://reviews.llvm.org/D64374 Files: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td clang/include/

[PATCH] D64374: [analyzer] CastValueChecker: Model casts

This revision was automatically updated to reflect the committed changes. Closed by commit rL365582: [analyzer] CastValueChecker: Model casts (authored by Charusso, committed by ). Herald added a project: LLVM. Herald added a subscriber: llvm-commits. Changed prior to commit: https://reviews.ll

[PATCH] D64374: [analyzer] CastValueChecker: Model casts

Charusso added a comment. Thanks for the review! In D64374#1577279 , @NoQ wrote: > That doesn't sound like it's high on our list. That's not too many times. > Probably very few false positives come out of it. Also modeling smart > pointers is much harde

[PATCH] D64374: [analyzer] CastValueChecker: Model casts

NoQ added a comment. Mmm. Ok, am i understanding correctly that this was crashing because you bound a `0 (Loc)` to a prvalue expression of type `unique_ptr`? Yeah, right, don't do that :) > used like 20 times in the LLVM codebase That doesn't sound like it's high on our list. That's not too ma

[PATCH] D64374: [analyzer] CastValueChecker: Model casts

Charusso added a comment. In D64374#1577266 , @NoQ wrote: > Can you provide more info, eg. the full backtrace? Well, `unique_dyn_cast<>` and `unique_dyn_cast_or_null<>` is used like 20 times in the LLVM codebase, whoops. We want to model it. Full info:

[PATCH] D64374: [analyzer] CastValueChecker: Model casts

NoQ added a comment. In D64374#1577262 , @Charusso wrote: > - Add the forgotten `llvm` namespace to the `CallDescription`. Nice! In D64374#1577254 , @Charusso wrote: > The only crash-able code was that: > > #0

[PATCH] D64374: [analyzer] CastValueChecker: Model casts

Charusso updated this revision to Diff 208831. Charusso added a comment. - Add the forgotten `llvm` namespace to the `CallDescription`. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64374/new/ https://reviews.llvm.org/D64374 Files: clang/include/clang/StaticAnalyzer/Checkers/Checkers

[PATCH] D64374: [analyzer] CastValueChecker: Model casts

Charusso added a comment. In D64374#1577235 , @NoQ wrote: > Whoops. I underestimated you (: > > Ok, anyway, this was my last comment. Great job! Remember to make sure that > it works on the actual LLVM, not only on tests; mocked-up test headers are > ver

[PATCH] D64374: [analyzer] CastValueChecker: Model casts

NoQ accepted this revision. NoQ added a comment. This revision is now accepted and ready to land. Whoops. I underestimated you (: Ok, anyway, this was my last comment. Great job! Remember to make sure that it works on the actual LLVM, not only on tests; mocked-up test headers are very easy to g

[PATCH] D64374: [analyzer] CastValueChecker: Model casts

Charusso marked 2 inline comments as done. Charusso added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp:172 + !CE->getArg(0)->getType()->getPointeeCXXRecordDecl()) +return false; + If we cannot obtain any of the CXXR

[PATCH] D64374: [analyzer] CastValueChecker: Model casts

NoQ added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp:58 +static std::string getCastName(const Expr *Cast) { + return Cast->getType()->getPointeeCXXRecordDecl()->getNameAsString(); +} You should still check if `Cast->getTy

[PATCH] D64374: [analyzer] CastValueChecker: Model casts

Charusso updated this revision to Diff 208824. Charusso marked 6 inline comments as done. Charusso added a comment. - Move to `apiModeling.llvm`. - Prevent unknown casts. - Refactor. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64374/new/ https://reviews.llvm.org/D64374 Files: clang

[PATCH] D64374: [analyzer] CastValueChecker: Model casts

NoQ added inline comments. Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:101-102 // intended for API modeling that is not controlled by the target triple. def APIModeling : Package<"apiModeling">, Hidden; def GoogleAPIModeling : Package<"google">, ParentP

[PATCH] D64374: [analyzer] CastValueChecker: Model casts

Szelethus added inline comments. Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:279 + HelpText<"Model implementation of custom RTTIs">, + Documentation; + This checker only does modeling, but isn't hidden. Should we hide it? CHANGES SINCE

[PATCH] D64374: [analyzer] CastValueChecker: Model casts

Charusso updated this revision to Diff 208588. Charusso added a comment. - Simplify the new `getNoteTag()`. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64374/new/ https://reviews.llvm.org/D64374 Files: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td clang/include/clang/St

[PATCH] D64374: [analyzer] CastValueChecker: Model casts

Charusso added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp:61 + + return Ty.getAsString(); +} NoQ wrote: > Use after free! `QualType::getAsString()` returns a temporary `std::string`. > You're returning a `StringRef` that

[PATCH] D64374: [analyzer] CastValueChecker: Model casts

Charusso updated this revision to Diff 208586. Charusso marked 9 inline comments as done. Charusso added a comment. - Fix. - More tests. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64374/new/ https://reviews.llvm.org/D64374 Files: clang/include/clang/StaticAnalyzer/Checkers/Checker

[PATCH] D64374: [analyzer] CastValueChecker: Model casts

NoQ added a subscriber: rnkovacs. NoQ added a comment. We should add at least some tests for the other three functions. Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:278 +def CastValueChecker : Checker<"CastValue">, + HelpText<"Model casts">, + Documenta

[PATCH] D64374: [analyzer] CastValueChecker: Model casts

Charusso updated this revision to Diff 208574. Charusso added a comment. - Fix a typo. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64374/new/ https://reviews.llvm.org/D64374 Files: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td clang/include/clang/StaticAnalyzer/Core/Pat

[PATCH] D64374: [analyzer] CastValueChecker: Model casts

Charusso added a comment. Thanks! My mind was really set to actually model these with `classof()`, whoops. Comment at: clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp:25-27 + using CastCheck = + std::function; NoQ wrote: > Kinda nice, but a simple

[PATCH] D64374: [analyzer] CastValueChecker: Model casts

Charusso updated this revision to Diff 208573. Charusso marked 12 inline comments as done. Charusso added a comment. - Fix - New `getNoteTag()` which accepts a plain note. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64374/new/ https://reviews.llvm.org/D64374 Files: clang/include/cl

[PATCH] D64374: [analyzer] CastValueChecker: Model casts

NoQ added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp:124 + +Out << "Cast from a null pointer results in a null pointer"; +return Out.str(); `Assuming null pointer is passed into cast` o.o Repository: rC

[PATCH] D64374: [analyzer] CastValueChecker: Model casts

NoQ added a comment. The overall code looks great! It's well-structured and easy to understand, thanks! Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64374/new/ https://reviews.llvm.org/D64374 ___ cfe-commits mailin

[PATCH] D64374: [analyzer] CastValueChecker: Model casts

NoQ added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp:25-27 + using CastCheck = + std::function; Kinda nice, but a simple pointer-to-member would have been sufficient and much more lightweight: ```lang=c++ using Cas

[PATCH] D64374: [analyzer] CastValueChecker: Model casts

Charusso marked an inline comment as done. Charusso added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp:175 + else*/ +Param = CE->getArg(0); + This CXXMemberCallExpr business is necessary? I am not sure as I have not see

[PATCH] D64374: [analyzer] CastValueChecker: Model casts

Charusso created this revision. Charusso added reviewers: NoQ, ravikandhadai. Charusso added a project: clang. Herald added subscribers: cfe-commits, dkrupp, donat.nagy, Szelethus, mikhail.ramalho, a.sidorin, szepet, baloghadamsoftware, xazax.hun, mgorny. It models the LLVM casts: - `cast<>` - `