Closed Bug 689066 Opened 13 years ago Closed 13 years ago

Fix c++11 incompatibility

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla10

People

(Reporter: espindola, Assigned: espindola)

References

Details

Attachments

(1 file, 2 obsolete files)

Attached patch proposed patch (obsolete) — Splinter Review
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
Attachment #562334 - Flags: review?(mh+mozilla)
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.
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?
Does your failing tree have bug 686280 fixed ?
(FYI, js/src is *not* built in c++0x mode)
(In reply to Mike Hommey [:glandium] from comment #3)
> Does your failing tree have bug 686280 fixed ?

Yes
(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.
(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.
Attachment #562334 - Flags: review?(mh+mozilla) → review?(luke)
(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; }
?
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 on attachment 562334 [details] [diff] [review]
proposed patch

Ok.  So, just to confirm: this is a C++11 backwards-incompatible change?
Attachment #562334 - Flags: review?(luke) → review+
(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.
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?
(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).
(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.
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.
this has been backed out for Android incompatibility
Target Milestone: --- → mozilla10
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?
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
(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?
> Does this mean that the next m-i=>m-c merge is also going to need a clobber?

Yes.
(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.
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
Attachment #562334 - Attachment is obsolete: true
Attachment #563300 - Flags: review?(ehsan)
(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.
(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.
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.
Assignee: general → respindola
Attachment #563300 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #563300 - Flags: review?(ehsan)
Attachment #563438 - Flags: review?(luke)
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.
Attachment #563438 - Flags: review?(luke) → review+
(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?
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.
https://hg.mozilla.org/mozilla-central/rev/9d8247091f4c
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
(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.
You need to log in before you can comment on or make changes to this bug.