Make compile with gcc 4.7 work

Make compile with gcc 4.7 work

BUG=none
R=thakis@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=286775

[Nico, 2014-07-28 15:25:30]

The string literal change is fine, but we want to require 4.8. When we use more
C++11 soon, 4.7 cannot be supported.

[Paweł Hajdan Jr., 2014-07-28 15:41:41]

On 2014/07/28 15:25:30, Nico (away) wrote:
> The string literal change is fine, but we want to require 4.8. When we use
more
> C++11 soon, 4.7 cannot be supported.

What about common.gypi changes? Could you list C++11 features that you are going
to use that are unsupported by gcc-4.7? I looked at
https://gcc.gnu.org/projects/cxx0x.html and it seems there are just a few. Also,
in my case I can have the build work with warnings (i.e. -Dwerror= ).

Also, sure I understand it wouldn't officially be supported, but if it can
happen to work for me that'd help. Nobody would be required to test with gcc-4.7
or to maintain the gcc-4.7 related parts e.g. on a refactoring.

To simplify the review, please just indicate which changes you're OK with, I can
have remaining ones carried as a distro patch, although I'd prefer to minimize
such a patch.

[Nico, 2014-07-28 15:47:11]

On 2014/07/28 15:41:41, Paweł Hajdan Jr. wrote:
> On 2014/07/28 15:25:30, Nico (away) wrote:
> > The string literal change is fine, but we want to require 4.8. When we use
> more
> > C++11 soon, 4.7 cannot be supported.
> 
> What about common.gypi changes? Could you list C++11 features that you are
going
> to use that are unsupported by gcc-4.7? I looked at
> https://gcc.gnu.org/projects/cxx0x.html and it seems there are just a few.
Also,
> in my case I can have the build work with warnings (i.e. -Dwerror= ).

Narrowing conversions and the string literal suffix thing are hard errors
instead of warnings that can't be disabled, and we can't fix all of these issues
as some are in system headers.

MOJO_ALIGNAS will go away in favor of just alignas().

(And there'll probably a long list of more things, down the road.)

> Also, sure I understand it wouldn't officially be supported, but if it can
> happen to work for me that'd help. Nobody would be required to test with
gcc-4.7
> or to maintain the gcc-4.7 related parts e.g. on a refactoring.

Sure, but if build/common.gypi checks for 47, then folks will believe that it's
supported.

> To simplify the review, please just indicate which changes you're OK with, I
can
> have remaining ones carried as a distro patch, although I'd prefer to minimize
> such a patch.

cpu_info_provider_linux.cc is ok

[Paweł Hajdan Jr., 2014-07-28 17:18:03]

On 2014/07/28 15:47:11, Nico (away) wrote:
> Narrowing conversions and the string literal suffix thing are hard errors
> instead of warnings that can't be disabled, and we can't fix all of these
issues
> as some are in system headers.

Not a problem for a Linux distro. For example, dbus was a non-issue on Gentoo
because it has a way more recent dbus in stable, and even if the version was
broken we can easily apply a patch to fix it.

> MOJO_ALIGNAS will go away in favor of just alignas().

I'm fine with that, is there a reason not to accept the change to it for now? If
it's removed the change will go away as well, and I'm fine with that.

> (And there'll probably a long list of more things, down the road.)

OK. That'd be a separate patch though with more specific changes to discuss, and
I can totally understand the limits of what would be accepted into chromium.

> > Also, sure I understand it wouldn't officially be supported, but if it can
> > happen to work for me that'd help. Nobody would be required to test with
> gcc-4.7
> > or to maintain the gcc-4.7 related parts e.g. on a refactoring.
> 
> Sure, but if build/common.gypi checks for 47, then folks will believe that
it's
> supported.

It also mentions freebsd (just below the block this patch is modifying btw). If
that makes someone believe freebsd is supported, I don't think this 47 change
being made or not would have much effect. Feel free to add me to any thread
where this comes up.

[Nico, 2014-07-28 17:31:49]

Ok, lgtm

[Paweł Hajdan Jr., 2014-07-29 09:48:29]

Thank you, Nico!

Scott, could you do OWNERS review for chrome and mojo?

[Nico, 2014-07-29 13:48:09]

I'm chrome owner
On Jul 29, 2014 2:48 AM, <phajdan.jr@chromium.org> wrote:

> Thank you, Nico!
>
> Scott, could you do OWNERS review for chrome and mojo?
>
> https://codereview.chromium.org/428613003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sky, 2014-07-29 17:38:43]

sky->vtl

[viettrungluu, 2014-07-29 17:47:28]

https://codereview.chromium.org/428613003/diff/1/mojo/public/c/system/macros.h
File mojo/public/c/system/macros.h (right):

https://codereview.chromium.org/428613003/diff/1/mojo/public/c/system/macros....
mojo/public/c/system/macros.h:77: (defined(__clang__) || (__GNUC__ * 10000 +
__GNUC_MINOR__ * 100 >= 40800))
What about future versions of MSVC? This would be another thing to update.

Why is gcc 4.7 claiming to support C++11 if it in fact doesn't?

[Nico, 2014-07-29 18:07:12]

On 2014/07/29 17:47:28, viettrungluu wrote:
> https://codereview.chromium.org/428613003/diff/1/mojo/public/c/system/macros.h
> File mojo/public/c/system/macros.h (right):
> 
>
https://codereview.chromium.org/428613003/diff/1/mojo/public/c/system/macros....
> mojo/public/c/system/macros.h:77: (defined(__clang__) || (__GNUC__ * 10000 +
> __GNUC_MINOR__ * 100 >= 40800))
> What about future versions of MSVC? This would be another thing to update.
> 
> Why is gcc 4.7 claiming to support C++11 if it in fact doesn't?

That define doesn't say "C++11 is supported", it just means "the user passed
-std=c++11".

Pawel said he's fine with this macro just going away altogether in favor of raw
alignas usage as soon as that works everywhere. Since that should be soonish, I
think it doesn't matter too much either way (see discussion upthread)

[viettrungluu, 2014-07-29 18:39:36]

On 2014/07/29 18:07:12, Nico (away) wrote:
> On 2014/07/29 17:47:28, viettrungluu wrote:
> >
https://codereview.chromium.org/428613003/diff/1/mojo/public/c/system/macros.h
> > File mojo/public/c/system/macros.h (right):
> > 
> >
>
https://codereview.chromium.org/428613003/diff/1/mojo/public/c/system/macros....
> > mojo/public/c/system/macros.h:77: (defined(__clang__) || (__GNUC__ * 10000 +
> > __GNUC_MINOR__ * 100 >= 40800))
> > What about future versions of MSVC? This would be another thing to update.
> > 
> > Why is gcc 4.7 claiming to support C++11 if it in fact doesn't?
> 
> That define doesn't say "C++11 is supported", it just means "the user passed
> -std=c++11".
> 
> Pawel said he's fine with this macro just going away altogether in favor of
raw
> alignas usage as soon as that works everywhere. Since that should be soonish,
I
> think it doesn't matter too much either way (see discussion upthread)

This file is part of the mojo public API, which has different constraints:

a) This is actually a C header, which should support as many compilers as
possible (including, e.g., all the NaCl variants).
b) For the mojo C++ bindings, we need to decide whether we're going to require
C++11.

