fix incorrect libstdc++ from GCC >= 5.1 check

fix incorrect libstdc++ from GCC >= 5.1 check

Assume that libstdc++ from GCC >= 5.1 is being used only when compiling with GCC >= 5.1.

This unbreaks GCC 4.8.5 and 4.9.3 builds after https://codereview.chromium.org/1837563002/
landed.

Committed: https://crrev.com/5c8abdc1aa67ebcfa7f2ecbb41506c65fa2ccc36
Cr-Commit-Position: refs/heads/master@{#385416}

[Mostyn Bramley-Moore, 2016-04-05 22:13:47]

mostynb@opera.com changed reviewers:
+ jfb@chromium.org, thakis@chromium.org

[Mostyn Bramley-Moore, 2016-04-05 22:13:48]

@JF, Nico: https://codereview.chromium.org/1837563002/ added an incorrect check
for GCC 5.1.  I have added a placeholder value that checks for GCC 5.2 as a
proof-of-concept for the fix, while I search for the actual value from 5.1.

https://codereview.chromium.org/1863523005/diff/1/base/bit_cast.h
File base/bit_cast.h (left):

https://codereview.chromium.org/1863523005/diff/1/base/bit_cast.h#oldcode67
base/bit_cast.h:67: #if ((defined(__GLIBCXX__) && (__GLIBCXX__ >= 20150422)) ||
\
This is the value from G++ 4.8, I believe.

https://codereview.chromium.org/1863523005/diff/1/base/bit_cast.h
File base/bit_cast.h (right):

https://codereview.chromium.org/1863523005/diff/1/base/bit_cast.h#newcode67
base/bit_cast.h:67: #if ((defined(__GLIBCXX__) && (__GLIBCXX__ >= 20151010ul))
|| \
This is actually the value from G++ 5.2, just a placeholder until I can find the
value corresponding to 5.1.

[JF, 2016-04-05 22:21:30]

https://codereview.chromium.org/1863523005/diff/1/base/bit_cast.h
File base/bit_cast.h (right):

https://codereview.chromium.org/1863523005/diff/1/base/bit_cast.h#newcode67
base/bit_cast.h:67: #if ((defined(__GLIBCXX__) && (__GLIBCXX__ >= 20151010ul))
|| \
On 2016/04/05 22:13:48, Mostyn Bramley-Moore wrote:
> This is actually the value from G++ 5.2, just a placeholder until I can find
the
> value corresponding to 5.1.

IIRC the docs are:

From: https://gcc.gnu.org/onlinedocs/libstdc++/manual/abi.html

  Incremental bumping of a library pre-defined macro. For releases before 3.4.0,
the macro is __GLIBCPP__. For later releases, it's __GLIBCXX__. (The libstdc++
project generously changed from CPP to CXX throughout its source to allow the
"C" pre-processor the CPP macro namespace.) These macros are defined as the date
the library was released, in compressed ISO date format, as an unsigned long.

  This macro is defined in the file "c++config" in the
"libstdc++-v3/include/bits" directory. (Up to GCC 4.1.0, it was changed every
night by an automated script. Since GCC 4.1.0, it is the same value as
gcc/DATESTAMP.)


And from: https://gcc.gnu.org/releases.html

  GCC 5.1	April 22, 2015


It sounds like that doesn't work for you?

[Mostyn Bramley-Moore, 2016-04-05 22:26:05]

https://codereview.chromium.org/1863523005/diff/1/base/bit_cast.h
File base/bit_cast.h (right):

