1200413 - The lambda expression static analysis has an interesting bug!

Status: RESOLVED FIXED

(Developer Infrastructure :: Source Code Analysis, defect)

Description

[(no longer active)]

This is the code in question:

<https://dxr.mozilla.org/mozilla-central/rev/f2518b8a7b97b5bb477e94bc9131584007aac887/image/ProgressTracker.cpp#445>

If you remove the MOZ_ASSERT there, the analysis will catch aObserver being used inside the body of the lambda.  If you put it back, the analysis happily accepts the code.

Michael, since you touched this last, do you mind taking a look, please?  Thanks!

Comment 1

[Nika Layzell [:nika] (ni? for response)]

Attachment: Re-write RefCountedInsideLambdaChecker to use captures instead of checking for DeclRef instances

I simplified (in my opinion) the logic in RefCountedInsideLambdaChecker by iterating over the lambda's captures rather than looking for DeclRefs inside of the LambdaExpr. The new version appears to correctly identify the incorrect capture.

 1:09.62 /Volumes/Devel/Code/mozilla/bug1200413/image/ProgressTracker.cpp:446:29: error: Refcounted variable 'aObserver' of type 'mozilla::image::IProgressObserver' cannot be captured by a lambda
 1:09.62     MOZ_ASSERT(!aTable->Get(aObserver, nullptr),
 1:09.62                             ^

I haven't fixed up the failures yet (like the above) yet, so we can't land this yet.

Comment 2

[Seth Fowler [:seth] [:s2h]]

(In reply to Michael Layzell [:mystor] from comment #1)
> I haven't fixed up the failures yet (like the above) yet, so we can't land
> this yet.

That failure is a false positive, of course, since that lambda is stack-only. It'd *really* be nice to have some sort of primitive escape analysis here.

Comment 3

[Botond Ballo [:botond]]

(In reply to Seth Fowler [:seth] [:s2h] from comment #2)
> (In reply to Michael Layzell [:mystor] from comment #1)
> > I haven't fixed up the failures yet (like the above) yet, so we can't land
> > this yet.
> 
> That failure is a false positive, of course, since that lambda is
> stack-only. It'd *really* be nice to have some sort of primitive escape
> analysis here.

I was discussing this with Ehsan today. The problem is that in a case like this, where the lambda is passed to another function (here, CopyOnWrite<ObserverTable>::Write()), whether or not the lambda escapes the function that created it depends on the body of the other function. In general, the static analysis doesn't have the bodies of functions available (as it operates on individual translation units), and as Ehsan argued today, making the result of a static analysis depend on whether or not the body of a function is available is fragile and makes for a poor developer experience ("moving the definition of my function out of line broke my code!").

Comment 4

[(no longer active)]

FWIW there are a *lot* of interesting checks that we could do if we could look at the body of the functions.  I would like to get us there some day, but we're talking about a huge amount of work!

Comment 5

[Nika Layzell [:nika] (ni? for response)]

(In reply to Ehsan Akhgari (don't ask for review please) from comment #4)
> FWIW there are a *lot* of interesting checks that we could do if we could
> look at the body of the functions.  I would like to get us there some day,
> but we're talking about a huge amount of work!

Analyzing every body of code in relation to one another would be inordinately expensive. I think that our best bet lays in doing what most programming languages end up doing: types & function signatures. For example, in this case with the escape analysis, we could have an annotation MOZ_NOESCAPE which could be placed on the parameter. This would mark to all consumers of the function (as it would be part of the function's signature) that that particular parameter doesn't escape the function. 

However, if we were in a compilation unit which had the body of the function available, we would also perform analysis on the body of the function to ensure that it meets the guarantees of the function signature (much in the same way that type checking is performed to make sure that the body conforms to the type signature of the function). In this case, we would analyze the body of the function to ensure that the parameter doesn't escape.

This is how programming languages like rust manage their variable lifetimes, by encoding the constraints on how the body of the function and how the callers of the function can interact through function signatures, and then validating each of these properties on a per-function level. In the case of rust, MOZ_NOESCAPE would be similar to `fn foo<'a>(&'a T)`, which encodes the constraint that the caller can provide a reference of any lifetime (which exceeds the function call), and the function call is valid (which implies the constraint that creating the object immediately before the function, and destroying it immediately after it is returned won't cause any problems). (Rust also has special lifetime elision which allows the 'as to be omitted in most situations, but that's just syntactic sugar to reduce lifetime clutter in type signatures - we probably wouldn't have it in our C++ plugin).

Comment 6

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 2: Make lambdas in ProgressTracker.cpp capture strong references

I made the changes to ProgressTracher.cpp - I figure I should get seth to review this part.

LMK if I should pester someone else.

Comment 7

[Seth Fowler [:seth] [:s2h]]

(In reply to Michael Layzell [:mystor] from comment #5)
> For
> example, in this case with the escape analysis, we could have an annotation
> MOZ_NOESCAPE which could be placed on the parameter. This would mark to all
> consumers of the function (as it would be part of the function's signature)
> that that particular parameter doesn't escape the function. 

MOZ_NOESCAPE would be nice.

I'll note, though, that in general functions that accept lambdas (as opposed to std::function) will be templates, and hence the definition should be available, right?

Comment 8

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 8655493 [details] [diff] [review]
Part 2: Make lambdas in ProgressTracker.cpp capture strong references

Review of attachment 8655493 [details] [diff] [review]:
-----------------------------------------------------------------

This saddens me since this does have a small perf cost (many of these objects use atomic reference counting) but it seems like the best thing to do in the short term.

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/35b17c07e5ee
https://hg.mozilla.org/integration/mozilla-inbound/rev/c13d26d0e0a4

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/35b17c07e5ee
https://hg.mozilla.org/mozilla-central/rev/c13d26d0e0a4

Comment 11

[Botond Ballo [:botond]]

(In reply to Seth Fowler [:seth] [:s2h] from comment #7)
> I'll note, though, that in general functions that accept lambdas (as opposed
> to std::function) will be templates, and hence the definition should be
> available, right?

I think the argument about fragility described in comment 3 still applies ("I changed a function from being a template to being a non-template that takes std::function, and now one the call sites gives a static analysis error").

Comment 12

[Botond Ballo [:botond]]

(In reply to Seth Fowler [:seth] [:s2h] from comment #7)
> (In reply to Michael Layzell [:mystor] from comment #5)
> > For
> > example, in this case with the escape analysis, we could have an annotation
> > MOZ_NOESCAPE which could be placed on the parameter. This would mark to all
> > consumers of the function (as it would be part of the function's signature)
> > that that particular parameter doesn't escape the function. 
> 
> MOZ_NOESCAPE would be nice.

I'm all for MOZ_NOESCAPE. Filed bug 1201275.