[PATCH] D86445: [analyzer][RFC] Simplify MetadataSymbol representation and resolve a CStringChecker FIXME

[Balázs Benics via Phabricator via cfe-commits]

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