================
@@ -731,33 +731,36 @@ void
CheckerManager::runCheckersForEvalCall(ExplodedNodeSet &Dst,
ExplodedNodeSet checkDst;
NodeBuilder B(Pred, checkDst, Eng.getBuilderContext());
+ ProgramStateRef State = Pred->getState();
+ CallEventRef<> UpdatedCall = Call.cloneWithState(State);
+
// Check if any of the EvalCall callbacks can evaluate the call.
for (const auto &EvalCallChecker : EvalCallCheckers) {
// TODO: Support the situation when the call doesn't correspond
// to any Expr.
ProgramPoint L = ProgramPoint::getProgramPoint(
- Call.getOriginExpr(), ProgramPoint::PostStmtKind,
+ UpdatedCall->getOriginExpr(), ProgramPoint::PostStmtKind,
Pred->getLocationContext(), EvalCallChecker.Checker);
bool evaluated = false;
{ // CheckerContext generates transitions(populates checkDest) on
// destruction, so introduce the scope to make sure it gets properly
// populated.
CheckerContext C(B, Eng, Pred, L);
- evaluated = EvalCallChecker(Call, C);
+ evaluated = EvalCallChecker(*UpdatedCall, C);
}
#ifndef NDEBUG
if (evaluated && evaluatorChecker) {
- const auto toString = [](const CallEvent &Call) -> std::string {
+ const auto toString = [](CallEventRef<> Call) -> std::string {
std::string Buf;
llvm::raw_string_ostream OS(Buf);
- Call.dump(OS);
+ Call->dump(OS);
----------------
NagyDonat wrote:
> Why did you change this?
It is true that this dump (which IIUC just prints the function name) doesn't
rely on the `State` within the `CallEvent` object, so it would work with the
non-updated instance -- but as we needed to create the `UpdatedCall` (because
we need to pass that to the checkers), I thought that it would be better to
consistently use it.
I don't exactly recall why I changed the type of this lambda, I think the
compiler complained about my first attempt. I can try to avoid these changes
(`CallEventRef<>` is a somewhat esoteric smart pointer type, but it _should_ be
possible to get a `const CallEvent &` from it).
`<offtopic>`
By the way I feel an urge to get rid of this lambda, because it is much more
verbose than e.g.
```c++
std::string FunctionName;
Call.dump(llvm:raw_string_ostream(FunctionName));
```
(I don't see a reason why we would need to declare a variable for the stream
instead of just using a temporary object -- but I'd test this before applying
it.) However, I acknowledge that this is subjective bikeshedding, and I won't
remove the lambda if you prefer to keep it.
`</offtopic>`
> But generally, in the checker loop, the updated call event indicates that the
> call was evaluated by multiple checkers. Which should be impossible in
> practice but it looks like we've decided to act gracefully when assertions
> are turned off.
What do you mean by this? My understanding of this situation is that:
- When assertions are turned off (`#ifdef NDEBUG` block a few lines below the
displayed area) we `break` and leave the loop eagerly when one checker
evaluated the call.
- When assertions are turned on, we always iterate over all the checker
callback, and do this formatted error printout if a second checker callback
evaluated the call.
> That said, we aren't doing a great job at that, given that we're using
> `llvm_unreachable()` that translates to pure undefined behavior (aka
> `__builtin_unreachable()`) when assertions are turned off.
Good to know in general, but this particular `llvm_unreachable()` call is
within an `#ifndef NDEBUG` block so I would guess that it is completely skipped
when assertions are turned off. (Is this correct? What is the relationship
between assertions and `NDEBUG`?)
https://github.com/llvm/llvm-project/pull/160707
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
