[RFC] Creating base class for 'Test after X' checkers

Hello!

Recently a TestAfterDivZero checker was added to the static analyser (r212731).

We could add many related checkers, for example:

  • test after null pointer dereference
  • test after array index out of bounds
  • test after dangerous/invalid function argument

I wonder what you think about creating generic base classes for TestAfterX checkers. Would you prefer copy/paste?

I attach a proof of concept patch. It adds a include file llvm/tools/clang/include/StaticAnalyzer/Core/TestAfterChecker.h. Is that the proper path where it should be or would it be better somewhere else? There is no implementation but I should add it in llvm/tools/clang/lib/StaticAnalyzer/Core/TestAfterChecker.cpp, shouldn’t I? Do you have an opinion about the namespace usage?

It might still be possible to move some more code from the checker to the base classes. The current code is work-in-progress.

Best regards,
Daniel Marjamäki

Daniel Marjamäki Senior Engineer

Evidente ES East AB Warfvinges väg 34 SE-112 51 Stockholm Sweden

Mobile: +46 (0)709 12 42 62

E-mail: Daniel.Marjamaki@evidente.se

www.evidente.se

Hi Daniel,

As Jordan mentioned earlier, I am concerned about making test after div-zero/dereference checkers rely on path sensitive analysis. These require all path reasoning and the analyzer does not guarantee to cover all paths (some of them might be silently dropped). My understanding is that you are working around this by performing checking within a single basic block; however, in that case relying on the power of path sensitivity is an overkill. I think it’s best to write these as non-path sensitive data flow analysis on the CFG. What do you think?

If we decided to go with path-sensitive checkers for this, addition of the base class would be the right way to go.

Hello!

Recently a TestAfterDivZero checker was added to the static analyser (r212731).

We could add many related checkers, for example:

  • test after null pointer dereference
  • test after array index out of bounds
  • test after dangerous/invalid function argument

I wonder what you think about creating generic base classes for TestAfterX checkers. Would you prefer copy/paste?

I attach a proof of concept patch. It adds a include file llvm/tools/clang/include/StaticAnalyzer/Core/TestAfterChecker.h.

This one should be added under include/clang/StaticAnalyzer/Checkers/. See ObjCRetainCount.h as an example.

Hello!

I want to have path-sensitive analysis. However I do not understood how it’s done effectively in Clang yet. It’s quite different to Cppcheck. :slight_smile:

My understanding is that you are working around this by performing checking within a single basic block

The problem is mostly that I don’t understand how CFG is used effectively. I don’t see how to properly handle inner/outer/same scope.

I think it’s best to write these as non-path sensitive data flow analysis on the CFG. What do you think?

sorry… could you be more specific. do you want to move the checker. do you want it to use a different visitor? do you want that we remove some logic?

best regards,
Daniel Marjamäki

Daniel Marjamäki Senior Engineer

Evidente ES East AB Warfvinges väg 34 SE-112 51 Stockholm Sweden

Mobile: +46 (0)709 12 42 62

E-mail: Daniel.Marjamaki@evidente.se

www.evidente.se

Hello!

I want to have path-sensitive analysis. However I do not understood how it’s done effectively in Clang yet. It’s quite different to Cppcheck. :slight_smile:

The static analyzer core generates exploded graph, which represents all paths through the program. As the graph is constructed, the analyzer calls to the path sensitive checkers and allows them to explore and augment the state. You can watch our presentation (http://llvm.org/devmtg/2012-11/#content) on how to build a checker to refresh on how this all plays together. We have some nice visualizations there.

The main problem with using the path sensitive analyzer engine here is that paths may be dropped and you are not guaranteed to explore all paths. This could lead to false positives in reverse null (test after X) checks. In order to work around this, you’ve limited the analysis to a single basic block (at least that’s what I understand from reading the review thread). However, if you are limited to a basic block, the advantage of path sensitive analysis is greatly diminished, and it’s not worth paying the performance price.

My understanding is that you are working around this by performing checking within a single basic block

The problem is mostly that I don’t understand how CFG is used effectively. I don’t see how to properly handle inner/outer/same scope.

I think it’s best to write these as non-path sensitive data flow analysis on the CFG. What do you think?

sorry… could you be more specific. do you want to move the checker. do you want it to use a different visitor? do you want that we remove some logic?

I am suggesting to write a data flow sensitive analysis (instead of path sensitive) for this particular set of problems. Flow sensitive analysis are much more lightweight; these are often used in compiler optimization algorithms. Currently, we do not have many of these. Live variable analysis would be one such example. Unfortunately, this would mean a rewrite of what you have now…