NagyDonat wrote: > Yeah my point is that these specific results aren't allowed to become > obsolete. > > The argument value is looked up from the Environment. A value of an > expression in Environment shall not change for as long as the expression > remains live. [...] Even if the callee code assigns values directly to the > parameter variable, that'd only go into the Store, so the Environment in the > new state would still correctly reflect the argument value from before the > call, the same one as the old state.
Thanks for clarifying this! I already suspected that something similar could be true and justify the use of old (but not too old) `State`s, but I wasn't confident enough to rely on this. > I hope that the same is true for all these actually-somewhat-irreplaceable > methods provided by `CallEvent`. If not, we'll need to think of a different > solution. But things like `getArgSVal()` simply don't get obsolete so easily. > They're not allowed to. I examined all code that relies on `CallEvent::getState()` and I found the following applications: - It is used by several code fragments (including `CallEvent::getSVal` which is used by `CallEvent::getArgSVal` and other similar functions) to call the method `ProgramState::getSVal(const Stmt*, const LocationContext*)`, which queries the `Environment` within the state. (Fortunately I found no references to the overloads like `ProgramState::getSVal(const MemRegion*, ...)` that query the `RegionStore`.) - (Tangentially related remark: it's a bit annoying that `ProgramState::getSVal` has overloads that get `SVal`s from very different sources -- reading code that uses them would be easier if they had different names.) - It is passed to `ExprEngine::getObjectUnderConstruction(...)` and `ExprEngine::computeObjectUnderConstruction(...)` in a few cases. - There are several calls like `getState()->getStateManager().getContext().getSourceManager()` or `getState()->getStateManager().getOwningEngine()` or `getState()->getStateManager().getSValBuilder()` that access the `ProgramStateManager`, the `ASTContext`, the `SourceManager`, the `ExprEngine` or the `SValBuilder` through the state instance attached to the `CallEvent`. This is awkward and a bit counter-intuitive, but seems to be harmless, as AFAIK these are all (essentially) global singleton objects. - This also happens in checkers, sometimes in locations where the `CheckerContext` is not available, but the `CallEvent` can be used to access the `ASTContext`. (If we make `CallEvent::getState()` protected, we will perhaps need to expose a public `CallEvent::getASTContext()`.) - (Tangentially related idea: perhaps we should introduce more straightforward ways to access these utility objects.) - A few checkers (`CheckObjCDealloc.cpp`, `StdVariantChecker.cpp`, `TaggedUnionModeling.h`) use the state attached to the `CallEvent` to do arbitrary operations (e.g. `->assume()`), these should be replaced by use of the state from the `CheckerContext` (which is also available at all of these call sites). - The method `CallEvent::invalidateRegions(unsigned BlockCount, ProgramStateRef Orig)` starts with `Result = (Orig ? Orig : getState())` (before updating the variable `Result`) -- that is, if the state passed as an argument is null (the default value), then defaults to using its own attached state. Unfortunately this method seems to be called in several locations (e.g. in `ExprEngine::VisitCXXNewExpr`) and can leak the state of the `CallEvent`. - I strongly suspect that here the right decision is to eliminate this "by default, use the state attached to the call event" shortcut and ensure that all call sites explicitly pass a `State` (which they should have). - 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. - Similar use of dynamic type info also happens in `ObjCMethodCall::getRuntimeDefinition()` - The method `ObjCMethodCall::getExtraInvalidatedValues` calls `getState()->getLValue(PropIvar, getReceiverSVal())` which looks up a value from the region store IIUC. I'm not familiar with handling of Objective C, so I cannot form an opinion about this method -- my wild guess is that it should explicitly take a `State` argument. - Similar calls also happen in `ObjCMethodCall::getReceiverSVal()`, `ObjCMethodCall::isReceiverSelfOrSuper()` and `ObjCMethodCall::getRuntimeDefinition()`. > So, like, do you know whether this assertion would fail without your patch? > If you stuff it into the old code and then analyze a bunch of code, would it > crash a lot? I don't have a strong opinion, but my wild guess is that without my patch this assertion would cause a significant amount of crashes in the EvalCall and PointerEscape callbacks (if the assert was added so that applies to these callbacks). > Maybe it makes sense to only include the Environment with the `CallEvent`, > not the entire State. That should prevent it from rotting. I think it is OK to keep the whole state as a private data member as long as it is marked with `MAY BE SLIGHTLY OBSOLETE!` comments (within the implementation of `CallEvent`) and the public interface of `CallEvent` only exposes parts (e.g. the `Environment`) that are stable enough. We should still pay some attention to keeping the state of the `CallEvent`s reasonably up-to-date on a best effort basis -- just to be safe -- but if we limit the use of the state attached to the call event, then this is no longer a bugprone area where we need to be extra careful. I feel that this pragmatic approach would be more practical than a principled solution that reimplements a "stable" subset of the state and perhaps duplicates some logic to do so. > Objects Under Construction which is basically an Environment for a > particularly spicy part of the AST I really like this description :joy: :hot_pepper: https://github.com/llvm/llvm-project/pull/160707 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
