ASDenysPetrov marked 3 inline comments as done. ASDenysPetrov added a comment.
@NoQ You concerns are absolutly justified. I agree with you. Let me tell what I'm thinking in inlines. If you are interested in why the assertion happens, please, look D77062#1977613 <https://reviews.llvm.org/D77062#1977613> ================ Comment at: clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp:273 Optional<DefinedSVal> val = V.getAs<DefinedSVal>(); - if (!val) - return std::pair<ProgramStateRef , ProgramStateRef >(state, state); + if (val && !V.getAs<nonloc::LazyCompoundVal>()) { + // return pair shall be {null, non-null} so reorder states ---------------- NoQ wrote: > Basically `SVal::getAs<>` should not be used for discovering the type of the > value; only the exact representation that's used for the value of the type > that's already in your possession. Say, it's ok to use it to discriminate > between, say, compound value and lazy compound value. It's ok to use it to > discriminate between a concrete integer and a symbol. It's ok to use it do > discriminate between a known value and an unknown value. But if it's used for > discriminating between a compound value and a numeric symbol, i'm 99% sure > it's incorrect. You should already know the type from the AST before you even > obtain the value. It doesn't make sense to run the checker at all if the > function receives a structure. And if it doesn't receive the structure but > the run-time value is of structure type, then either the checker isn't > obtaining the value correctly or there's bug in path-sensitive analysis. > That's why i still believe you're only treating the symptoms. There's nothing > normal in the situation where "strcpy suddenly accepts a structure (or an > array) by value". I'll remove `V.getAs<nonloc::LazyCompoundVal>()`. I mistakenly added this `V.getAs<nonloc::LazyCompoundVal>()` to keep code safe regardless of the checker intention and sanity. The point is that `state->assume(*val)` calls `ConstraintMgr->assumeDual` which finally calls `SimpleConstraintManager::assumeAux`. `assumeAux` handles all NonLoc kinds except **LazyCompoundValKind**and **CompoundValKind**(I missed to check it). In case of these two kinds it leads to `llvm_unreachable("'Assume' not implemented for this NonLoc");`. But for now I understand that I failed because **unreachable** means I might not have check for those kinds, since the function already takes on this work. However it is not an actual fix. The fix is `std::tie(states.second, states.first) = state->assume(*val);`. You can see the summary above. ================ Comment at: clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp:2270-2278 // Pro-actively check that argument types are safe to do arithmetic upon. // We do not want to crash if someone accidentally passes a structure // into, say, a C++ overload of any of these functions. We could not check // that for std::copy because they may have arguments of other types. for (auto I : CE->arguments()) { QualType T = I->getType(); if (!T->isIntegralOrEnumerationType() && !T->isPointerType()) ---------------- This check prevents passing **structures**. From this point we are sure to work with **pointers **and **integral **types in arguments: E.g. `char *strcpy(int i1, int i2);`, `char *strcpy(char *c1, char *c2);`, `char *strcpy(Kind t1, Kind t2);` This confirms @NoQ's supposition that checker narrows argument types. P.S. Honestly I'd narrow this more aggressively to just a **char pointer **type, anyway it doesn't relate to the bug and I wouldn't add this to a single patch. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77062/new/ https://reviews.llvm.org/D77062 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
