1605075 - Inconsistent behaviour of kungFuDeathGrip clang analyis

Status: RESOLVED FIXED

(Developer Infrastructure :: Source Code Analysis, task)

Description

[Simon Giesecke [:sg] [he/him]]

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:

• The term 'temporary value' is somewhat confusing in the message, since this is move-assigning from a member.
• I found there are test cases in https://searchfox.org/mozilla-central/source/build/clang-plugin/tests/TestKungFuDeathGrip.cpp where the source is identified as a 'member', but none where it is identified as a 'temporary value'.

Comment 1

[Andi [:andi]]

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!

Comment 2

[Simon Giesecke [:sg] [he/him]]

I've just hit this again.

Comment 3

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1605075 - Ignore kfdg variables initialized from xvalues in KungFuDeatGripChecker. r=andi

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/81df249dff5b
Ignore kfdg variables initialized from xvalues in KungFuDeatGripChecker. r=andi

Comment 5

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/81df249dff5b

Comment 6

[Simon Giesecke [:sg] [he/him]]

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:

Comment 7

[Ryan VanderMeulen [:RyanVM]]

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

Approved for 76.0b7.

Comment 8

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/releases/mozilla-beta/rev/6b1f5f8ed852