[viettrungluu, 2014-07-29 18:43:30]

On 2014/07/29 18:39:36, viettrungluu wrote:
> On 2014/07/29 18:07:12, Nico (away) wrote:
> > On 2014/07/29 17:47:28, viettrungluu wrote:
> > >
> https://codereview.chromium.org/428613003/diff/1/mojo/public/c/system/macros.h
> > > File mojo/public/c/system/macros.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/428613003/diff/1/mojo/public/c/system/macros....
> > > mojo/public/c/system/macros.h:77: (defined(__clang__) || (__GNUC__ * 10000
+
> > > __GNUC_MINOR__ * 100 >= 40800))
> > > What about future versions of MSVC? This would be another thing to update.
> > > 
> > > Why is gcc 4.7 claiming to support C++11 if it in fact doesn't?
> > 
> > That define doesn't say "C++11 is supported", it just means "the user passed
> > -std=c++11".

Actually, the standard says: "The name __cplusplus is defined to the value
201103L when compiling a C++ translation unit.[*]" with the footnote being "[*]
It is intended that future versions of this standard will replace the value of
this macro with a greater value. Non-conforming
compilers should use a value with at most five decimal digits.".

Since gcc 4.7 is non-conforming, it's wrong for it to define the value of
__cplusplus to 201103L.

> > 
> > Pawel said he's fine with this macro just going away altogether in favor of
> raw
> > alignas usage as soon as that works everywhere. Since that should be
soonish,
> I
> > think it doesn't matter too much either way (see discussion upthread)
> 
> This file is part of the mojo public API, which has different constraints:
> 
> a) This is actually a C header, which should support as many compilers as
> possible (including, e.g., all the NaCl variants).
> b) For the mojo C++ bindings, we need to decide whether we're going to require
> C++11.

[Nico, 2014-07-29 20:36:28]

On Tue, Jul 29, 2014 at 11:43 AM, <viettrungluu@chromium.org> wrote:

> On 2014/07/29 18:39:36, viettrungluu wrote:
>
>> On 2014/07/29 18:07:12, Nico (away) wrote:
>> > On 2014/07/29 17:47:28, viettrungluu wrote:
>> > >
>> https://codereview.chromium.org/428613003/diff/1/mojo/
>> public/c/system/macros.h
>> > > File mojo/public/c/system/macros.h (right):
>> > >
>> > >
>> >
>>
>
> https://codereview.chromium.org/428613003/diff/1/mojo/
> public/c/system/macros.h#newcode77
>
>> > > mojo/public/c/system/macros.h:77: (defined(__clang__) || (__GNUC__ *
>> 10000
>>
> +
>
>> > > __GNUC_MINOR__ * 100 >= 40800))
>> > > What about future versions of MSVC? This would be another thing to
>> update.
>> > >
>> > > Why is gcc 4.7 claiming to support C++11 if it in fact doesn't?
>> >
>> > That define doesn't say "C++11 is supported", it just means "the user
>> passed
>> > -std=c++11".
>>
>
> Actually, the standard says: "The name __cplusplus is defined to the value
> 201103L when compiling a C++ translation unit.[*]" with the footnote being
> "[*]
> It is intended that future versions of this standard will replace the
> value of
> this macro with a greater value. Non-conforming
> compilers should use a value with at most five decimal digits.".
>
> Since gcc 4.7 is non-conforming, it's wrong for it to define the value of
> __cplusplus to 201103L.
>

