Page MenuHomePhabricator

[analyzer] Check the checker name, rather than the ProgramPointTag when silencing a checker
ClosedPublic

Authored by Szelethus on May 18 2021, 5:23 AM.

Details

Summary

The program point created by the checker, even if it is an error node, might not be the same as the name under which the report is emitted. In the picture shown below, two tags are placed, one after the call (registering a new symbol as dynamically allocated with the tag unix.DynamicMemoryModeling), and one after the end of the call (when the symbol is leaked with the tag MallocChecker : DeadSymbolsLeak):

Whether its a good idea to create error nodes where the tag and the checker name is different -- I suppose not really, but it doesn't bother me much yet. Could add an assert later down the line.

// Generate leak node.
ExplodedNode *N = C.getPredecessor();
if (!Errors.empty()) {
  // Would it be more elegant to create the emitting checkers tag here? Mind that this is a checker that houses many others.
  static CheckerProgramPointTag Tag("MallocChecker", "DeadSymbolsLeak");
  N = C.generateNonFatalErrorNode(C.getState(), &Tag);
  if (N) {
    for (SmallVectorImpl<SymbolRef>::iterator
         I = Errors.begin(), E = Errors.end(); I != E; ++I) {
      HandleLeak(*I, N, C);
    }
  }
}

Diff Detail

Event Timeline

Szelethus created this revision.May 18 2021, 5:23 AM
Szelethus requested review of this revision.May 18 2021, 5:23 AM
Szelethus edited the summary of this revision. (Show Details)May 18 2021, 5:50 AM
Szelethus set the repository for this revision to rG LLVM Github Monorepo.
Szelethus added a project: Restricted Project.

While I still see checker silencing as a solution to a problem that shouldn't exist, I'm unsure about the amount of tedious and invasive work required to make the underlying infrastructure elegant just for the sake of it being elegant. Most notably, checker silencing solves the problem where a checker finds fatal errors -- disabling such a checker also gets rid of the sink node, which makes the entire analysis behave differently. This is desirable, as explained by @a.sidorin in https://lists.llvm.org/pipermail/cfe-dev/2019-August/063135.html. Although I still have concerns about the user interface if we promoted checker silencing to be user facing, it might be worth investigating a bit further.

NoQ accepted this revision.May 18 2021, 10:21 AM

Oof. Accurate!

// Would it be more elegant to create the emitting checkers tag here? Mind that this is a checker that houses many others.

I think the checkers should have a right to attach custom tags to their nodes. This right guarantees that they can create multiple different nodes with the same state (eg., the error state) and point (eg., the error point). Not sure why would they need it but I guess in some cases it may be easier for the checker to generate an extra node than to pass existing node around, as the existing node may be next to impossible to retrieve given that addTransition() with the same state will simply return null if the node is already there. So, like, the same reason why generally we allow checkers to chain nodes instead of forcing them to make all changes in a single transition.

This revision is now accepted and ready to land.May 18 2021, 10:21 AM

Oh, I'm late to the party!
Glad to see you back @Szelethus !

manas added a subscriber: manas.May 19 2021, 8:45 AM