Closed Bug 1170388 Opened 5 years ago Closed 5 years ago

Static analyzer false positives about lambdas using local (non-captured) raw pointers to refcounted objects

Categories

(Firefox Build System :: Source Code Analysis, defect)

defect
Not set

Tracking

(firefox41 fixed, firefox43 fixed)

RESOLVED FIXED
mozilla43
Tracking Status
firefox41 --- fixed
firefox43 --- fixed

People

(Reporter: botond, Assigned: botond)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(1 file)

The patches in bug 1158424 trip [1] the static analysis added in bug 1153304 even though the lambda they introduce does not *capture* a raw pointer to a refcounted object - it merely takes one as argument.

I believe this is perfectly safe - it's just like any other function taking a raw pointer to the refcounted object. The point of bug 1153304 was to guard against *capturing* such a pointer, since that gets stored inside the lambda which may in turn outlive its caller.

[1] http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/bballo@mozilla.com-96128147095d/try-linux64-st-an-debug/try-linux64-st-an-debug-bm79-try1-build46.txt.gz
Bug 1170388 - Restrict the static analysis error given about raw pointers to refcounted objects inside a lambda, to the case where the raw pointer is captured. r=ehsan
Attachment #8614972 - Flags: review?(ehsan)
Assignee: nobody → botond
Attachment #8614972 - Flags: review?(ehsan) → review+
Comment on attachment 8614972 [details]
MozReview Request: Bug 1170388 - Removed an outdated workaround for old clang versions in the static analysis plugin. r=mystor

https://reviewboard.mozilla.org/r/10131/#review8949

Ship It!
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/b0b9843b8654 for bustage.