No compiler gets the standard exactly right, so by that reasoning no
compiler could set __cplusplus to 2011.

In practice, __cplusplus is set exactly if you pass -std=c++11 /
-std=gnu++11.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Paweł Hajdan Jr., 2014-07-30 10:24:30]

On 2014/07/29 18:43:30, viettrungluu wrote:
> Actually, the standard says: "The name __cplusplus is defined to the value
> 201103L when compiling a C++ translation unit.[*]" with the footnote being
"[*]
> It is intended that future versions of this standard will replace the value of
> this macro with a greater value. Non-conforming
> compilers should use a value with at most five decimal digits.".
> 
> Since gcc 4.7 is non-conforming, it's wrong for it to define the value of
> __cplusplus to 201103L.

Trung, thanks for taking a look. What is your specific concern with this patch?

Fixing gcc-4.7 seems out of scope for this, and upstream doesn't work on it
anymore, see e.g. https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61639

I'm fine with distros just having this as a custom patch, but if it can be
upstreamed in chromium I'd like to do that. Is there a specific reason you have
comments about it?

[viettrungluu, 2014-07-30 16:26:15]

On 2014/07/30 10:24:30, Paweł Hajdan Jr. wrote:
> On 2014/07/29 18:43:30, viettrungluu wrote:
> > Actually, the standard says: "The name __cplusplus is defined to the value
> > 201103L when compiling a C++ translation unit.[*]" with the footnote being
> "[*]
> > It is intended that future versions of this standard will replace the value
of
> > this macro with a greater value. Non-conforming
> > compilers should use a value with at most five decimal digits.".
> > 
> > Since gcc 4.7 is non-conforming, it's wrong for it to define the value of
> > __cplusplus to 201103L.
> 
> Trung, thanks for taking a look. What is your specific concern with this
patch?

I said: "What about future versions of MSVC? This would be another thing to
update."

Basically, the change adds a maintenance burden for us.

> 
> Fixing gcc-4.7 seems out of scope for this, and upstream doesn't work on it
> anymore, see e.g. https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61639
> 
> I'm fine with distros just having this as a custom patch, but if it can be
> upstreamed in chromium I'd like to do that. Is there a specific reason you
have
> comments about it?

I imagine that it'll get harder and harder to get Chromium to compile using gcc
4.7, and I'd like to avoid adding accommodations for it.

[Paweł Hajdan Jr., 2014-07-30 17:38:46]

On 2014/07/30 16:26:15, viettrungluu wrote:
> > Trung, thanks for taking a look. What is your specific concern with this
> patch?
> 
> I said: "What about future versions of MSVC? This would be another thing to
> update."

Sorry, maybe I'm just confused, but what does this patch has to do with anything
MSVC?

> I imagine that it'll get harder and harder to get Chromium to compile using
gcc
> 4.7, and I'd like to avoid adding accommodations for it.

This sounds like a speculation. If it becomes infeasible we can remove all
traces of gcc-4.7 related code.

If that's still not convincing I'll just remove the mojo part and land the
remaining one for which I have approval.

[Paweł Hajdan Jr., 2014-07-31 12:55:07]

Committed patchset #2 manually as r286775 (presubmit successful).

[lgombos, 2014-08-10 14:39:17]

On 2014/07/31 12:55:07, Paweł Hajdan Jr. wrote:
> Committed patchset #2 manually as r286775 (presubmit successful).

I found this commit as I am building with gcc 4.7 as well and this change seems
to make the situation worse for me. "Ubuntu 13.04", "gcc (Ubuntu/Linaro
4.7.3-1ubuntu1)" and reverting gcc_version>=47 to gcc_version>=48 helped.

[Paweł Hajdan Jr., 2014-08-11 09:47:56]

On 2014/08/10 14:39:17, lgombos wrote:
> On 2014/07/31 12:55:07, Paweł Hajdan Jr. wrote:
> > Committed patchset #2 manually as r286775 (presubmit successful).
> 
> I found this commit as I am building with gcc 4.7 as well and this change
seems
> to make the situation worse for me. "Ubuntu 13.04", "gcc (Ubuntu/Linaro
> 4.7.3-1ubuntu1)" and reverting gcc_version>=47 to gcc_version>=48 helped.

Please note Ubuntu 13.04 is no longer supported by upstream.

Feel free to post specific errors, but Chromium trunk *requires* C++11 support,
so reverting gcc_version change will result in errors elsewhere that *can't* be
fixed without just removing the C++11 features from the code (not feasible in
the long run).