Closed Bug 942980 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: MFBT, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED DUPLICATE of bug 915555

People

(Reporter: botond, Unassigned)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

I see hundreds of these during every build.

Example:

In file included from ../../../dist/include/mozilla/LinkedList.h:62,
                 from ../../../dist/include/jspubtd.h:15,
                 from ../../../dist/include/jsfriendapi.h:14,
                 from ../../../dist/include/jsproxy.h:13,
                 from ../../../dist/include/jswrapper.h:13,
                 from /home/botond/dev/mozilla/central/js/xpconnect/wrappers/XrayWrapper.h:14,
                 from /home/botond/dev/mozilla/central/js/xpconnect/wrappers/XrayWrapper.cpp:9:
../../../dist/include/mozilla/Move.h: In function 'typename mozilla::RemoveReference<T>::Type&& mozilla::Move(T&&) [with T = JSObject*&]':
../../../dist/include/js/HashTable.h:631:   instantiated from 'js::HashMapEntry< <template-parameter-1-1>, <template-parameter-1-2> >::HashMapEntry(js::HashMapEntry< <template-parameter-1-1>, <template-parameter-1-2> >&&) [with Key = JSObject*, Value = nsXPCWrappedJS*]'
../../../dist/include/js/HashTable.h:721:   instantiated from 'void js::detail::HashTableEntry<T>::setLive(js::HashNumber, U&&) [with U = js::HashMapEntry<JSObject*, nsXPCWrappedJS*>, T = js::HashMapEntry<JSObject*, nsXPCWrappedJS*>]'
../../../dist/include/js/HashTable.h:1467:   instantiated from 'bool js::detail::HashTable<T, HashPolicy, AllocPolicy>::add(js::detail::HashTable<T, HashPolicy, AllocPolicy>::AddPtr&, U&&) [with U = js::HashMapEntry<JSObject*, nsXPCWrappedJS*>, T = js::HashMapEntry<JSObject*, nsXPCWrappedJS*>, HashPolicy = js::HashMap<JSObject*, nsXPCWrappedJS*, js::PointerHasher<JSObject*, 3u>, js::SystemAllocPolicy>::MapHashPolicy, AllocPolicy = js::SystemAllocPolicy]'
../../../dist/include/js/HashTable.h:142:   instantiated from 'bool js::HashMap<Key, Value, HashPolicy, AllocPolicy>::add(typename js::detail::HashTable<js::HashMapEntry<Key, Value>, js::HashMap<Key, Value, HashPolicy, AllocPolicy>::MapHashPolicy, AllocPolicy>::AddPtr&, KeyInput&&, ValueInput&&) [with KeyInput = JSObject*&, ValueInput = nsXPCWrappedJS*&, Key = JSObject*, Value = nsXPCWrappedJS*, HashPolicy = js::PointerHasher<JSObject*, 3u>, AllocPolicy = js::SystemAllocPolicy]'
/home/botond/dev/mozilla/central/js/xpconnect/wrappers/../src/XPCMaps.h:53:   instantiated from here
../../../dist/include/mozilla/Move.h:193: warning: returning reference to temporary

This is for a B2G build with GCC 4.6. It's not anything new, I'm just reporting it now.
The code referenced is:

185 /**
186  * Identical to std::Move(); this is necessary until our stlport supports
187  * std::move().
188  */
189 template<typename T>
190 inline typename RemoveReference<T>::Type&&
191 Move(T&& a)
192 {
193   return static_cast<typename RemoveReference<T>::Type&&>(a);
194 }

http://mxr.mozilla.org/mozilla-central/source/mfbt/Move.h?force=1#185
There's nothing wrong with the Move code, that's totally generic.  At best there'd be something wrong with the caller, js/public/HashTable.h:631, for Key=JSObject*, Value=nsXPCWrappedJS*:

    HashMapEntry(HashMapEntry &&rhs)
      : key(mozilla::Move(const_cast<Key &>(rhs.key))), value(mozilla::Move(rhs.value)) { }

