Closed Bug 689609 Opened 13 years ago Closed 12 years ago

We enable c++0x in places it is not supported

Categories

(Firefox Build System :: General, defect)

x86_64
Linux
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: espindola, Assigned: espindola)

References

Details

Attachments

(1 file, 1 obsolete file)

Attached patch Use a more strict c++0x test (obsolete) — Splinter Review
      No description provided.
Attachment #562788 - Flags: review?(mh+mozilla)
The problem that I found is that android uses gcc 4.4 which has the option, but uses a libstdc++ with no c++0x support.
Comment on attachment 562788 [details] [diff] [review]
Use a more strict c++0x test

Review of attachment 562788 [details] [diff] [review]:
-----------------------------------------------------------------

::: configure.in
@@ +2832,5 @@
>      _SAVE_CXXFLAGS=$CXXFLAGS
>      CXXFLAGS="$CXXFLAGS -std=gnu++0x"
>  
> +    CXXFLAGS="$CXXFLAGS $_MOZ_RTTI_FLAGS"
> +    AC_CACHE_CHECK(for c++0x support,

Please make it clear that this is only testing the subset that we know we need for our codebase.
Attachment #562788 - Flags: review?(mh+mozilla) → review+
The failure on Android is because ANPCanvas.cpp has a dependency on c++11!!! The reduced case is 

enum foo{
  bar = 1
};
int zed = foo::bar;

which is valid c++11, but not valid c++03.
OK, I've objected to building our code in C++11 mode before, but let me rephrase my objection again (and make it stronger):

* Comment 4 includes an example of a valid C++11 program which is not valid C++98.
* Building our code in C++11 mode has caused this pattern to crawl its way into our codebase.  As a result, our Firefox Android code is no longer valid C++98.
* C++11 doesn't currently have any complete implementations, so there is no way for us to know whether our code is valid C++11 by compiling it under gcc/clang c++0x mode.

Therefore, I think we should take out that mode as the default build option for Firefox on all platforms.

I certainly will not object to people building m-c clones under c++0x mode in gcc/clang locally, and submit patches for build failures.  I won't even object to us having Tier-2/3 builders which compile Firefox under c++0x mode.  What I do object to is having that flag enabled by default which has clearly lead to us accept non-valid C++98 code.

glandium, do you agree?
Attachment #562788 - Attachment is obsolete: true
This issue changed my opinion. I liked having the build use c++11 when possible, since it added some interesting checks (like the initializer lists) and got us ready for newer compilers.

What I didn't realize is that it made so easy to create a dependency on an experimental feature.

Is it easy to set up a bot that does a daily build of m-c with c++0x? I would be more than happy to keep an eye on it like I do with clang builds.
I don't disagree, but:

(In reply to Ehsan Akhgari [:ehsan] from comment #5)
> * Building our code in C++11 mode has caused this pattern to crawl its way
> into our codebase.  As a result, our Firefox Android code is no longer valid
> C++98.

Should we actually care? I mean, there's not a lot of different NDKs, and at least all those >= r4 are using the same compiler version. So, as long as everyone is using the same mode, there's no problem.

> * C++11 doesn't currently have any complete implementations, so there is no
> way for us to know whether our code is valid C++11 by compiling it under
> gcc/clang c++0x mode.

On the other hand, on Linux and Android, not using c++0x mode means relying on short wchar_t, which has possible much worse issues.

If only there was a way to have char16_t and utf-16 string literals without c++0x...
(In reply to Mike Hommey [:glandium] from comment #7)
> I don't disagree, but:
> 
> (In reply to Ehsan Akhgari [:ehsan] from comment #5)
> > * Building our code in C++11 mode has caused this pattern to crawl its way
> > into our codebase.  As a result, our Firefox Android code is no longer valid
> > C++98.
> 
> Should we actually care? I mean, there's not a lot of different NDKs, and at
> least all those >= r4 are using the same compiler version. So, as long as
> everyone is using the same mode, there's no problem.

It will also be an issue with OS X only code when we switch to clang. In the other hand, gcc 4.2 is so old that we would probably get incompatible with it fairly quickly anyway.
(In reply to Mike Hommey [:glandium] from comment #7)
> (In reply to Ehsan Akhgari [:ehsan] from comment #5)
> > * Building our code in C++11 mode has caused this pattern to crawl its way
> > into our codebase.  As a result, our Firefox Android code is no longer valid
> > C++98.
> 
> Should we actually care? I mean, there's not a lot of different NDKs, and at
> least all those >= r4 are using the same compiler version. So, as long as
> everyone is using the same mode, there's no problem.

Yes, we should care.  Note that this is not an Android only issue.  This is going to continuously bite us as the compilers change, and it could potentially bite us even more if we get some code in our cross platform code which results in different side effects because of different compilers implementing C++11 differently.

C++11 support at this point is a highly moving target in the compilers.  I'm all for us being future ready, but the fact is that we can't use C++11 features in our code yet, so turning it on by default seems like the wrong choice to me.

> > * C++11 doesn't currently have any complete implementations, so there is no
> > way for us to know whether our code is valid C++11 by compiling it under
> > gcc/clang c++0x mode.
> 
> On the other hand, on Linux and Android, not using c++0x mode means relying
> on short wchar_t, which has possible much worse issues.
> 
> If only there was a way to have char16_t and utf-16 string literals without
> c++0x...

Which issues are you talking about?  Calling into system libraries implementing things like wcslen?  Can we just handle these problems the same way as we did before enabling c++0x mode?
(In reply to Rafael Ávila de Espíndola (:espindola) from comment #6)
> Is it easy to set up a bot that does a daily build of m-c with c++0x? I
> would be more than happy to keep an eye on it like I do with clang builds.

It's definitely possible, it's something that RelEng needs to do so it might be a bit before they get the builds up and running.  But in the mean time a bunch of us can build in C++0x mode locally on a regular basis.  I'd be happy to do that myself, and I'm sure that we can find a few other people as well.  :-)
(In reply to Ehsan Akhgari [:ehsan] from comment #10)
> Which issues are you talking about?  Calling into system libraries
> implementing things like wcslen?  Can we just handle these problems the same
> way as we did before enabling c++0x mode?

That's exactly where the problem is: before enabling c++0x mode, we were relying on -fshort-wchar, and the more we use the STL, the more it is dangerous.
This is the same patch you approved last time, but fixes the use of c++11 features in the android code.

Since this disables c++11 for android (when not using libstdc++), it is also on the right path if we decide to disable it completely.
Assignee: nobody → respindola
Status: NEW → ASSIGNED
Attachment #563126 - Flags: review?(mh+mozilla)
> That's exactly where the problem is: before enabling c++0x mode, we were
> relying on -fshort-wchar, and the more we use the STL, the more it is
> dangerous.

Can you use the system you wrote for checking for unwanted dependencies on newer versions of libstdc++? We could check that no wchar_t is present in a name? For example, in

#include <vector>
int foobar(wchar_t x, std::vector<wchar_t> &y) {
  y.push_back(x);
}

I get a weak copy of _ZNSt6vectorIwSaIwEE9push_backERKw, but in a different libstdc++ it could be a U, which would be bad, but can be found by looking at the libstdc++ symbols that XUL uses.
(In reply to Rafael Ávila de Espíndola (:espindola) from comment #14)
> Can you use the system you wrote for checking for unwanted dependencies on
> newer versions of libstdc++?

Nope, it doesn't do the same thing at all.
In any case, we cannot really use char16_t is core firefox until we switch to clang and vc 2010, right? Until then, the value of having c++0x by default in compilers that support it is much reduced.
(In reply to Rafael Ávila de Espíndola (:espindola) from comment #16)
> In any case, we cannot really use char16_t is core firefox until we switch
> to clang and vc 2010, right?

We effectively use it with gcc now, but that's limited to NS_LITERAL_STRINGs.
OK. We should probably move the more general c++0x discussion to another bug or list and keep this bug for just the smaller issue of not enabling it when it is not supported.
Comment on attachment 563126 [details] [diff] [review]
better check for c++11

r=me on the ANPCanvas.cpp part.

We definitely want this patch for Android.  I'll post something to dev.platform on the general question of whether or not we should use c++0x mode.
Attachment #563126 - Flags: review?(mh+mozilla) → review+
Parts of this patch were backed out and/or re-landed before being merged.  I'm lost track and am not sure whether to resolve it or not.

http://hg.mozilla.org/mozilla-central/rev/4fce3a720570
http://hg.mozilla.org/mozilla-central/rev/0c9d0f4f1c82
http://hg.mozilla.org/mozilla-central/rev/51cc78c7433d
We are stuck with c++11 on android. Things will get better when we upgrade the compiler we use there, but we are not dropping c++11.

Closing this as wontfix.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → WONTFIX
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.