You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Benjamin Bannier <be...@mesosphere.io> on 2017/01/21 00:23:18 UTC
Re: Review Request 55771: Rejected optimizing if we are working with a
libcxx with UB.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55771/
-----------------------------------------------------------
(Updated Jan. 21, 2017, 1:23 a.m.)
Review request for mesos and Till Toenshoff.
Changes
-------
Turned warning into error.
Summary (updated)
-----------------
Rejected optimizing if we are working with a libcxx with UB.
Bugs: MESOS-6606
https://issues.apache.org/jira/browse/MESOS-6606
Repository: mesos
Description (updated)
-------
See summary.
Diffs (updated)
-----
configure.ac 60ffa0e6a0dc590af929ab8a011b6d78f82a48e0
Diff: https://reviews.apache.org/r/55771/diff/
Testing
-------
Tested on OS X Sierra with both bundled clang+stdlib and versions close to their HEADs with
$ autoreconf -fi && ./configure CXX=/usr/bin/clang++
$ autoreconf -fi && ./configure CXX=clang++ # versions from HEAD
$ autoreconf -fi && ./configure --enable-optimize CXX=/usr/bin/clang++
$ autoreconf -fi && ./configure --enable-optimize CXX=clang++ # versions from HEAD
Thanks,
Benjamin Bannier
Re: Review Request 55771: Rejected optimizing if we are working with a
libcxx with UB.
Posted by Till Toenshoff <to...@me.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55771/#review162802
-----------------------------------------------------------
Ship it!
Ship It!
- Till Toenshoff
On Jan. 23, 2017, 11:33 a.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55771/
> -----------------------------------------------------------
>
> (Updated Jan. 23, 2017, 11:33 a.m.)
>
>
> Review request for mesos and Till Toenshoff.
>
>
> Bugs: MESOS-6606
> https://issues.apache.org/jira/browse/MESOS-6606
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> configure.ac 60ffa0e6a0dc590af929ab8a011b6d78f82a48e0
>
> Diff: https://reviews.apache.org/r/55771/diff/
>
>
> Testing
> -------
>
> Tested on OS X Sierra with both bundled clang+stdlib and versions close to their HEADs with
>
> $ autoreconf -fi && ./configure CXX=/usr/bin/clang++
> $ autoreconf -fi && ./configure CXX=clang++ # versions from HEAD
>
> $ autoreconf -fi && ./configure --enable-optimize CXX=/usr/bin/clang++
> $ autoreconf -fi && ./configure --enable-optimize CXX=clang++ # versions from HEAD
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 55771: Rejected optimizing if we are working with a
libcxx with UB.
Posted by Till Toenshoff <to...@me.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55771/#review164622
-----------------------------------------------------------
configure.ac
<https://reviews.apache.org/r/55771/#comment236390>
Lets keep this one.
- Till Toenshoff
On Feb. 7, 2017, 7:49 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55771/
> -----------------------------------------------------------
>
> (Updated Feb. 7, 2017, 7:49 p.m.)
>
>
> Review request for mesos and Till Toenshoff.
>
>
> Bugs: MESOS-6606
> https://issues.apache.org/jira/browse/MESOS-6606
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> CHANGELOG 1478ad4dd3b20fc768fd679f81d803acf0995ad2
> configure.ac 60ffa0e6a0dc590af929ab8a011b6d78f82a48e0
> src/launcher/default_executor.cpp 8e942c6faf6e2dbc70a6fff1629343c9a3ce8668
> src/oci/spec.cpp 0a88edb404836205f11e61ba3edb771df8b618ef
> src/tests/default_executor_tests.cpp 1afab8292c7fc15bd16bee4bcd93ce290a233c9a
>
> Diff: https://reviews.apache.org/r/55771/diff/
>
>
> Testing
> -------
>
> Tested on OS X Sierra with both bundled clang+stdlib and versions close to their HEADs with
>
> $ autoreconf -fi && ./configure CXX=/usr/bin/clang++
> $ autoreconf -fi && ./configure CXX=clang++ # versions from HEAD
>
> $ autoreconf -fi && ./configure --enable-optimize CXX=/usr/bin/clang++
> $ autoreconf -fi && ./configure --enable-optimize CXX=clang++ # versions from HEAD
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 55771: Rejected optimizing if we are working with a
libcxx with UB.
Posted by Till Toenshoff <to...@me.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55771/#review164621
-----------------------------------------------------------
Rebase please.
- Till Toenshoff
On Feb. 7, 2017, 7:49 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55771/
> -----------------------------------------------------------
>
> (Updated Feb. 7, 2017, 7:49 p.m.)
>
>
> Review request for mesos and Till Toenshoff.
>
>
> Bugs: MESOS-6606
> https://issues.apache.org/jira/browse/MESOS-6606
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> CHANGELOG 1478ad4dd3b20fc768fd679f81d803acf0995ad2
> configure.ac 60ffa0e6a0dc590af929ab8a011b6d78f82a48e0
> src/launcher/default_executor.cpp 8e942c6faf6e2dbc70a6fff1629343c9a3ce8668
> src/oci/spec.cpp 0a88edb404836205f11e61ba3edb771df8b618ef
> src/tests/default_executor_tests.cpp 1afab8292c7fc15bd16bee4bcd93ce290a233c9a
>
> Diff: https://reviews.apache.org/r/55771/diff/
>
>
> Testing
> -------
>
> Tested on OS X Sierra with both bundled clang+stdlib and versions close to their HEADs with
>
> $ autoreconf -fi && ./configure CXX=/usr/bin/clang++
> $ autoreconf -fi && ./configure CXX=clang++ # versions from HEAD
>
> $ autoreconf -fi && ./configure --enable-optimize CXX=/usr/bin/clang++
> $ autoreconf -fi && ./configure --enable-optimize CXX=clang++ # versions from HEAD
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 55771: Rejected optimizing if we are working with a
libcxx with UB.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55771/
-----------------------------------------------------------
(Updated Feb. 8, 2017, 7:54 a.m.)
Review request for mesos and Till Toenshoff.
Changes
-------
Rebased.
Bugs: MESOS-6606
https://issues.apache.org/jira/browse/MESOS-6606
Repository: mesos
Description
-------
See summary.
Diffs (updated)
-----
configure.ac 60ffa0e6a0dc590af929ab8a011b6d78f82a48e0
Diff: https://reviews.apache.org/r/55771/diff/
Testing
-------
Tested on OS X Sierra with both bundled clang+stdlib and versions close to their HEADs with
$ autoreconf -fi && ./configure CXX=/usr/bin/clang++
$ autoreconf -fi && ./configure CXX=clang++ # versions from HEAD
$ autoreconf -fi && ./configure --enable-optimize CXX=/usr/bin/clang++
$ autoreconf -fi && ./configure --enable-optimize CXX=clang++ # versions from HEAD
Thanks,
Benjamin Bannier
Re: Review Request 55771: Rejected optimizing if we are working with a
libcxx with UB.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55771/
-----------------------------------------------------------
(Updated Feb. 7, 2017, 8:49 p.m.)
Review request for mesos and Till Toenshoff.
Changes
-------
Used idiomatic way to output progress.
Bugs: MESOS-6606
https://issues.apache.org/jira/browse/MESOS-6606
Repository: mesos
Description
-------
See summary.
Diffs (updated)
-----
CHANGELOG 1478ad4dd3b20fc768fd679f81d803acf0995ad2
configure.ac 60ffa0e6a0dc590af929ab8a011b6d78f82a48e0
src/launcher/default_executor.cpp 8e942c6faf6e2dbc70a6fff1629343c9a3ce8668
src/oci/spec.cpp 0a88edb404836205f11e61ba3edb771df8b618ef
src/tests/default_executor_tests.cpp 1afab8292c7fc15bd16bee4bcd93ce290a233c9a
Diff: https://reviews.apache.org/r/55771/diff/
Testing
-------
Tested on OS X Sierra with both bundled clang+stdlib and versions close to their HEADs with
$ autoreconf -fi && ./configure CXX=/usr/bin/clang++
$ autoreconf -fi && ./configure CXX=clang++ # versions from HEAD
$ autoreconf -fi && ./configure --enable-optimize CXX=/usr/bin/clang++
$ autoreconf -fi && ./configure --enable-optimize CXX=clang++ # versions from HEAD
Thanks,
Benjamin Bannier
Re: Review Request 55771: Rejected optimizing if we are working with a
libcxx with UB.
Posted by Benjamin Bannier <be...@mesosphere.io>.
> On Feb. 7, 2017, 6:32 p.m., Till Toenshoff wrote:
> > configure.ac, line 1892
> > <https://reviews.apache.org/r/55771/diff/3/?file=1612483#file1612483line1892>
> >
> > Sorry for not noticing ealier but I believe the idiomatic autotools way here is to use a pair of `AC_MSG_CHECKING([string])`, `AC_MSG_RESULT([string])`. We could then have the failure message follow that result; something along these lines, I guess....
> >
> > ```
> > AC_MSG_CHECKING([C++ standard library has undefined behaviour with selected optimization level])
> > AC_LANG_PUSH([C++])
> > AC_RUN_IFELSE([
> > AC_LANG_SOURCE([[
> > [...]
> > ]])],
> > [message='no'], [message='yes']
> > )
> > AC_MSG_RESULT([$message])
> >
> > if test "x$message" = "xyes"; then
> > AC_MSG_ERROR([Mesos cannot be built with optimizations against this version of libcxx (MESOS-5745).
> > Consider building without optimizations, or changing the used C++ standard library.])
> > fi
> > ```
> >
> > ...which would look like this;
> >
> > ```
> > Checking C++ standard library has undefined behaviour with selected optimization level... no
> > ```
Very good point. I updated the code to use `AC_MSG_CHECKING`/`AC_MSG_RESULT`, but did not introduce additional branching (via raw shell). Fixed here and in following patches.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55771/#review164527
-----------------------------------------------------------
On Feb. 7, 2017, 8:49 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55771/
> -----------------------------------------------------------
>
> (Updated Feb. 7, 2017, 8:49 p.m.)
>
>
> Review request for mesos and Till Toenshoff.
>
>
> Bugs: MESOS-6606
> https://issues.apache.org/jira/browse/MESOS-6606
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> CHANGELOG 1478ad4dd3b20fc768fd679f81d803acf0995ad2
> configure.ac 60ffa0e6a0dc590af929ab8a011b6d78f82a48e0
> src/launcher/default_executor.cpp 8e942c6faf6e2dbc70a6fff1629343c9a3ce8668
> src/oci/spec.cpp 0a88edb404836205f11e61ba3edb771df8b618ef
> src/tests/default_executor_tests.cpp 1afab8292c7fc15bd16bee4bcd93ce290a233c9a
>
> Diff: https://reviews.apache.org/r/55771/diff/
>
>
> Testing
> -------
>
> Tested on OS X Sierra with both bundled clang+stdlib and versions close to their HEADs with
>
> $ autoreconf -fi && ./configure CXX=/usr/bin/clang++
> $ autoreconf -fi && ./configure CXX=clang++ # versions from HEAD
>
> $ autoreconf -fi && ./configure --enable-optimize CXX=/usr/bin/clang++
> $ autoreconf -fi && ./configure --enable-optimize CXX=clang++ # versions from HEAD
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 55771: Rejected optimizing if we are working with a
libcxx with UB.
Posted by Till Toenshoff <to...@me.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55771/#review164527
-----------------------------------------------------------
configure.ac (line 1891)
<https://reviews.apache.org/r/55771/#comment236289>
Sorry for not noticing ealier but I believe the idiomatic autotools way here is to use a pair of `AC_MSG_CHECKING([string])`, `AC_MSG_RESULT([string])`. We could then have the failure message follow that result; something along these lines, I guess....
```
AC_MSG_CHECKING([C++ standard library has undefined behaviour with selected optimization level])
AC_LANG_PUSH([C++])
AC_RUN_IFELSE([
AC_LANG_SOURCE([[
[...]
]])],
[message='no'], [message='yes']
)
AC_MSG_RESULT([$message])
if test "x$message" = "xyes"; then
AC_MSG_ERROR([Mesos cannot be built with optimizations against this version of libcxx (MESOS-5745).
Consider building without optimizations, or changing the used C++ standard library.])
fi
```
...which would look like this;
```
Checking C++ standard library has undefined behaviour with selected optimization level... no
```
- Till Toenshoff
On Jan. 23, 2017, 11:33 a.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55771/
> -----------------------------------------------------------
>
> (Updated Jan. 23, 2017, 11:33 a.m.)
>
>
> Review request for mesos and Till Toenshoff.
>
>
> Bugs: MESOS-6606
> https://issues.apache.org/jira/browse/MESOS-6606
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> configure.ac 60ffa0e6a0dc590af929ab8a011b6d78f82a48e0
>
> Diff: https://reviews.apache.org/r/55771/diff/
>
>
> Testing
> -------
>
> Tested on OS X Sierra with both bundled clang+stdlib and versions close to their HEADs with
>
> $ autoreconf -fi && ./configure CXX=/usr/bin/clang++
> $ autoreconf -fi && ./configure CXX=clang++ # versions from HEAD
>
> $ autoreconf -fi && ./configure --enable-optimize CXX=/usr/bin/clang++
> $ autoreconf -fi && ./configure --enable-optimize CXX=clang++ # versions from HEAD
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 55771: Rejected optimizing if we are working with a
libcxx with UB.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55771/
-----------------------------------------------------------
(Updated Jan. 23, 2017, 12:33 p.m.)
Review request for mesos and Till Toenshoff.
Changes
-------
Make whitespace more awesome.
Bugs: MESOS-6606
https://issues.apache.org/jira/browse/MESOS-6606
Repository: mesos
Description
-------
See summary.
Diffs (updated)
-----
configure.ac 60ffa0e6a0dc590af929ab8a011b6d78f82a48e0
Diff: https://reviews.apache.org/r/55771/diff/
Testing
-------
Tested on OS X Sierra with both bundled clang+stdlib and versions close to their HEADs with
$ autoreconf -fi && ./configure CXX=/usr/bin/clang++
$ autoreconf -fi && ./configure CXX=clang++ # versions from HEAD
$ autoreconf -fi && ./configure --enable-optimize CXX=/usr/bin/clang++
$ autoreconf -fi && ./configure --enable-optimize CXX=clang++ # versions from HEAD
Thanks,
Benjamin Bannier