Closed Bug 915555 Opened 6 years ago Closed 11 months ago

Move.h:186: warning: returning reference to temporary

Categories

(Core :: MFBT, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED WONTFIX

People

(Reporter: hub, Unassigned)

References

()

Details

Attachments

(1 obsolete file)

Building gecko for B2G, I get a lot of these warning in various parts of the code.

In file included from ../../../dist/include/js/Utility.h:14,
                 from ../../../dist/include/js/ProfilingStack.h:14,
                 from ../../../dist/include/PseudoStack.h:12,
                 from ../../../dist/include/GeckoProfilerImpl.h:21,
                 from ../../../dist/include/GeckoProfiler.h:158,
                 from ../../../ipc/ipdl/_ipdlheaders/mozilla/net/PNecko.h:21,
                 from ../../../ipc/ipdl/_ipdlheaders/mozilla/net/PNeckoChild.h:10,
                 from ../../../dist/include/mozilla/net/NeckoChild.h:12,
                 from /home/hub/source/mozilla/B2G-inari/gecko/netwerk/protocol/http/HttpLog.h:43,
                 from /home/hub/source/mozilla/B2G-inari/gecko/netwerk/protocol/http/nsHttpConnectionMgr.cpp:8:
../../../dist/include/mozilla/Move.h: In function 'typename mozilla::RemoveReference<T>::Type&& mozilla::Move(T&&) [with T = nsHttpConnectionMgr::nsConnectionEntry*&]':
../../../dist/include/nsBaseHashtable.h:358:   instantiated from 'nsBaseHashtableET<KeyClass, DataType>::nsBaseHashtableET(nsBaseHashtableET<KeyClass, DataType>&&) [with KeyClass = nsCStringHashKey, DataType = nsHttpConnectionMgr::nsConnectionEntry*]'
../../../dist/include/nsTHashtable.h:444:   instantiated from 'static void nsTHashtable<EntryType>::s_CopyEntry(PLDHashTable*, const PLDHashEntryHdr*, PLDHashEntryHdr*) [with EntryType = nsBaseHashtableET<nsCStringHashKey, nsHttpConnectionMgr::nsConnectionEntry*>]'
../../../dist/include/nsTHashtable.h:407:   instantiated from 'void nsTHashtable<EntryType>::Init(uint32_t) [with EntryType = nsBaseHashtableET<nsCStringHashKey, nsHttpConnectionMgr::nsConnectionEntry*>]'
../../../dist/include/nsTHashtable.h:90:   instantiated from 'nsTHashtable<EntryType>::nsTHashtable() [with EntryType = nsBaseHashtableET<nsCStringHashKey, nsHttpConnectionMgr::nsConnectionEntry*>]'
../../../dist/include/nsBaseHashtable.h:62:   instantiated from 'nsBaseHashtable<KeyClass, DataType, UserDataType>::nsBaseHashtable() [with KeyClass = nsCStringHashKey, DataType = nsHttpConnectionMgr::nsConnectionEntry*, UserDataType = nsHttpConnectionMgr::nsConnectionEntry*]'
../../../dist/include/nsDataHashtable.h:26:   instantiated from 'nsDataHashtable<KeyClass, DataType>::nsDataHashtable() [with KeyClass = nsCStringHashKey, DataType = nsHttpConnectionMgr::nsConnectionEntry*]'
/home/hub/source/mozilla/B2G-inari/gecko/netwerk/protocol/http/nsHttpConnectionMgr.cpp:72:   instantiated from here
../../../dist/include/mozilla/Move.h:186: warning: returning reference to temporary
Attached patch Possible patch? (obsolete) — Splinter Review
I think this might be a case of gcc 4.4 being silly, because all that's happening here is a temporary (rvalue reference) is being static_cast<> to an rvalue reference: pointless but harmless.  (Perhaps gcc > 4.4 stopped warning on cast of a temporary, if it was a cast *to* a temporary?  *shrug*)  Does removing the cast as this patch does eliminate the warnings?
Attachment #803844 - Flags: feedback?(hub)
Comment on attachment 803844 [details] [diff] [review]
Possible patch?

yes this remove the warning when building FxOS.
Attachment #803844 - Flags: feedback?(hub) → feedback+
Oops, this got lost in a tree I haven't touched in practically a month.  Landed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/82d5d92a3eac
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla27
Blargh.  Will look later.
...oh, I might know.  I think I wrote this patch atop patches that fixed bug 896100, which changed stuff a bit so that move constructors were used, rather than constructors taking MoveRef and such.  Then I moved this patch to the top of the stack to land it, without thinking of the dependency there.  So I guess this depends on that bug, then.
Depends on: 896100
Comment on attachment 803844 [details] [diff] [review]
Possible patch?

Actually, no, on second look I think this is wrong.  The type of |*fromEntry| is an lvalue reference, and we want to move the guts out of that, into |to|.  Without the Move(), we'd just be copy-constructing.  Which should work, and which is why the warning went away, but it loses all the moveful-ness.

This might require some sort of macro hack to disable this warning specifically for certain versions of gcc, maybe.  :-\
Attachment #803844 - Attachment is obsolete: true
FWIW, it looks like we hit this warning in B2G builds on TBPL, too (B2G ICS Emulator, at least):
 https://tbpl.mozilla.org/php/getParsedLog.php?id=31062590&tree=Mozilla-Inbound

(unsurprising, but a nice sanity check that this isn't just a botond's-machine-only issue)
(er, sorry -- I meant to post that on bug 942980 (which may be a dupe of this bug))
Duplicate of this bug: 942980
This bug is preventing us from using nsDataHashTable in our code. Is there an ETA on fixing this?
Not really.  Without spending serious time to page this bug back into memory, my last conclusion was this was a compiler bug, and without an obvious workaround in sight, the only real option is to wait for b2g to use a newer gcc.  Can't keep using gcc 4.4 (or the weird 4.4/4.5 amalgam that it appears to be) forever, after all.
(In reply to Anshul from comment #12)
> This bug is preventing us from using nsDataHashTable in our code. Is there
> an ETA on fixing this?

Anshul, can you please explain how this impacts 2.1. The user impact is unclear here so can you plase help with that info and also how did we deal with that in past release ?
Flags: needinfo?(anshulj)
Bhavna, I can't reproduce this issue anymore on 2.1.
No longer blocks: CAF-v2.1-FC-metabug
Flags: needinfo?(anshulj)
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #13)
> [...] without an
> obvious workaround in sight, the only real option is to wait for b2g to use
> a newer gcc.  Can't keep using gcc 4.4 (or the weird 4.4/4.5 amalgam that it
> appears to be) forever, after all.

This is tracked in bug 1077549, FWIW.
Depends on: 1077549
Target Milestone: mozilla27 → ---
No longer blocks: 1085635
No assignee, updating the status.
Status: ASSIGNED → NEW
No assignee, updating the status.
No assignee, updating the status.
It looks like this only affected older versions of gcc (4.4 and older, per comment 5).

https://developer.mozilla.org/en-US/docs/Mozilla/Using_CXX_in_Mozilla_code says we now have a much-newer minimum required version of gcc:
>  As of Mozilla 61, gcc 6.1 is required on all platforms.

So this is WONTFIX (i.e. we won't add any hacks to work around this gcc-4.4-and-older issue)
Status: NEW → RESOLVED
Closed: 11 months ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.