Closed Bug 1479232 Opened 4 years ago Closed 4 years ago

error: variable of type 'already_AddRefed<*>' is only valid as a temporary

Categories

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

defect
Not set
normal

Tracking

(firefox64 fixed)

RESOLVED FIXED
mozilla64
Tracking Status
firefox64 --- fixed

People

(Reporter: glandium, Assigned: glandium)

References

(Depends on 1 open bug)

Details

Attachments

(1 file)

Background:
- bug 1434689 added a static analysis for classes intended to be used for temporaries only.
- bug 1443265 marked already_AddRefed as such.

I was going a try build with the same revision of clang on osx as we use on windows to see if it would fix its LTO issues. Mac builds have the clang plugin enabled by default, and my build failed with:

[task 2018-07-28T23:48:58.705Z] 23:48:58     INFO -  In file included from /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/dom/ChromeUtilsBinding.h:11:
[task 2018-07-28T23:48:58.705Z] 23:48:58     INFO -  /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/dom/FakeString.h:130:31: error: variable of type 'already_AddRefed<nsStringBuffer>' is only valid as a temporary
[task 2018-07-28T23:48:58.705Z] 23:48:58     INFO -    void AssignFromStringBuffer(already_AddRefed<nsStringBuffer> aBuffer) {
[task 2018-07-28T23:48:58.707Z] 23:48:58     INFO -                                ^
[task 2018-07-28T23:48:58.708Z] 23:48:58     INFO -  /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/dom/FakeString.h:130:31: note: value incorrectly allocated in an automatic variable
[task 2018-07-28T23:48:58.709Z] 23:48:58     INFO -  In file included from /builds/worker/workspace/build/src/obj-firefox/security/certverifier/Unified_cpp_certverifier0.cpp:110:
[task 2018-07-28T23:48:58.710Z] 23:48:58     INFO -  In file included from /builds/worker/workspace/build/src/security/certverifier/NSSCertDBTrustDomain.cpp:28:
[task 2018-07-28T23:48:58.711Z] 23:48:58     INFO -  In file included from /builds/worker/workspace/build/src/obj-firefox/dist/include/nsThreadUtils.h:20:
[task 2018-07-28T23:48:58.711Z] 23:48:58     INFO -  In file included from /builds/worker/workspace/build/src/obj-firefox/dist/include/nsIThread.h:10:
[task 2018-07-28T23:48:58.712Z] 23:48:58     INFO -  In file included from /builds/worker/workspace/build/src/obj-firefox/dist/include/nsISerialEventTarget.h:10:
[task 2018-07-28T23:48:58.713Z] 23:48:58     INFO -  /builds/worker/workspace/build/src/obj-firefox/dist/include/nsIEventTarget.h:60:23: error: variable of type 'already_AddRefed<nsIRunnable>' is only valid as a temporary
[task 2018-07-28T23:48:58.713Z] 23:48:58     INFO -    NS_IMETHOD Dispatch(already_AddRefed<nsIRunnable> event, uint32_t flags = DISPATCH_NORMAL) = 0;
[task 2018-07-28T23:48:58.713Z] 23:48:58     INFO -                        ^
[task 2018-07-28T23:48:58.714Z] 23:48:58     INFO -  /builds/worker/workspace/build/src/obj-firefox/dist/include/nsIEventTarget.h:60:23: note: value incorrectly allocated in an automatic variable
[task 2018-07-28T23:48:58.714Z] 23:48:58     INFO -  /builds/worker/workspace/build/src/obj-firefox/dist/include/nsIEventTarget.h:66:30: error: variable of type 'already_AddRefed<nsIRunnable>' is only valid as a temporary
[task 2018-07-28T23:48:58.714Z] 23:48:58     INFO -    NS_IMETHOD DelayedDispatch(already_AddRefed<nsIRunnable> event, uint32_t delay) = 0;
[task 2018-07-28T23:48:58.714Z] 23:48:58     INFO -                               ^
[task 2018-07-28T23:48:58.714Z] 23:48:58     INFO -  /builds/worker/workspace/build/src/obj-firefox/dist/include/nsIEventTarget.h:66:30: note: value incorrectly allocated in an automatic variable
[task 2018-07-28T23:48:58.714Z] 23:48:58     INFO -  In file included from /builds/worker/workspace/build/src/obj-firefox/security/certverifier/Unified_cpp_certverifier0.cpp:110:
[task 2018-07-28T23:48:58.714Z] 23:48:58     INFO -  In file included from /builds/worker/workspace/build/src/security/certverifier/NSSCertDBTrustDomain.cpp:28:
[task 2018-07-28T23:48:58.714Z] 23:48:58     INFO -  In file included from /builds/worker/workspace/build/src/obj-firefox/dist/include/nsThreadUtils.h:20:
[task 2018-07-28T23:48:58.714Z] 23:48:58     INFO -  /builds/worker/workspace/build/src/obj-firefox/dist/include/nsIThread.h:51:27: error: variable of type 'already_AddRefed<nsIRunnable>' is only valid as a temporary
[task 2018-07-28T23:48:58.714Z] 23:48:58     INFO -    NS_IMETHOD IdleDispatch(already_AddRefed<nsIRunnable> event) = 0;
[task 2018-07-28T23:48:58.714Z] 23:48:58     INFO -                            ^
[task 2018-07-28T23:48:58.714Z] 23:48:58     INFO -  /builds/worker/workspace/build/src/obj-firefox/dist/include/nsIThread.h:51:27: note: value incorrectly allocated in an automatic variable
[task 2018-07-28T23:48:58.714Z] 23:48:58     INFO -  4 errors generated.
[task 2018-07-28T23:48:58.714Z] 23:48:58     INFO -  /builds/worker/workspace/build/src/config/rules.mk:1052: recipe for target 'Unified_cpp_certverifier0.o' failed
[task 2018-07-28T23:48:58.714Z] 23:48:58     INFO -  make[4]: *** [Unified_cpp_certverifier0.o] Error 1

Here's the weird thing though: those files have been using alreadyAddRefed this way since well before those bugs landed.

Considering the error is happening on method declarations rather than calls, it would seem the static analysis is broken somehow with newer versions of clang.
I saw errors like that in bug 1428158 comment 10 (static analysis for Android), but I think they went away after fixing some mismatches between what configure thought clang supported, AST-wise, and what the static analysis actually needed to look for (cf bug 1454667).  Maybe there's a mismatch between host and target clangs, or something?
This also (unsurprisingly) happens if I upgrade clang to version 7rc3 on Linux builds.
Andi, when you are back from pto, could you please help Mike with that?
Assignee: nobody → bpostelnicu
Blocks: 1492663
Assignee: bpostelnicu → mh+mozilla
ParmVarDecl being a subclass of VarDecl, using two matchers then caused
ScopeChecker::check to be called twice for ParmVarDecl nodes, once for
each match. But the code in ScopeCheck::check is written with the
assumption that it's called only once for such nodes.

Somehow, this didn't cause problems with clang up to version 6, but
makes the plugin spuriously warn about already_AddRefed not being used
as temporaries when used as argument in function declarations, with
clang 7.
Depends on: 1492743
Comment on attachment 9010540 [details]
Bug 1479232 - Only use one matcher for varDecl and parmVarDecl

Andi-Bogdan Postelnicu [:andi] has approved the revision.
Attachment #9010540 - Flags: review+
Pushed by mh@glandium.org:
https://hg.mozilla.org/integration/autoland/rev/65cd5f435d68
Only use one matcher for varDecl and parmVarDecl r=andi
https://hg.mozilla.org/mozilla-central/rev/65cd5f435d68
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
You need to log in before you can comment on or make changes to this bug.