More checker callbacks should have `CheckerContext`

Most checker callbacks have CheckerContext, from which the checker can create ExplodedNodes and emit diagnostics. Great stuff.

However, there are a couple of callbacks which doesn’t. This breaks the symmetry along the rest of the callbacks and makes it impossible to emit diagnostics from those.

Are there any design decisions why these callbacks don’t have CheckerContext?

I’m considering extending the following checker callbacks with an additional CheckerContext parameter:

  • checkPointerEscape
  • checkConstPointerEscape
  • checkRegionChanges
  • checkLiveSymbols
  • evalAssume (*)

The rest of the callbacks are not relevant, because they are AST-based, to not possible to construct a CheckerContext:

  • checkEndAnalysis
  • checkEndOfTranslationUnit
  • checkASTDecl
  • checkEvent (I’m not even sure what this is used for xD)

(*): It’s tempting to have CheckerContext for evalAssume, but it would require a significant amount to work to make it happen. State->assume() should also pass this along the whole ConstraintManager infrastructure, making such change quite challenging to do incrementally.

@NoQ @martong @Xazax-hun @ASDenysPetrov @balazske @Szelethus

These callbacks don’t have a CheckerContext because they can be (and often are) invoked in the middle of another checker callback. It’s very problematic if another checker builds a node in the middle of your own callback and you no longer know what’s your predecessor node to build upon.

checkLiveSymbols is different: it can’t be invoked inside another checker callback but I suspect it’s still problematic for our liveness code if the current node (or even the current state) changes in the middle of a garbage collection pass.

I see. If this is the case, I’m not sure if we can do anything to make regionChanges possible to report diagnostics.

What diagnostics do you want to emit from checkRegionChanges that you can’t emit from checkBind (if it’s about a bind) or checkPostCall (if it’s about invalidation caused by a call)?

I dont have anything specific for this, but I can remember that it would have been handy in the past.

This thread was more like theoretical than practical.

However, in the case of evalAssume, I wanted a solution for the upcoming Errno checker. We wanted to restrict comparing the value of errno to anything if the value is indeterminate.
While allowing code making copy of errno and restoring the errno to the copied value. (Think about some graceful resource release in case something bad happens.) By using evalAssume we vould have checked if the symbolic errno value takes part of some condition or not, and allow making copies and everything else. So this was the primary motivation for this question, but we settled on check::Location.

I was juet curious, and I suspect there might be other check ideas which could possibly employ something I layed out.

checkBranchCondition was supposed to be the evalAssume's counterpart for emitting bug reports. Uninitialized variable checker already uses it. I agree that naming could have been better.

1 Like