But I don't think there's anything wrong here, either.  The type of |rhs.value| there should be |nsXPCWrappedJS* &|.  Passing through Move, we get |T = nsXPCWrappedJS* &|, with a return type of |nsXPCWrappedJS* &&|, with that applied to an argument of type |nsXPCWrappedJS* &|.  An lvalue reference is being cast to an rvalue reference, that's kosher.  Definitely |a| is not a temporary.

I'm inclined to chalk this (as well as bug 915555, which is I think the same issue except with respect to nsTHashTable instead of to js::HashTable) up to gcc being buggy here.  I'd try to figure out some sort of warning-disabling workaround, but I'm having trouble generating a reproducible testcase for this from scratch.  Hopefully the gcc 4.6 trunk build I just kicked off will reproduce the warning, and I can trim down a testcase from an original source file.
jimb, am I missing anything here in what I said in comment 2?
Flags: needinfo?(jimb)
Yeah, with my gcc 4.6 build of trunk I don't get any warnings that have "ref" in them anywhere.  Are you sure this isn't gcc 4.4, like bug 915555 is?  I don't have gcc 4.4 handy, but I could cons up a build of it if necessary.
Perhaps the const_cast is a source of confusion to GCC? We could try doing the change suggested in bug 940033 and see if that makes the warnings go away.
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #4)
> Yeah, with my gcc 4.6 build of trunk I don't get any warnings that have
> "ref" in them anywhere.  Are you sure this isn't gcc 4.4, like bug 915555
> is?  I don't have gcc 4.4 handy, but I could cons up a build of it if
> necessary.

It's a B2G build, and I have "export CC=gcc-4.6 CXX=g++-4.6" in my .userconfig file, so I'm assuming gcc 4.6 is being used. Maybe the B2G build is using gcc 4.4 for certain things though? Is there a way I can check?
That is, technically, the argument to Move is not an lvalue, because the result of a cast is not an lvalue. Botond, does this patch (which we should *not* land) make the warnings go away?
Flags: needinfo?(jimb)
Flags: needinfo?(botond)
(In reply to Jim Blandy :jimb from comment #7)
> Botond, does this patch (which we should *not* land) make the warnings go away?

Unfortunately not. I still see the same warnings.
Flags: needinfo?(botond)
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

though that log does seem to be using GCC 4.4 (which I guess this is known to affect, per comment 4).
Per C++11 [expr.const.cast]p1, "The result of the expression const_cast<T>(v) is of type T. If T is an lvalue reference to object type, the result is an lvalue", so I think the result's an lvalue here.  But it seems to me the const_cast is a red herring -- the other bug had this happening with a non-const_cast situation.  I think it may just be some weird issue with lvalue references to pointers.
According to http://wiki.apache.org/stdcxx/C++0xCompilerSupport gcc 4.4 only supports rvalue references v1.0. Not sure what differences there are with v2.1 and v3.0 supported resp. by gcc 4.5 and 4.6, but that could be a reason for this warning.
(In reply to Botond Ballo [:botond] from comment #6)
> (In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #4)
> > Yeah, with my gcc 4.6 build of trunk I don't get any warnings that have
> > "ref" in them anywhere.  Are you sure this isn't gcc 4.4,
> 
> It's a B2G build, and I have "export CC=gcc-4.6 CXX=g++-4.6" in my
> .userconfig file, so I'm assuming gcc 4.6 is being used. Maybe the B2G build
> is using gcc 4.4 for certain things though?

I think the B2G build is indeed using GCC 4.4.

At least, in my build (with those exports in my .userconfig), I got an unrelated build error, which printed the full command line, which started with
[/path/to]/B2G/prebuilt/linux-x86/toolchain/arm-linux-androideabi-4.4.x/bin/arm-linux-androideabi-gcc

That looks like a 4.4 version of GCC, which we use for stuff compiled for ARM, I guess.  So this seems to be a dupe of bug 915555.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 915555
Yes, we indeed use gcc4.4 for b2g.
You need to log in before you can comment on or make changes to this bug.