Closed Bug 1605075 Opened 8 months ago Closed 4 months ago

Inconsistent behaviour of kungFuDeathGrip clang analyis

Categories

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

task
Not set
normal

Tracking

(firefox76 fixed, firefox77 fixed)

RESOLVED FIXED
mozilla77
Tracking Status
firefox76 --- fixed
firefox77 --- fixed

People

(Reporter: sg, Assigned: sg)

References

Details

Attachments

(1 file)

The following code is accepted by the kungFuDeathGrip analysis in https://searchfox.org/mozilla-central/source/build/clang-plugin/KungFuDeathGripChecker.cpp:

RefPtr<Foo> foo;
mFoo.swap(foo);

(and foo is not used anymore) but the following, equivalent code is not:

const RefPtr<Foo> foo = std::move(mFoo);

and yields an error such as (using a different example):

[task 2019-12-19T11:19:37.222Z] 11:19:37    ERROR -  /builds/worker/workspace/build/src/dom/indexedDB/ActorsChild.cpp:3465:3: error: Unused "kungFuDeathGrip" 'const RefPtr<mozilla::dom::IDBCursor>' objects constructed from temporary values are prohibited
[task 2019-12-19T11:19:37.222Z] 11:19:37     INFO -    const RefPtr<IDBCursor> cursor = std::move(mStrongCursor);
[task 2019-12-19T11:19:37.222Z] 11:19:37     INFO -    ^
[task 2019-12-19T11:19:37.222Z] 11:19:37     INFO -  /builds/worker/workspace/build/src/dom/indexedDB/ActorsChild.cpp:3465:36: note: Please switch all accesses to this value to go through 'cursor', or explicitly pass 'cursor' to `mozilla::Unused`
[task 2019-12-19T11:19:37.222Z] 11:19:37     INFO -    const RefPtr<IDBCursor> cursor = std::move(mStrongCursor);

When looking at the reasoning in Bug 1018486, I don't think either of these cases should be caught by the analysis, but I might miss something.

In any case, both cases should be handled consistenly, i.e. either both should be accepted or both should be rejected by the analysis.

Obviously, a workaround is already suggested in that mozilla::Unused << foo; could be added, but this seems to unnecessarily clutter the code if there is no fundamental reason for the alert.

Two additional points to note in this context:

Flags: needinfo?(bpostelnicu)

I agree with you, in this case where we move something we don't actually create any temporary so I think the checker should be refined, this is a very old implementation and surely it deserves a revamp.
Thanks for noticing this!

Flags: needinfo?(bpostelnicu)

I've just hit this again.

Assignee: nobody → sgiesecke
Blocks: 1619965
Status: NEW → ASSIGNED
Pushed by sgiesecke@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/81df249dff5b
Ignore kfdg variables initialized from xvalues in KungFuDeatGripChecker. r=andi
Status: ASSIGNED → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla77

Comment on attachment 9139401 [details]
Bug 1605075 - Ignore kfdg variables initialized from xvalues in KungFuDeatGripChecker. r=andi

Beta/Release Uplift Approval Request

  • User impact if declined: Blocks uplift of Bug 1619965
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Static analysis is affected only.
  • String changes made/needed:
Attachment #9139401 - Flags: approval-mozilla-beta?

Comment on attachment 9139401 [details]
Bug 1605075 - Ignore kfdg variables initialized from xvalues in KungFuDeatGripChecker. r=andi

Approved for 76.0b7.

Attachment #9139401 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.