Last Comment Bug 689066 - Fix c++11 incompatibility
: Fix c++11 incompatibility
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86_64 Linux
: -- normal (vote)
: mozilla10
Assigned To: Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
:
Mentors:
Depends on: 689609
Blocks:
  Show dependency treegraph
 
Reported: 2011-09-25 16:10 PDT by Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
Modified: 2011-09-30 02:30 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
proposed patch (679 bytes, patch)
2011-09-25 16:10 PDT, Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
luke: review+
Details | Diff | Review
use static_cast instead of std::move (1.02 KB, patch)
2011-09-28 21:47 PDT, Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
no flags Details | Diff | Review
now with the correct style (1.05 KB, patch)
2011-09-29 09:11 PDT, Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
luke: review+
Details | Diff | Review

Description Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-09-25 16:10:45 PDT
Created attachment 562334 [details] [diff] [review]
proposed patch

Clang is failing to build firefox in c++0x mode. The error message is:

error: no viable conversion from 'MoveRef<foo>' to 'foo'

for various values of "foo".

I initially thought it was a clang bug (see llvm.org/pr11003), but after reading [dcl.init.ref ] I think it is the correct behavior. A summary of the problem is that we were trying to initialize a rvalue reference with an lvalue reference and that is not supported.

A try run is at https://tbpl.mozilla.org/?tree=Try&rev=8b370f4dde80
Comment 1 Luke Wagner [:luke] 2011-09-25 19:43:32 PDT
I do not understand why this fix is necessary; could you explain it in more detail?  Note, this code pattern was just analyzed in the context of a gcc-4.6-only error that was ruled to be the fault of gcc: http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50442.
Comment 2 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-09-25 20:27:13 PDT
It is a different issue, in fact clang handles the example in the gcc pr just fine.

The reduced case that clang complains about is:

class Value {
};
struct MoveRef {
  operator Value &() const ;
};
MoveRef Move(int);
void growTo() {
  Value x = Move(0);
  Value y(Move(0)); // no viable conversion from 'MoveRef' to 'Value'
}

In here clang is looking for a conversion to a rvalue reference.  The rules for what can be done are in dcl.init.ref paragraph 5, which for rvalue references states:

If the initializer expression...

  ... (first case)
  has a class type (i.e., T2 is a class type), where T1 is not reference-related to T2, and can be implicitly converted to an xvalue, class prvalue, or function lvalue of type “cv3 T3”, where “cv1 T1” is reference-compatible with “cv3 T3”,

....

In the second case, if the reference is an rvalue reference and the second standard con-
version sequence of the user-defined conversion sequence includes an lvalue-to-rvalue conversion, the program is ill-formed.


Why is clang looking for a rvalue reference? The implicit move constructor of Value.

This is my first time reading this part of the standard, so it is probably OK if we want to wait for someone else to reply in http://llvm.org/pr11003.