https://codereview.chromium.org/1863523005/diff/1/base/bit_cast.h#newcode67
base/bit_cast.h:67: #if ((defined(__GLIBCXX__) && (__GLIBCXX__ >= 20151010ul))
|| \
On 2016/04/05 22:21:29, JF wrote:
> On 2016/04/05 22:13:48, Mostyn Bramley-Moore wrote:
> > This is actually the value from G++ 5.2, just a placeholder until I can find
> the
> > value corresponding to 5.1.
> 
> IIRC the docs are:
> 
> From: https://gcc.gnu.org/onlinedocs/libstdc++/manual/abi.html
> 
>   Incremental bumping of a library pre-defined macro. For releases before
3.4.0,
> the macro is __GLIBCPP__. For later releases, it's __GLIBCXX__. (The libstdc++
> project generously changed from CPP to CXX throughout its source to allow the
> "C" pre-processor the CPP macro namespace.) These macros are defined as the
date
> the library was released, in compressed ISO date format, as an unsigned long.
> 
>   This macro is defined in the file "c++config" in the
> "libstdc++-v3/include/bits" directory. (Up to GCC 4.1.0, it was changed every
> night by an automated script. Since GCC 4.1.0, it is the same value as
> gcc/DATESTAMP.)
> 
> 
> And from: https://gcc.gnu.org/releases.html
> 
>   GCC 5.1	April 22, 2015
> 
> 
> It sounds like that doesn't work for you?

gcc/DATESTAMP in the GCC 4.8.5 tarball is 20150623, larger than the value from
the GCC 5.1 tarball.

[JF, 2016-04-05 22:38:03]

On 2016/04/05 22:26:05, Mostyn Bramley-Moore wrote:
> https://codereview.chromium.org/1863523005/diff/1/base/bit_cast.h
> File base/bit_cast.h (right):
> 
> https://codereview.chromium.org/1863523005/diff/1/base/bit_cast.h#newcode67
> base/bit_cast.h:67: #if ((defined(__GLIBCXX__) && (__GLIBCXX__ >= 20151010ul))
> || \
> On 2016/04/05 22:21:29, JF wrote:
> > On 2016/04/05 22:13:48, Mostyn Bramley-Moore wrote:
> > > This is actually the value from G++ 5.2, just a placeholder until I can
find
> > the
> > > value corresponding to 5.1.
> > 
> > IIRC the docs are:
> > 
> > From: https://gcc.gnu.org/onlinedocs/libstdc++/manual/abi.html
> > 
> >   Incremental bumping of a library pre-defined macro. For releases before
> 3.4.0,
> > the macro is __GLIBCPP__. For later releases, it's __GLIBCXX__. (The
libstdc++
> > project generously changed from CPP to CXX throughout its source to allow
the
> > "C" pre-processor the CPP macro namespace.) These macros are defined as the
> date
> > the library was released, in compressed ISO date format, as an unsigned
long.
> > 
> >   This macro is defined in the file "c++config" in the
> > "libstdc++-v3/include/bits" directory. (Up to GCC 4.1.0, it was changed
every
> > night by an automated script. Since GCC 4.1.0, it is the same value as
> > gcc/DATESTAMP.)
> > 
> > 
> > And from: https://gcc.gnu.org/releases.html
> > 
> >   GCC 5.1	April 22, 2015
> > 
> > 
> > It sounds like that doesn't work for you?
> 
> gcc/DATESTAMP in the GCC 4.8.5 tarball is 20150623, larger than the value from
> the GCC 5.1 tarball.

lulwut? The date you picked sounds conservatively fine then, it doesn't need to
be super precise.

[Mostyn Bramley-Moore, 2016-04-05 22:43:02]

> > gcc/DATESTAMP in the GCC 4.8.5 tarball is 20150623, larger than the value
from
> > the GCC 5.1 tarball.
> 
> lulwut? The date you picked sounds conservatively fine then, it doesn't need
to
> be super precise.

I looks like __GLIBCXX__ is not reliable for anything except the release date of
that very specific version of GCC.  Note that GCC 4.8.5 and 4.9.3 where both
released after GCC 5.1, and therefore have higher values of __GLIBCXX__:
https://gcc.gnu.org/releases.html

IMO it would be better to check for something like:
__GNUC__ > 5 || (__GNUC__ == 5 && __GNUC_MINOR__ >= 1)

