[PATCH] D86445: [analyzer][RFC] Simplify MetadataSymbol representation and resolve a CStringChecker FIXME
Balázs Benics via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Tue Sep 8 07:45:20 PDT 2020
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
More information about the cfe-commits
mailing list