Assuming the above interpretation is correct, would the patch be OK?
Comment 3 Mike Hommey [:glandium] 2011-09-25 23:54:36 PDT
Does your failing tree have bug 686280 fixed ?
Comment 4 Mike Hommey [:glandium] 2011-09-25 23:56:56 PDT
(FYI, js/src is *not* built in c++0x mode)
Comment 5 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-09-26 09:20:29 PDT
(In reply to Mike Hommey [:glandium] from comment #3)
> Does your failing tree have bug 686280 fixed ?

Yes
Comment 6 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-09-26 09:22:06 PDT
(In reply to Mike Hommey [:glandium] from comment #4)
> (FYI, js/src is *not* built in c++0x mode)

I don't remember with TU found this bug. I will check. BTW, why shouldn't js be built in c++0x mode? There are ABI differences, so it is strange to have part of the code built with one and one without.
Comment 7 Mike Hommey [:glandium] 2011-09-26 09:25:57 PDT
(In reply to Rafael Ávila de Espíndola (:espindola) from comment #6)
> (In reply to Mike Hommey [:glandium] from comment #4)
> > (FYI, js/src is *not* built in c++0x mode)
> 
> I don't remember with TU found this bug. I will check. BTW, why shouldn't js
> be built in c++0x mode? There are ABI differences, so it is strange to have
> part of the code built with one and one without.

Merely by mistake. I only figured recently that the flag wasn't used in js/src. I also figured that js/src doesn't build with gcc in c++0x mode but haven't filed bugs yet.
Comment 8 Luke Wagner [:luke] 2011-09-26 09:29:20 PDT
(In reply to Rafael Ávila de Espíndola (:espindola) from comment #2)
So, IIUC, C++11 is making a backwards-incompatible change?

What happens if you just add (no #ifdefs, no other changes)
  operator const Value &() const { return *pointer; }
?
Comment 9 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-09-26 09:54:36 PDT
You mean something like

class Value {
};
struct MoveRef {
  operator Value &() const ;
  operator const Value &() const;
};
MoveRef Move(int);
void growTo() {
  Value y(Move(0));
}

We get an error with both gcc and clang about the ambiguity of having two operator().
Comment 10 Luke Wagner [:luke] 2011-09-26 10:07:02 PDT
Comment on attachment 562334 [details] [diff] [review]
proposed patch

Ok.  So, just to confirm: this is a C++11 backwards-incompatible change?
Comment 11 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-09-26 10:11:30 PDT
(In reply to Luke Wagner [:luke] from comment #10)
> Comment on attachment 562334 [details] [diff] [review] [diff] [details] [review]
> proposed patch
> 
> Ok.  So, just to confirm: this is a C++11 backwards-incompatible change?

I think so. I will give it a day or so to see if anyone comments on the clang side.
Comment 12 Luke Wagner [:luke] 2011-09-26 10:37:58 PDT
Thanks for tracking this all down Rafael!

I'm surprised they would make breaking changes.  I wonder if there is some fundamental conflict regarding rval-references that requires it?
Comment 13 :Ehsan Akhgari (busy, don't ask for review please) 2011-09-26 10:40:29 PDT
(In reply to Rafael Ávila de Espíndola (:espindola) from comment #9)
> You mean something like
> 
> class Value {
> };
> struct MoveRef {
>   operator Value &() const ;
>   operator const Value &() const;
> };
> MoveRef Move(int);
> void growTo() {
>   Value y(Move(0));
> }
> 
> We get an error with both gcc and clang about the ambiguity of having two
> operator().

What if we change the first operator Value& to be non-const?  That _should_ compile correctly (although I'm not sure if that would preserve the semantics or not).
Comment 14 Luke Wagner [:luke] 2011-09-26 11:00:51 PDT
(In reply to Ehsan Akhgari [:ehsan] from comment #13)
> What if we change the first operator Value& to be non-const?  That _should_
> compile correctly (although I'm not sure if that would preserve the
> semantics or not).

That would change the semantics: since MoveRef is a pointer-like thing, the change would conflate the const-ness of the pointer and the pointee.
Comment 15 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-09-27 07:10:36 PDT
I pushed it to inbound with an added comment on what is going on.

https://hg.mozilla.org/integration/mozilla-inbound/rev/687e0bbfe996

Back in llvm.org/pr11003 Eli Friedman points out that maybe the problem is not not finding a conversion, it is trying to use the move constructor instead of the copy constructor, which might be a defect in the standard.

I will keep an eye on it and update this as appropriate.
Comment 16 Marco Bonardo [::mak] 2011-09-28 01:59:27 PDT
this has been backed out for Android incompatibility
Comment 17 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-09-28 14:01:24 PDT
The underline Android problem was fixed in

https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=4fce3a720570

And a try run on top of that is in

https://tbpl.mozilla.org/?tree=Try&rev=81c4b4d29edb

Is it OK to check it in again once the try is done?
Comment 18 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-09-28 15:04:19 PDT
I canceled the previous try before noticing it was a missing clobber. A new one is in

https://tbpl.mozilla.org/?usebuildbot=1&tree=Try&rev=97e3443efdbc
Comment 19 :Ehsan Akhgari (busy, don't ask for review please) 2011-09-28 16:29:35 PDT
(In reply to Rafael Ávila de Espíndola (:espindola) from comment #18)
> I canceled the previous try before noticing it was a missing clobber. A new
> one is in
> 
> https://tbpl.mozilla.org/?usebuildbot=1&tree=Try&rev=97e3443efdbc

Does this mean that the next m-i=>m-c merge is also going to need a clobber?
Comment 20 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-09-28 18:48:59 PDT
> Does this mean that the next m-i=>m-c merge is also going to need a clobber?

Yes.
Comment 21 :Ehsan Akhgari (busy, don't ask for review please) 2011-09-28 18:58:43 PDT
(In reply to Rafael Ávila de Espíndola (:espindola) from comment #20)
> > Does this mean that the next m-i=>m-c merge is also going to need a clobber?
> 
> Yes.

CCing some people who commonly do merges.
Comment 22 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-09-28 21:47:11 PDT
Created attachment 563300 [details] [diff] [review]
use static_cast instead of std::move

The android c++ library has no c++ 11 support, but disabling c++11 causes fennec to crash. Life is too short to try to figure out why a patched gcc 4.4 or our use of -fshort-wchar causes problems to Fennec is c++ 98 mode.

Instead, use static cast instead of std::move.

A try is at https://tbpl.mozilla.org/?tree=Try&rev=35060eed2648
Comment 23 Mike Hommey [:glandium] 2011-09-28 22:29:30 PDT
(In reply to Ehsan Akhgari [:ehsan] from comment #19)
> (In reply to Rafael Ávila de Espíndola (:espindola) from comment #18)
> > I canceled the previous try before noticing it was a missing clobber. A new
> > one is in
> > 
> > https://tbpl.mozilla.org/?usebuildbot=1&tree=Try&rev=97e3443efdbc
> 
> Does this mean that the next m-i=>m-c merge is also going to need a clobber?

Please note that Try always clobbers.
Comment 24 Mike Hommey [:glandium] 2011-09-29 00:55:51 PDT
(In reply to Rafael Ávila de Espíndola (:espindola) from comment #22)
> The android c++ library has no c++ 11 support, but disabling c++11 causes
> fennec to crash. Life is too short to try to figure out why a patched gcc
> 4.4 or our use of -fshort-wchar causes problems to Fennec is c++ 98 mode.

I could reproduce the crash on a local build with your try push. It's even a double crash: the crash reporter is not able to create a dump meaning it probably crashes trying to.
Removing -fshort-wchar option fixed it.
Comment 25 Mike Hommey [:glandium] 2011-09-29 02:15:27 PDT
Here is a good one: apparently, it's the crash reporter code that crashes us in the first place: I tried a build without the crash reporter, and it works. Haven't tried more than startup, though. We do call into the crash reporter at startup to tell it where our libraries are loaded, so that would make sense.
Comment 26 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-09-29 09:11:30 PDT
Created attachment 563438 [details] [diff] [review]
now with the correct style
Comment 27 Luke Wagner [:luke] 2011-09-29 09:27:03 PDT
Comment on attachment 563438 [details] [diff] [review]
now with the correct style

I would merge the two comments into one.  Also there is a strange newline after the static_cast.
Comment 28 :Ehsan Akhgari (busy, don't ask for review please) 2011-09-29 10:06:01 PDT
(In reply to Mike Hommey [:glandium] from comment #25)
> Here is a good one: apparently, it's the crash reporter code that crashes us
> in the first place: I tried a build without the crash reporter, and it
> works. Haven't tried more than startup, though. We do call into the crash
> reporter at startup to tell it where our libraries are loaded, so that would
> make sense.

Does -fshort-wchar successfully make it to the CXXFLAGS for the crashreporter code?
Comment 29 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-09-29 10:51:39 PDT
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=9d8247091f4c

Once this lands we should hopefully be able to build with clang again both on linux and OS X.
Comment 30 :Ehsan Akhgari (busy, don't ask for review please) 2011-09-29 15:54:19 PDT
https://hg.mozilla.org/mozilla-central/rev/9d8247091f4c
Comment 31 Mike Hommey [:glandium] 2011-09-30 02:30:34 PDT
(In reply to Ehsan Akhgari [:ehsan] from comment #28)
> (In reply to Mike Hommey [:glandium] from comment #25)
> > Here is a good one: apparently, it's the crash reporter code that crashes us
> > in the first place: I tried a build without the crash reporter, and it
> > works. Haven't tried more than startup, though. We do call into the crash
> > reporter at startup to tell it where our libraries are loaded, so that would
> > make sense.
> 
> Does -fshort-wchar successfully make it to the CXXFLAGS for the
> crashreporter code?

For the record, as this was discussed on IRC, yes it does. It also happens that i was able to get a backtrace from the crash, and it happens in IPC code, not in crash reporter code.
The conclusion from the IRC discussion is that we're pretty much screwed on android: we can't use -fshort-wchar.

Note You need to log in before you can comment on or make changes to this bug.