That way, if there's a gcc 4.8.6 release later, this won't break.

[Mostyn Bramley-Moore, 2016-04-05 22:46:54]

Description was changed from

==========
fix incorrect GCC >= 5.* check

This unbreaks GCC 4.8 builds after https://codereview.chromium.org/1837563002/
landed.
==========

to

==========
fix incorrect GCC >= 5.1 check

This unbreaks GCC 4.8.5 and 4.9.3 builds after
https://codereview.chromium.org/1837563002/
landed.
==========

[JF, 2016-04-05 22:48:34]

On 2016/04/05 22:43:02, Mostyn Bramley-Moore wrote:
> > > gcc/DATESTAMP in the GCC 4.8.5 tarball is 20150623, larger than the value
> from
> > > the GCC 5.1 tarball.
> > 
> > lulwut? The date you picked sounds conservatively fine then, it doesn't need
> to
> > be super precise.
> 
> I looks like __GLIBCXX__ is not reliable for anything except the release date
of
> that very specific version of GCC.  Note that GCC 4.8.5 and 4.9.3 where both
> released after GCC 5.1, and therefore have higher values of __GLIBCXX__:
> https://gcc.gnu.org/releases.html
> 
> IMO it would be better to check for something like:
> __GNUC__ > 5 || (__GNUC__ == 5 && __GNUC_MINOR__ >= 1)
> 
> That way, if there's a gcc 4.8.6 release later, this won't break.

That sounds good to me, and indeed more precise. I was thinking that I wanted to
match the stdlib version (so that clang with libstdc++ would work) but the elif
will catch that case so your fix sounds fine.

[Mostyn Bramley-Moore, 2016-04-05 22:57:02]

On 2016/04/05 22:48:34, JF wrote:
> That sounds good to me, and indeed more precise. I was thinking that I wanted
to
> match the stdlib version (so that clang with libstdc++ would work) but the
elif
> will catch that case so your fix sounds fine.

Your reasoning makes sense, it's the GCC convention that is strange in this
case.

[Mostyn Bramley-Moore, 2016-04-05 22:57:11]

The CQ bit was checked by mostynb@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-04-05 22:57:32]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1863523005/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1863523005/20001

[JF, 2016-04-05 23:06:01]

Great, lgtm and sorry for the breakage! You'll need Nico to sign off on base
though.

[commit-bot: I haz the power, 2016-04-06 00:28:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-06 00:28:05]

Dry run: This issue passed the CQ dry run.

[michaelpg, 2016-04-06 00:28:18]

On 2016/04/05 23:06:01, JF wrote:
> Great, lgtm and sorry for the breakage! You'll need Nico to sign off on base
> though.

seems nico is away until friday. this is breaking local builds, would it make
sense for another /base OWNER to review?

[Nico, 2016-04-06 00:39:41]

lgtm to unbreak your build, but the change is wrong as far as i can tell:

https://codereview.chromium.org/1863523005/diff/20001/base/bit_cast.h
File base/bit_cast.h (right):

https://codereview.chromium.org/1863523005/diff/20001/base/bit_cast.h#newcode67
base/bit_cast.h:67: #if (__GNUC__ > 5 || (__GNUC__ == 5 && __GNUC_MINOR__ >= 1))
this drops the _libcpp_version check; that's probably not intentional?

also, this breaks clang with libstdc++ too, no?

[Alexander Alekseev, 2016-04-06 03:56:39]

On 2016/04/06 00:39:41, Nico (hiding until Fri) wrote:
> lgtm to unbreak your build, but the change is wrong as far as i can tell:
> 
> https://codereview.chromium.org/1863523005/diff/20001/base/bit_cast.h
> File base/bit_cast.h (right):
> 
>
https://codereview.chromium.org/1863523005/diff/20001/base/bit_cast.h#newcode67
> base/bit_cast.h:67: #if (__GNUC__ > 5 || (__GNUC__ == 5 && __GNUC_MINOR__ >=
1))
> this drops the _libcpp_version check; that's probably not intentional?
> 
> also, this breaks clang with libstdc++ too, no?