Looks like automation's using clang 3.3 for the static analysis, but my patch uses the matcher 'equalsBoundNode' which was only added in clang 3.4.
(In reply to Botond Ballo [:botond] from comment #4)
> Backed out in
> https://hg.mozilla.org/integration/mozilla-inbound/rev/b0b9843b8654 for
> bustage.
> 
> Looks like automation's using clang 3.3 for the static analysis, but my
> patch uses the matcher 'equalsBoundNode' which was only added in clang 3.4.

Ugh.

You can probably copy its implementation...  IIRC I had to do the same for example for IsInSystemHeader.
Comment on attachment 8614972 [details]
MozReview Request: Bug 1170388 - Removed an outdated workaround for old clang versions in the static analysis plugin. r=mystor

Bug 1170388 - Restrict the static analysis error given about raw pointers to refcounted objects inside a lambda, to the case where the raw pointer is captured. r=ehsan
Attachment #8614972 - Flags: review+ → review?(ehsan)
Updated patch to polyfill equalsBoundNode on clang < 3.4. It wasn't a straight copy of the 3.4 implementation because some of the infrastructure changed, but it seems to be working.

Try push showing that it's working:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bfd42baee21f

Posting for re-review. Note that the only change from the last patch is the added hunk that provides equalsBoundNode.
Attachment #8614972 - Flags: review?(ehsan) → review+
Comment on attachment 8614972 [details]
MozReview Request: Bug 1170388 - Removed an outdated workaround for old clang versions in the static analysis plugin. r=mystor

https://reviewboard.mozilla.org/r/10131/#review9189

Ship It!

::: build/clang-plugin/clang-plugin.cpp:611
(Diff revision 2)
> +    const NodeType& Node;

Nit: we try to use the LLVM coding style for this file, so * and & should be attached to the variable names here and elsewhere.

::: build/clang-plugin/clang-plugin.cpp:622
(Diff revision 2)
> +  Visitor* visitor = new Visitor(Node, ID, haveMatchingResult);

Hmm, why are you allocating this on the heap?  It seems like it could live on the stack.
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #8)
> Comment on attachment 8614972 [details]
> MozReview Request: Bug 1170388 - Restrict the static analysis error given
> about raw pointers to refcounted objects inside a lambda, to the case where
> the raw pointer is captured. r=ehsan
> 
> https://reviewboard.mozilla.org/r/10131/#review9189
> 
> Ship It!
> 
> ::: build/clang-plugin/clang-plugin.cpp:611
> (Diff revision 2)
> > +    const NodeType& Node;
> 
> Nit: we try to use the LLVM coding style for this file, so * and & should be
> attached to the variable names here and elsewhere.

Fixed.

> ::: build/clang-plugin/clang-plugin.cpp:622
> (Diff revision 2)
> > +  Visitor* visitor = new Visitor(Node, ID, haveMatchingResult);
> 
> Hmm, why are you allocating this on the heap?  It seems like it could live
> on the stack.

My reason was that visitMatches() takes the visitor by pointer, but that's a bad reason :) Fixed.
https://hg.mozilla.org/mozilla-central/rev/03ff333d49c0
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
The fact that the static analysis is still triggering [1] on my patches in bug 1158424 indicates that the fix for clang 3.3 isn't correct. Reopening.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=5ff606ee785a
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
I spent some time trying to debug this the issue on the cland 3.3 codepath, without luck. 

Ehsan and I discussed this briefly and agreed that a better way forward is to upgrade automation to use clang >= 3.4.
Flags: needinfo?(ehsan)
Going to unassign for now as I have no immediately plans to continue working on this. If we end up upgrading automation's clang to >= 3.4, that will solve the issue without further work, otherwise I can continue trying to find a 3.3 workaround after Whistler.
Assignee: botond → nobody
No longer blocks: 1173255
Summary: Static analyzer objects to lambda taking as argument a raw pointer to a refcounted object → Static analyzer false positives about lambdas using local (non-captured) raw pointers to refcounted objects
Bug 1123386 has now landed, which upgrades the clang version used for the static analysis builds in automation to >= 3.4. I believe this upgrade should resolve this issue.

Once we verify that this is the case (by someone landing a patch that uses lambdas that use non-captured raw pointers to refcounted objects and that patch passing static analysis), I'll land a patch in this bug to remove my attempt at a pre-3.4 solution (which never worked) that landed in comment 11.
Flags: needinfo?(ehsan)
(In reply to Botond Ballo [:botond] from comment #15)
> Bug 1123386 has now landed, which upgrades the clang version used for the
> static analysis builds in automation to >= 3.4. I believe this upgrade
> should resolve this issue.
> 
> Once we verify that this is the case (by someone landing a patch that uses
> lambdas that use non-captured raw pointers to refcounted objects and that
> patch passing static analysis), I'll land a patch in this bug to remove my
> attempt at a pre-3.4 solution (which never worked) that landed in comment 11.

Bug 1200413 has made this point moot by rewriting the static analysis to not use the 'equalsBoundNode' matcher that I was trying to polyfill. I'll post a patch to remove the (broken) polyfill attempt.
Comment on attachment 8614972 [details]
MozReview Request: Bug 1170388 - Removed an outdated workaround for old clang versions in the static analysis plugin. r=mystor

Bug 1170388 - Removed an outdated workaround for old clang versions in the static analysis plugin. r=mystor
Attachment #8614972 - Attachment description: MozReview Request: Bug 1170388 - Restrict the static analysis error given about raw pointers to refcounted objects inside a lambda, to the case where the raw pointer is captured. r=ehsan → MozReview Request: Bug 1170388 - Removed an outdated workaround for old clang versions in the static analysis plugin. r=mystor
Attachment #8614972 - Flags: review?(michael)
Comment on attachment 8614972 [details]
MozReview Request: Bug 1170388 - Removed an outdated workaround for old clang versions in the static analysis plugin. r=mystor

https://reviewboard.mozilla.org/r/10131/#review16409

LGTM
Attachment #8614972 - Flags: review?(michael) → review+
https://hg.mozilla.org/mozilla-central/rev/d7fed4b3bb8a
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: mozilla41 → mozilla43
Assignee: nobody → botond
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.