This is an archive of the discontinued LLVM Phabricator instance.

[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

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

"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

Why the extra parameter?

xazax.hun added inline comments.Sep 11 2015, 9:24 AM
include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
515–517

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

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

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?