It looks, clang build is also broken:
FAILED: .../third_party/llvm-build/Release+Asserts/bin/clang++ ...
..
bit_cast.h:71:22: error: no member named 'is_trivially_copyable' in namespace
'std'

[Alexander Alekseev, 2016-04-06 03:58:18]

On 2016/04/06 03:56:39, Alexander Alekseev wrote:
> On 2016/04/06 00:39:41, Nico (hiding until Fri) wrote:
> > lgtm to unbreak your build, but the change is wrong as far as i can tell:
> > 
> > https://codereview.chromium.org/1863523005/diff/20001/base/bit_cast.h
> > File base/bit_cast.h (right):
> > 
> >
>
https://codereview.chromium.org/1863523005/diff/20001/base/bit_cast.h#newcode67
> > base/bit_cast.h:67: #if (__GNUC__ > 5 || (__GNUC__ == 5 && __GNUC_MINOR__ >=
> 1))
> > this drops the _libcpp_version check; that's probably not intentional?
> > 
> > also, this breaks clang with libstdc++ too, no?
> 
> It looks, clang build is also broken:
> FAILED: .../third_party/llvm-build/Release+Asserts/bin/clang++ ...
> ..
> bit_cast.h:71:22: error: no member named 'is_trivially_copyable' in namespace
> 'std'

I mean, clang build is broken _now_ . Without this patch.

Can we just roll back the previous CL? :
---------------------------
commit d81c1ced74e63e4bf50019ffd2d398def71bdae7
Author: jfb <jfb@chromium.org>
Date:   Tue Apr 5 13:50:35 2016 -0700

    bit_cast: check is_trivially_copyable
----------------------------

[JF, 2016-04-06 05:20:37]

On 2016/04/06 00:39:41, Nico (hiding until Fri) wrote:
> lgtm to unbreak your build, but the change is wrong as far as i can tell:
> 
> https://codereview.chromium.org/1863523005/diff/20001/base/bit_cast.h
> File base/bit_cast.h (right):
> 
>
https://codereview.chromium.org/1863523005/diff/20001/base/bit_cast.h#newcode67
> base/bit_cast.h:67: #if (__GNUC__ > 5 || (__GNUC__ == 5 && __GNUC_MINOR__ >=
1))
> this drops the _libcpp_version check; that's probably not intentional?
> 
> also, this breaks clang with libstdc++ too, no?

I think this is fine:
* __GNUC__ is used as a proxy for GCC+libstdc++ detection because they're
usually tied. We know that version of "GCC" will have the right libstdc++.
* The next fallback __has_feature(is_trivially_copyable) checks whether the
compiler supports the intrinsic, which catches clang regardless which stdlib
it's using.
* Then we fall back to COMPILER_GCC and the older intrinsic for older GCC.
* Then we rely on bots for other configurations, which is what we had before
this change anyways.

[Mostyn Bramley-Moore, 2016-04-06 07:39:44]

The CQ bit was checked by mostynb@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-04-06 07:40:01]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1863523005/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1863523005/40001

[Mostyn Bramley-Moore, 2016-04-06 07:41:50]

Description was changed from

==========
fix incorrect GCC >= 5.1 check

This unbreaks GCC 4.8.5 and 4.9.3 builds after
https://codereview.chromium.org/1837563002/
landed.
==========

to

==========
fix incorrect libstdc++ from GCC >= 5.1 check

Assume that libstdc++ from GCC >= 5.1 is being used only when compiling with GCC
>= 5.1.

This unbreaks GCC 4.8.5 and 4.9.3 builds after
https://codereview.chromium.org/1837563002/
landed.
==========

[commit-bot: I haz the power, 2016-04-06 07:44:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-06 07:44:10]

Dry run: Try jobs failed on following builders:
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[Mostyn Bramley-Moore, 2016-04-06 07:47:43]

