[PATCH] D84979: [analyzer][NFC] Refine CStringLength modeling API

Tue Aug 18 01:27:03 PDT 2020

steakhal added a comment.

In D84979#2190996 <https://reviews.llvm.org/D84979#2190996>, @balazske wrote:

> Really I am still not totally familiar how the checkers work and if it is good to have these function names.

Yea, naming things is pretty hard. I'm open to have a less verbose one - especially since these API functions will live under some namespace.

> [...]  It could get the data from CStringChecker using a "get" function or test if there is data at all. In this case it may happen that this get function modifies state or computes the length at that point?

This patch makes sure that the //get// function will not modify the state (takes a `const ProgramStateRef`).
We don't really want to store the length of a `StringLiteral`, since that can be computed on demand.

> And how do the //set// operation work, it only stores a value computed by the calling code?

It tries to avoid storing the length information. E.g. if a region refers to a string literal - we don't have to store the length. (This was the previous behavior, I don't want to change that.)
However, we don't handle ElementRegions (for now, but I'm working on it).

> And the //create// operation then does the computation and //set// together?

CStringChecker needs a way to defer associating the length of the region.
I'm not sure why, but the magic //Hypothetical// parameter was introduced just for accomplishing that.
I'm not confident enough to refactor the function which exploits this. [1]

> The functions look not symmetrical, the set and create takes a `MemRegion` but the get takes a `SVal`.

The current implementation checks if the SVal is a GotoLabel (which is not a MemRegion). I didn't want to change its behavior in this patch.
`MemRegion` parameter would fit much more nicely, just as you said.

> The create function sets the length for the passed memory region

No, it just creates a symbol, representing a cstring length. That symbol is (strictly speaking) not yet associated with the memory region.
Furthermore, it applies implicit constraints like assuming that the cstring length is always less than the corresponding extent (It is not yet done, but I'm working on this too).

> but makes no tests on it like the set function (can we call the set function from the create?).

We could call, but we need a mechanism to defer associating the length of a region.
CStringChecker relies on this behavior. See my paragraph about the //Hypothetical// parameter.

---

@balazske Unfortunately, I don't have satisfying answers to you, but any further improvements could be done in the future.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D84979/new/

https://reviews.llvm.org/D84979