[NFCI][analyzer] Make CallEvent::getState protected #162673
Conversation
`CallEvent` instances have a reference to a state object instead having separate data members for storing the arguments (as `SVal` instances), the return value (as `SVal`, if available), the dynamic type information and similar things. Previously this state was publicly available, which meant that many checker callbacks had two ways to access the state: either through the `CallEvent` or through the `CheckerContext`. This redundancy is inelegant and bugprone (e.g. the recent commit llvm#160707 fixed a situation where the state attached to the `CallEvent` could be obsolete in `EvalCall` and `PointerEscape` callbacks), so this commit limits access to the state attached to a `CallEvent` and turns it into a protected implementation detail. In the future it may be a good idea to completely remove the state instance from the `CallEvent` and explicitly store the few parts of the state which are relevant for the call. In theory this commit should be a non-functional change (because AFAIK the `CallEvent` and `CheckerContext` provide the same state after the recent fix), but there is a small chance that it fixes some bugs that I do not know about.
|
This is intended as a follow-up commit after #160707 and was inspired by ideas within the discussion of that PR. |
llvmbot
added
clang
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-static-analyzer-1 Author: Donát Nagy (NagyDonat) Changes
Previously this state was publicly available, which meant that many checker callbacks had two ways to access the state: either through the In the future it may be a good idea to completely remove the state instance from the In theory this commit should be a non-functional change (because AFAIK the Full diff: https://github.com/llvm/llvm-project/pull/162673.diff 6 Files Affected:
diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
index 5dcf03f7a4648..2b082d07da71a 100644
--- a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
+++ b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
@@ -211,6 +211,16 @@ class CallEvent {
getExtraInvalidatedValues(ValueList &Values,
RegionAndSymbolInvalidationTraits *ETraits) const {}
+ /// The state in which the call is being evaluated.
+ /// CallEvent instances have a reference to a state object instead of storing
+ /// the argument `SVal`s, the return value (if available), the dynamic type
+ /// information and similar things as separate data members. However, the
+ /// existence of this state object is an implementation detail (and perhaps
+ /// it should be eventually eliminated), so use of this method should be
+ /// avoided if possible. (The checker callbacks can and should access the
+ /// canonical state through the CheckerContext.)
+ const ProgramStateRef &getState() const { return State; }
+
public:
CallEvent &operator=(const CallEvent &) = delete;
virtual ~CallEvent() = default;
@@ -231,8 +241,14 @@ class CallEvent {
}
void setForeign(bool B) const { Foreign = B; }
- /// The state in which the call is being evaluated.
- const ProgramStateRef &getState() const { return State; }
+ /// Returns the ASTContext which is (indirectly) associated with this call
+ /// event. This method is exposed for the convenience of a few checkers that
+ /// need the AST context in functions that take a CallEvent as argument. For
+ /// the sake of clarity prefer to get the ASTContext from more "natural"
+ /// sources (e.g. the CheckerContext) instead of using this method!
+ ASTContext &getASTContext() const {
+ return getState()->getStateManager().getContext();
+ }
/// The context in which the call is being evaluated.
const LocationContext *getLocationContext() const { return LCtx; }
diff --git a/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
index bf35bee70870b..3ddd6590fcbb0 100644
--- a/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
@@ -104,7 +104,7 @@ class RAIIMutexDescriptor {
// this function is called instead of early returning it. To avoid this, a
// bool variable (IdentifierInfoInitialized) is used and the function will
// be run only once.
- const auto &ASTCtx = Call.getState()->getStateManager().getContext();
+ const auto &ASTCtx = Call.getASTContext();
Guard = &ASTCtx.Idents.get(GuardName);
}
}
diff --git a/clang/lib/StaticAnalyzer/Checkers/CheckObjCDealloc.cpp b/clang/lib/StaticAnalyzer/Checkers/CheckObjCDealloc.cpp
index 9d3aeff465ca1..242084876a3c5 100644
--- a/clang/lib/StaticAnalyzer/Checkers/CheckObjCDealloc.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/CheckObjCDealloc.cpp
@@ -929,7 +929,7 @@ ObjCDeallocChecker::getValueReleasedByNillingOut(const ObjCMethodCall &M,
SVal Arg = M.getArgSVal(0);
ProgramStateRef notNilState, nilState;
std::tie(notNilState, nilState) =
- M.getState()->assume(Arg.castAs<DefinedOrUnknownSVal>());
+ C.getState()->assume(Arg.castAs<DefinedOrUnknownSVal>());
if (!(nilState && !notNilState))
return nullptr;
diff --git a/clang/lib/StaticAnalyzer/Checkers/ObjCSuperDeallocChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/ObjCSuperDeallocChecker.cpp
index f984caf59afb8..c1550f28f6f27 100644
--- a/clang/lib/StaticAnalyzer/Checkers/ObjCSuperDeallocChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/ObjCSuperDeallocChecker.cpp
@@ -230,7 +230,7 @@ ObjCSuperDeallocChecker::isSuperDeallocMessage(const ObjCMethodCall &M) const {
if (M.getOriginExpr()->getReceiverKind() != ObjCMessageExpr::SuperInstance)
return false;
- ASTContext &Ctx = M.getState()->getStateManager().getContext();
+ ASTContext &Ctx = M.getASTContext();
initIdentifierInfoAndSelectors(Ctx);
return M.getSelector() == SELdealloc;
diff --git a/clang/lib/StaticAnalyzer/Checkers/StdVariantChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StdVariantChecker.cpp
index 4fc1c576a9687..db8bbee8761d5 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StdVariantChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StdVariantChecker.cpp
@@ -211,13 +211,13 @@ class StdVariantChecker : public Checker<eval::Call, check::RegionChanges> {
if (!DefaultType)
return;
- ProgramStateRef State = ConstructorCall->getState();
+ ProgramStateRef State = C.getState();
State = State->set<VariantHeldTypeMap>(ThisMemRegion, *DefaultType);
C.addTransition(State);
}
bool handleStdGetCall(const CallEvent &Call, CheckerContext &C) const {
- ProgramStateRef State = Call.getState();
+ ProgramStateRef State = C.getState();
const auto &ArgType = Call.getArgSVal(0)
.getType(C.getASTContext())
diff --git a/clang/lib/StaticAnalyzer/Checkers/TaggedUnionModeling.h b/clang/lib/StaticAnalyzer/Checkers/TaggedUnionModeling.h
index dec461296fed5..b8fb57213fd65 100644
--- a/clang/lib/StaticAnalyzer/Checkers/TaggedUnionModeling.h
+++ b/clang/lib/StaticAnalyzer/Checkers/TaggedUnionModeling.h
@@ -52,7 +52,7 @@ removeInformationStoredForDeadInstances(const CallEvent &Call,
template <class TypeMap>
void handleConstructorAndAssignment(const CallEvent &Call, CheckerContext &C,
SVal ThisSVal) {
- ProgramStateRef State = Call.getState();
+ ProgramStateRef State = C.getState();
if (!State)
return;
|
| /// Returns the ASTContext which is (indirectly) associated with this call | ||
| /// event. This method is exposed for the convenience of a few checkers that | ||
| /// need the AST context in functions that take a CallEvent as argument. For | ||
| /// the sake of clarity prefer to get the ASTContext from more "natural" | ||
| /// sources (e.g. the CheckerContext) instead of using this method! |
There was a problem hiding this comment.
Why do you think preferring the ASTContext of the CheckerContext is better?
I was expecting no comments at this member function.
There was a problem hiding this comment.
Why do you think preferring the ASTContext of the CheckerContext is better?
Because the fact that CallEvent can easily provide an ASTContext is just an accidental consequence of the current implementation. This is not a natural role for a call event object, and in fact, there are vague plans to avoid storing a state within the CallEvent (suggested at #160707 (comment)) and in that case we would probably eliminate this method.
| /// The state in which the call is being evaluated. | ||
| /// CallEvent instances have a reference to a state object instead of storing | ||
| /// the argument `SVal`s, the return value (if available), the dynamic type | ||
| /// information and similar things as separate data members. However, the | ||
| /// existence of this state object is an implementation detail (and perhaps | ||
| /// it should be eventually eliminated), so use of this method should be | ||
| /// avoided if possible. (The checker callbacks can and should access the | ||
| /// canonical state through the CheckerContext.) | ||
| const ProgramStateRef &getState() const { return State; } |
There was a problem hiding this comment.
By reading the comment it was not clear to me why it returns a state by const ref.
State objects can never be mutated. Only a new one could be constructed that applies the mutation one desired.
BTW I also feel like a comment should fit into a single line, or maybe just a couple of lines, if the intention is that someone should read it.
If the message can't be expressed in that limited space, it must be chunked, to make it easier to read. Possibly also use Doxygen note or similar sections.
There was a problem hiding this comment.
By reading the comment it was not clear to me why it returns a state by const ref.
State objects can never be mutated. Only a new one could be constructed that applies the mutation one desired.
I just moved this method from the public section to protected without modifying its signature. You're right that there is no reason to take the state by (const) ref, so I'll update it.
BTW I also feel like a comment should fit into a single line, or maybe just a couple of lines, if the intention is that someone should read it.
I'll try to reduce the length of the comment -- although personally I read and often prefer long and explicit comments.
There was a problem hiding this comment.
I see. So the discouragement is to avoid more dependence on using the State. IMO. if that is a concern, we could easily swap that State for a reference to the ASTContext if we decide to drop that State field.
To me, the comment does not pull its weight, and probably confusing (just like it did confuse me).
Probably any comments about the State filed should go to the field declaration, and not to this getter.
There was a problem hiding this comment.
I see. So the discouragement is to avoid more dependence on using the State. IMO. if that is a concern, we could easily swap that State for a reference to the ASTContext if we decide to drop that State field. To me, the comment does not pull its weight, and probably confusing (just like it did confuse me).
Is this still relevant after my commit e0d10ba or did I manage to clarify it?
You are right that "we could easily swap that State for a reference to the ASTContext if we decide to drop that State field" and therefore we don't strictly need the discouragement before getASTContext() [which is btw the other big comment block] -- but I still feel that "can provide an ASTContext" is not a "natural" responsibility for a call event object, so I would prefer to keep this discouragement.
Probably any comments about the State filed should go to the field declaration, and not to this getter.
The warning which highlights the use of the state object belongs to the getter, because a developer who wants to use getState() perhaps looks at its declaration and sees the warning, but almost surely won't look at the field declaration. (Although perhaps we should rename getState to getStateUnsafe to highlight that there is something fishy going on, because otherwise developers happily autocomplete getState without seeing the warning comment.)
You're right that the TODO note for replacing the State field with smaller individual fields would logically belong to the field declaration, but it is also closely related to the warning comment, so I don't want to separate it from there.
clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
Outdated
Show resolved
Hide resolved
|
The commit "Shorten and clarify comments" also changes the return type of |
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
Outdated
Show resolved
Hide resolved
Co-authored-by: Artem Dergachev <[email protected]>
|
The CI failure is irrelevant: lldb-unit :: DAP/./DAPTests/DisconnectRequestHandlerTest/DisconnectTriggersTerminateCommands is completely unrelated to this commit. |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/186/builds/13285 Here is the relevant piece of the build log for the reference |
|
The CI failure is apparently caused by a buggy buildbot:
|
5 participants
CallEventinstances have a reference to a state object instead having separate data members for storing the arguments (asSValinstances), the return value (asSVal, if available), the dynamic type information and similar things.Previously this state was publicly available, which meant that many checker callbacks had two ways to access the state: either through the
CallEventor through theCheckerContext. This redundancy is inelegant and bugprone (e.g. the recent commit 6420da6 fixed a situation where the state attached to theCallEventcould be obsolete inEvalCallandPointerEscapecallbacks), so this commit limits access to the state attached to aCallEventand turns it into a protected implementation detail.In the future it may be a good idea to completely remove the state instance from the
CallEventand explicitly store the few parts of the state which are relevant for the call and do not change during the evaluation of the call.In theory this commit should be a non-functional change (because AFAIK the
CallEventandCheckerContextprovide the same state after the recent fix), but there is a small chance that it fixes some bugs that I do not know about.