[clang] [analyzer] Avoid use of `CallEvent`s with obsolete state (PR #160707)

[Donát Nagy via cfe-commits]

NagyDonat wrote:

> > `getState()->getStateManager().getContext()`
> 
> This is indeed a bit silly but legal. They could probably also do a `LocationContext->getAnalysisDeclContext()->getASTContext()` instead. You can probably give them a direct accessor method for `ASTContext`.

I'll do so if I have time.

> > The method `CXXInstanceCall::getDeclForDynamicType()` calls `getDynamicTypeInfo(getState(), R)` where `R = getCXXThisVal().getAsRegion()`. This seems to be a suspicious call where an obsolete state could perhaps cause problems (although I'm not sure about this -- I just presume that the dynamic type info can change dynamically). If needed, we can modify this method to take the State as an explicit parameter.
> 
> Yes, the dynamic type can change over time. It usually acts as a constraint, i.e. "what we know about the runtime type behind the pointer". Our knowledge can improve over time but it can't evolve in contradictory ways. (At least not until the program performs a placement-new or something of that nature.)
> 
> So this is one of the payoffs for tracking dynamic types. It helps us resolve virtual calls as part of `getRuntimeDefinition()`. The thing about `getRuntimeDefinition()` is that it's a complex piece of imperative code that arguably needs to be invoked exactly once per function call, by the engine, and then the value should probably be simply stored somewhere and communicated through other means. Eg., the chosen `LocationContext` is a good way to keep track of it.

Thanks for the information!

> Given that it'd be invoked exactly once and very carefully, the question of multiple out-of-sync states hopefully doesn't come up. Maybe it should be moved out of `CallEvent` to somewhere else, have the virtual part of it minimized somehow, so that it only extracted the relevant bits and pieces of information from the sub-class but didn't try to load stuff from the Store? And then the rest of the implementation could use `CallEvent` through its public interface?

I don't have a strong opinion about this, but your suggestions sound vaguely right.

> > Similar use of dynamic type info also happens in `ObjCMethodCall::getRuntimeDefinition()`
> 
> This one's a bit worse because ObjC `self`, unlike `this`, is an actual variable. It can be, and often is, overwritten in the middle of the method [...]

Eww...

> > The method `CallEvent::invalidateRegions(unsigned BlockCount, ProgramStateRef Orig)` starts with `Result = (Orig ? Orig : getState())`
> 
> Same story as `getRuntimeDefinition()` imho. I think this one needs to be invoked exactly once per function call. It's virtual because it picks up additional regions and/or per-region invalidation modes from subclasses. Maybe that's the only thing `CallEvent` should be doing, and the rest should be done by someone else.

I agree that it's reasonable to have this as a virtual method of the `CallEvent` class hieararcy. As it already takes the state as an optional parameter, I'd say that it would be straightforward to enusre that it always takes the state explicitly, which would ensure that it behaves cleanly.

> The slightly annoying part with this one is that it also needs to be invoked by some of the `evalCall` checkers when they're giving up on modeling the call precisely and need to model it conservatively instead. Maybe they should be given a different API that does exactly that and comes with a guarantee that it's exactly equivalent to conservative evaluation.

I agree that it would be probably useful to have a "do exactly the same thing as conservative evaluation" function.

-----

Based on this discussion, I'm planning the following improvements:

1. Within 1-2 weeks I'll finalize this commit:
  - I'll add tests (probably a new debug checker + a simple lit test that invokes it – I think this would be easier to implement than unit tests).
  - I'll fix the duplicated word typos.
2. After that I will probably create a PR that makes `CallEvent::getState()` ~~private~~ protected and exposes a `CallEvent::getASTContext()` utility method (because it's harmless and would be useful for the checker code that currently calls `getState()`).
3. Perhaps I'll also create a commit which ensures that `CallEvent::invalidateRegions()` always takes the state explicitly (instead of defaulting to the state within the event).

I'm not planning to work on the other topics (runtime definition, Objective-C trickery) because they seem to be more complex and I'm less familiar with those areas. I feel that picking the (above mentioned) low hanging ~~fruit~~ footguns will be sufficient to make this are as safe as other areas of the analyzer codebase.

https://github.com/llvm/llvm-project/pull/160707