The CQ bit was checked by mostynb@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-04-06 07:47:59]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1863523005/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1863523005/60001

[Mostyn Bramley-Moore, 2016-04-06 07:54:23]

The only way that I can see to reliably check the libstdc++ version is to check
for specific values corresponding to known GCC releases.  ie something like:

#if ((__GLIBCXX__ == 20150422ul) || /* GCC 5.1.0 */
     (__GLIBCXX__ == 20150716ul) || /* GCC 5.2.0 */
     (__GLIBCXX__ == 20151204ul))   /* GCC 5.3.0 */
...
#endif

But this would need to be updated for each new GCC version.

https://codereview.chromium.org/1863523005/diff/20001/base/bit_cast.h
File base/bit_cast.h (right):

https://codereview.chromium.org/1863523005/diff/20001/base/bit_cast.h#newcode67
base/bit_cast.h:67: #if (__GNUC__ > 5 || (__GNUC__ == 5 && __GNUC_MINOR__ >= 1))
On 2016/04/06 00:39:40, Nico (hiding until Fri) wrote:
> this drops the _libcpp_version check; that's probably not intentional?
> 
> also, this breaks clang with libstdc++ too, no?

Right- added this back.

[commit-bot: I haz the power, 2016-04-06 09:17:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-06 09:17:06]

Dry run: This issue passed the CQ dry run.

[Mostyn Bramley-Moore, 2016-04-06 09:18:35]

The CQ bit was checked by mostynb@opera.com

[Mostyn Bramley-Moore, 2016-04-06 09:18:35]

The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org,
jfb@chromium.org
Link to the patchset: https://codereview.chromium.org/1863523005/#ps60001
(title: "fix ios builds")

[commit-bot: I haz the power, 2016-04-06 09:18:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1863523005/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1863523005/60001

[Mostyn Bramley-Moore, 2016-04-06 09:19:36]

Queued this, I hope it helps.

@Alexander: what is your toolchain/configuration?

[commit-bot: I haz the power, 2016-04-06 09:24:01]

Description was changed from

==========
fix incorrect libstdc++ from GCC >= 5.1 check

Assume that libstdc++ from GCC >= 5.1 is being used only when compiling with GCC
>= 5.1.

This unbreaks GCC 4.8.5 and 4.9.3 builds after
https://codereview.chromium.org/1837563002/
landed.
==========

to

==========
fix incorrect libstdc++ from GCC >= 5.1 check

Assume that libstdc++ from GCC >= 5.1 is being used only when compiling with GCC
>= 5.1.

This unbreaks GCC 4.8.5 and 4.9.3 builds after
https://codereview.chromium.org/1837563002/
landed.
==========

[commit-bot: I haz the power, 2016-04-06 09:24:02]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-04-06 09:25:38]

Description was changed from

==========
fix incorrect libstdc++ from GCC >= 5.1 check

Assume that libstdc++ from GCC >= 5.1 is being used only when compiling with GCC
>= 5.1.

This unbreaks GCC 4.8.5 and 4.9.3 builds after
https://codereview.chromium.org/1837563002/
landed.
==========

to

==========
fix incorrect libstdc++ from GCC >= 5.1 check

Assume that libstdc++ from GCC >= 5.1 is being used only when compiling with GCC
>= 5.1.

This unbreaks GCC 4.8.5 and 4.9.3 builds after
https://codereview.chromium.org/1837563002/
landed.

Committed: https://crrev.com/5c8abdc1aa67ebcfa7f2ecbb41506c65fa2ccc36
Cr-Commit-Position: refs/heads/master@{#385416}
==========

[commit-bot: I haz the power, 2016-04-06 09:25:40]

Patchset 4 (id:??) landed as
https://crrev.com/5c8abdc1aa67ebcfa7f2ecbb41506c65fa2ccc36
Cr-Commit-Position: refs/heads/master@{#385416}