NoQ added a comment.
> - ... Implementation of isTracked on it's own is fairly simple. And we can
> have users of `isTracked` and `ExpressionHandler` at the same time. Maybe,
> once we migrate all uses, `ExpressionHandler` can be safely retired.
I think this sounds like a plan. More specifically, if we could maintain a
//map// of all currently tracked expressions inside the
`PathSensitiveBugReport` object, dynamically updated as tracking proceeds
(expressions added as they're tracked and removed as soon as they're no longer
necessary), with values in the map being arbitrary auxiliary information we
want to pass around (eg., an explanation of how to refer to the value in
notes), then all tracking machineries, regardless of how they're implemented,
could communicate to each other easily through that map.
>> I still think the solution that relies on well-defined end of tracking is
>> significantly more elegant. Which, again, could be implemented with note
>> tags: we could have a note tag that'd call a callback stuffed directly into
>> the bug report.
>
> I'm not sure I understood the last thing, can you please elaborate?
I was thinking of something like:
void NullDereferenceChecker::checkLocation(...) {
trackExpressionValue(
BR, Ex, /*completionHandler=*/ [](const ExplodedNode *N) {
if (N->getLocationContext()->inTopFrame())
BR.markInvalid();
}); // stashes completionHandler into the aforementioned tracking map
// inside BR for Ex.
}
void ExprEngine::processBranch(...) {
Bldr.addTransition(..., getNoteTag([](BugReport &BR) {
if (BR.isTracked(Condition)) {
// We've tracked a null pointer back to the assumption.
// Our job here is done. Look up the callback in the tracking map.
if (auto completionHandler = BR.getTrackingCompletionHandler(Condition))
(*completionHandler)();
}
}));
}
> - Why do we even need to introduce tracker and event handlers if we are going
> to hide/delete them in this plan? The trick is about making this whole plan
> manageable. One enormous refactoring is hard to plan and test. Big work can
> demotivate and drain if you don't see the end of it. I believe that we can
> make great things if we take a lot of baby steps.
I completely agree. Incremental development ftw! I guess I'll keep an eye on
whether we could convert some of your newly extracted handlers to note tags
immediately (but most will probably require the expression tracking map to be
implemented first).
>> This code could also be moved into a checker's `checkBranchCondition()` so
>> that to make it truly checker-specific. That would come at the cost of
>> creating a lot of redundant exploded nodes just for the sake of attaching
>> the tag that will probably never be used so this is rather bad, wouldn't
>> recommend.
>
> Why not model defensive checks in the checker, so we don't make undesired
> assumptions in the first place?
That's pretty difficult. We can't enter (or skip) a branch without recording
its respective condition. We can't continue analysis unless we enter (or skip)
the branch. Once the branch has exited, we can't get rid of the constraint yet
because we may encounter the same condition again and we're obliged to be
consistent. Even when we've exited the nested stack frame we're still obliged
to be consistent, unless we're also willing to completely drop the execution
path inside the function and replace it with some form of summary. Or, you
know, do summaries from the start.
----
Ok, I think I mostly understand where this is going, thanks!
I'll now dive into code.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D103605/new/
https://reviews.llvm.org/D103605
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
