Page MenuHomePhabricator

[Static Analyzer] Lambda support.
ClosedPublic

Authored by xazax.hun on Sep 4 2015, 3:49 PM.

Details

Summary

This patch adds lambda support to the static analyzer.
All of my initial tests are passed.

Before this patch each time a LambdaExpr was encountered a sink node was generated. This also reduced the coverage of code that is actually not inside a lambda operator(). Also the lack of lambda support is a known cause of some false positives (for example in the dead store checker).

This is a work in progress version of this patch, I will move this feature behind a flag and run it on LLVM to make sure it is also tested with real world code.

Diff Detail

Repository
rL LLVM

Event Timeline

xazax.hun updated this revision to Diff 34085.Sep 4 2015, 3:49 PM
xazax.hun retitled this revision from to [Static Analyzer] Lambda support..
xazax.hun updated this object.
xazax.hun updated this object.
xazax.hun added a subscriber: cfe-commits.
xazax.hun updated this revision to Diff 34285.Sep 8 2015, 5:21 PM
  • Updated to newest trunk.
  • Moved the feature behind an option.
  • Fixed a crash when an operator() of a lambda is analyzed as a top level function, and a ThisExpr is referring to the this in the enclosing scope (this can only happen when lambda support is turned off).
  • Added a new test case for nested lambdas capturing 'this'.
zaks.anna edited edge metadata.Sep 10 2015, 2:55 PM

Have you tested this on a large codebase that uses lambdas? When do you think we should turn this on by default?

Please, add test cases that demonstrate what happens when an issue is reported within a lambda and to check if inlined defensive checks work.

(As a follow up to this patch, we may need to teach LiveVariables.cpp and UninitializedValues.cpp about lambdas. For example, to address issues like this one: https://llvm.org/bugs/show_bug.cgi?id=22834)

xazax.hun updated this revision to Diff 34498.Sep 10 2015, 3:21 PM
xazax.hun edited edge metadata.
  • Updated to latest trunk
  • Added test for the defensive inline check heuristic case
  • Added test for diagnostic within a lambda body
  • Added test for path notes

Have you tested this on a large codebase that uses lambdas? When do you think we should turn this on by default?

I checked llvm and clang and did not found any failure. There are no obvious limitations or problems that I know of at the moment. I think this is a good candidate to turn on by default. At least some potential errors might be found earlier.

Please, add test cases that demonstrate what happens when an issue is reported within a lambda and to check if inlined defensive checks work.

I extended the tests with these cases.

(As a follow up to this patch, we may need to teach LiveVariables.cpp and UninitializedValues.cpp about lambdas. For example, to address issues like this one: https://llvm.org/bugs/show_bug.cgi?id=22834)

zaks.anna accepted this revision.Sep 10 2015, 6:32 PM
zaks.anna edited edge metadata.

Please, do turn on by default.
LGTM

This revision is now accepted and ready to land.Sep 10 2015, 6:32 PM
jordan_rose edited edge metadata.Sep 10 2015, 6:57 PM

A few comments coming late…

include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
515–517 ↗(On Diff #34498)

"inline" is kind of a misnomer, since we may not actually inline lambdas. I would have suggested "model lambdas" or "lambda support".

lib/StaticAnalyzer/Core/MemRegion.cpp
740–741 ↗(On Diff #34498)

Why the extra parameter?

xazax.hun added inline comments.Sep 11 2015, 9:24 AM
include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
515–517 ↗(On Diff #34498)

Even when this configuration option is set to false, the body of the lambda is analyzed as a top level function. For this reason I think the "lambda support" might be a misnomer too. What do you think?

lib/StaticAnalyzer/Core/MemRegion.cpp
740–741 ↗(On Diff #34498)

This is just a leftover from code evolution, thank you for spotting this.

This revision was automatically updated to reflect the committed changes.
xazax.hun added inline comments.Sep 11 2015, 9:58 AM
include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
515–517 ↗(On Diff #34498)

If we come up with a better name for the option I will address that in a separate patch.

ted removed a reviewer: ted.Sep 11 2015, 10:26 AM

Looks like this patch is causing regressions:
https://llvm.org/bugs/show_bug.cgi?id=24914

I will look into it. Do you prefer to revert the patch this it is fixed?