steakhal added a comment. In D86445#2260744 <https://reviews.llvm.org/D86445#2260744>, @martong wrote:
>> But if the string is invalidated (or the new length is completely unknown), >> **do not remove** the length from the state; instead, set it to >> SymbolConjured. > > Where exactly do you implement the above? > >> When strlen(R) is used for the first time on a region R, produce >> `$meta<size_t R>` as the answer, but *do not store* it in the string length >> map. > > And this? Hm yes. I missed them. I will extend the current implementation, rethinking the invalidation cases, when to store and what etc. It will bloat this patch, but indeed - necessary. Thank you! However, as I planned to clean the CStringChecker a little bit up, it seems reasonable to me to land those patches before I start working on this to save me from some rebasing :| What do you think about these patches: - D84316 <https://reviews.llvm.org/D84316> [analyzer][NFC] Split CStringChecker to modeling and reporting - D84979 <https://reviews.llvm.org/D84979> [analyzer][NFC] Refine CStringLength modeling API If you like that direction, I can rebase this on top of that. That would help me a lot, to know when & how a given metadata symbol would be used, and check if we indeed handle the invalidation, etc. in the right way. ================ Comment at: clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp:2379 + // Overwrite the associated cstring length values of the invalidated regions. + for (const auto &Entry : Entries) { + const MemRegion *MR = Entry.first; ---------------- martong wrote: > It's just a tiny nit. But, perhaps we could come up with a more meaningful > name instead of `Entry` and `Entries`. Maybe `Length` ? We will see. Thanks. ================ Comment at: clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp:2384-2385 if (SuperRegions.count(MR)) { Entries = F.remove(Entries, MR); + Entries = F.add(Entries, MR, UnknownVal()); continue; ---------------- martong wrote: > martong wrote: > > Umm, these two lines are quite disturbing after each other. Seems like > > first we remove `MR` then we add `MR` again, so, this must not be a noop, > > right? But then what is happening here exactly? Some comments around here > > in the code would help. > Ugh, giving it more thought, we just reset the Value associated to `MR`. > Still, would be nice to comment this there. Exactly. I will do that, ok. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D86445/new/ https://reviews.llvm.org/D86445 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
