|
|
Created:
3 years, 6 months ago by Tom Anderson Modified:
3 years, 5 months ago CC:
chromium-reviews, Lei Zhang, michaelpg Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionSwitch Chrome on Linux to libc++
Design doc detailing the changes at [1], and announcement to
chromium-dev at [2].
In addition, this CL updates perf_expectations to increase the number
of allowed static initializers. libc++ adds some initializers from
iostream.cpp. However, this case is net-neutral since the initializer
used to live in libstdc++, but has been pushed into the chrome and
nacl executables. The perf_expectations change also required adding
NOPRESUBMIT=true to silence a bogus warning.
[1] https://docs.google.com/document/d/1zmHUXlpGNXB433wHnr40dLwj7c-USsIVlyAeH7vOI9o/edit#heading=h.bfky2xssferh
[2] https://groups.google.com/a/chromium.org/forum/?fromgroups#!topic/chromium-dev/z2YnFCxq0MA
BUG=593874
R=thakis@chromium.org
TBR=rdevlin.cronin@chromium.org
CC=thestig@chromium.org
NOPRESUBMIT=true
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_dbg_ng;master.tryserver.chromium.linux:linux_arm;master.tryserver.chromium.linux:linux_chromium_archive_rel_ng;master.tryserver.chromium.linux:linux_chromium_dbg_32_ng
Review-Url: https://codereview.chromium.org/2933573002
Cr-Commit-Position: refs/heads/master@{#483498}
Committed: https://chromium.googlesource.com/chromium/src/+/39fcd6af4d8c9ddcf909b8d489c6ba684d046157
Patch Set 1 #Patch Set 2 : Rebase #Patch Set 3 : Enable on official builds too #Patch Set 4 : time to laaaand this #
Total comments: 5
Patch Set 5 : Simplify use_custom_libcxx conditions #
Messages
Total messages: 70 (59 generated)
Description was changed from ========== [DO NOT LAND] Test enabling libc++ on non-instrumented Linux builds BUG=593874 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_dbg_ng ========== to ========== Switch Chrome on Linux to libc++ BUG=593874 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_dbg_ng;master.tryserver.chromium.linux:linux_arm;master.tryserver.chromium.linux:linux_chromium_archive_rel_ng;master.tryserver.chromium.linux:linux_chromium_dbg_32_ng NOPRESUBMIT=true R=thakis@chromium.org ==========
Description was changed from ========== Switch Chrome on Linux to libc++ BUG=593874 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_dbg_ng;master.tryserver.chromium.linux:linux_arm;master.tryserver.chromium.linux:linux_chromium_archive_rel_ng;master.tryserver.chromium.linux:linux_chromium_dbg_32_ng NOPRESUBMIT=true R=thakis@chromium.org ========== to ========== Switch Chrome on Linux to libc++ BUG=593874 R=thakis@chromium.org,thestig@chromium.org,brettw@chromium.org NOPRESUBMIT=true CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_dbg_ng;master.tryserver.chromium.linux:linux_arm;master.tryserver.chromium.linux:linux_chromium_archive_rel_ng;master.tryserver.chromium.linux:linux_chromium_dbg_32_ng ==========
Description was changed from ========== Switch Chrome on Linux to libc++ BUG=593874 R=thakis@chromium.org,thestig@chromium.org,brettw@chromium.org NOPRESUBMIT=true CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_dbg_ng;master.tryserver.chromium.linux:linux_arm;master.tryserver.chromium.linux:linux_chromium_archive_rel_ng;master.tryserver.chromium.linux:linux_chromium_dbg_32_ng ========== to ========== Switch Chrome on Linux to libc++ BUG=593874 R=thakis@chromium.org,thestig@chromium.org,brettw@chromium.org NOPRESUBMIT=true CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_dbg_ng;master.tryserver.chromium.linux:linux_arm;master.tryserver.chromium.linux:linux_chromium_archive_rel_ng;master.tryserver.chromium.linux:linux_chromium_dbg_32_ng ==========
thomasanderson@chromium.org changed reviewers: + brettw@chromium.org, thakis@chromium.org, thestig@chromium.org
Description was changed from ========== Switch Chrome on Linux to libc++ BUG=593874 R=thakis@chromium.org,thestig@chromium.org,brettw@chromium.org NOPRESUBMIT=true CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_dbg_ng;master.tryserver.chromium.linux:linux_arm;master.tryserver.chromium.linux:linux_chromium_archive_rel_ng;master.tryserver.chromium.linux:linux_chromium_dbg_32_ng ========== to ========== Switch Chrome on Linux to libc++ BUG=593874 R=thakis@chromium.org,thestig@chromium.org,brettw@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_dbg_ng;master.tryserver.chromium.linux:linux_arm;master.tryserver.chromium.linux:linux_chromium_archive_rel_ng;master.tryserver.chromium.linux:linux_chromium_dbg_32_ng ==========
Patchset #3 (id:40001) has been deleted
The CQ bit was checked by thomasanderson@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Description was changed from ========== Switch Chrome on Linux to libc++ BUG=593874 R=thakis@chromium.org,thestig@chromium.org,brettw@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_dbg_ng;master.tryserver.chromium.linux:linux_arm;master.tryserver.chromium.linux:linux_chromium_archive_rel_ng;master.tryserver.chromium.linux:linux_chromium_dbg_32_ng ========== to ========== Switch Chrome on Linux to libc++ BUG=593874 R=thakis@chromium.org,thestig@chromium.org,brettw@chromium.org NOPRESUBMIT=true CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_dbg_ng;master.tryserver.chromium.linux:linux_arm;master.tryserver.chromium.linux:linux_chromium_archive_rel_ng;master.tryserver.chromium.linux:linux_chromium_dbg_32_ng ==========
The CQ bit was checked by thomasanderson@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #4 (id:80001) has been deleted
The CQ bit was checked by thomasanderson@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
The CQ bit was checked by thomasanderson@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
Patchset #4 (id:100001) has been deleted
Patchset #4 (id:120001) has been deleted
The CQ bit was checked by thomasanderson@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
The CQ bit was checked by thomasanderson@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_arm on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_arm/build...)
The CQ bit was checked by thomasanderson@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Switch Chrome on Linux to libc++ BUG=593874 R=thakis@chromium.org,thestig@chromium.org,brettw@chromium.org NOPRESUBMIT=true CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_dbg_ng;master.tryserver.chromium.linux:linux_arm;master.tryserver.chromium.linux:linux_chromium_archive_rel_ng;master.tryserver.chromium.linux:linux_chromium_dbg_32_ng ========== to ========== Switch Chrome on Linux to libc++ BUG=593874 R=thakis@chromium.org,jochen@chromium.org CC=thestig@chromium.org NOPRESUBMIT=true CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_dbg_ng;master.tryserver.chromium.linux:linux_arm;master.tryserver.chromium.linux:linux_chromium_archive_rel_ng;master.tryserver.chromium.linux:linux_chromium_dbg_32_ng ==========
thomasanderson@chromium.org changed reviewers: + jochen@chromium.org - brettw@chromium.org, thestig@chromium.org, thomasanderson@google.com
Patchset #5 (id:160001) has been deleted
Description was changed from ========== Switch Chrome on Linux to libc++ BUG=593874 R=thakis@chromium.org,jochen@chromium.org CC=thestig@chromium.org NOPRESUBMIT=true CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_dbg_ng;master.tryserver.chromium.linux:linux_arm;master.tryserver.chromium.linux:linux_chromium_archive_rel_ng;master.tryserver.chromium.linux:linux_chromium_dbg_32_ng ========== to ========== Switch Chrome on Linux to libc++ BUG=593874 R=thakis@chromium.org TBR=jochen@chromium.org CC=thestig@chromium.org NOPRESUBMIT=true CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_dbg_ng;master.tryserver.chromium.linux:linux_arm;master.tryserver.chromium.linux:linux_chromium_archive_rel_ng;master.tryserver.chromium.linux:linux_chromium_dbg_32_ng ==========
Dry run: Try jobs failed on following builders: linux_chromium_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_arm on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_arm/build...)
The CQ bit was checked by thomasanderson@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_arm on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_arm/build...)
The CQ bit was checked by thomasanderson@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
thakis ptal
what am I being TBR'ed for?
On 2017/06/28 18:15:49, jochen wrote: > what am I being TBR'ed for? extensions/shell/installer/linux/debian/expected_deps_x64_jessie
As said yesterday, let's finish our doc (on it, on it) and send an intent to implement first.
lgtm! https://codereview.chromium.org/2933573002/diff/140001/build/config/c++/c++.gni File build/config/c++/c++.gni (right): https://codereview.chromium.org/2933573002/diff/140001/build/config/c++/c++.g... build/config/c++/c++.gni:16: (is_linux && !is_chromeos) || The next line suggests that this wants to be is_linux && !is_chromeos && (!is_chromecast || is_cast_desktop_build). You can also remove the next line then. I think you can almost remove the whole rest of the line – we only use tsan and msan on linux. This is probably also true for ubsan, and we don't want to use custom_libcxx on mac (at least atm) so we don't need the libfuzzer bit. But maybe safer to not do that in this change. https://codereview.chromium.org/2933573002/diff/140001/chrome/installer/linux... File chrome/installer/linux/debian/expected_deps_ia32_jessie (left): https://codereview.chromium.org/2933573002/diff/140001/chrome/installer/linux... chrome/installer/linux/debian/expected_deps_ia32_jessie:20: libstdc++6 (>= 4.8.1) :-) https://codereview.chromium.org/2933573002/diff/140001/tools/perf_expectation... File tools/perf_expectations/perf_expectations.json (right): https://codereview.chromium.org/2933573002/diff/140001/tools/perf_expectation... tools/perf_expectations/perf_expectations.json:1: {"linux-release-64/sizes/chrome-si/initializers": {"reva": 480969, "revb": 480969, "type": "absolute", "better": "lower", "improve": 8, "regress": 8, "tolerance": 0, "sha1": "de779422"}, Please say in the CL description that the c++ standard library contains a static initializer and that this CL moves it from a .so into chrome. (In general, we never accept CLs that increase this number, but in this case it's net-neutral so it's ok.)
Please ask rdevlin.cronin@ to review that file (or TBR it to them). I don't know what this file is
thomasanderson@chromium.org changed reviewers: + rdevlin.cronin@chromium.org - jochen@chromium.org, thomasanderson@google.com
Description was changed from ========== Switch Chrome on Linux to libc++ BUG=593874 R=thakis@chromium.org TBR=jochen@chromium.org CC=thestig@chromium.org NOPRESUBMIT=true CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_dbg_ng;master.tryserver.chromium.linux:linux_arm;master.tryserver.chromium.linux:linux_chromium_archive_rel_ng;master.tryserver.chromium.linux:linux_chromium_dbg_32_ng ========== to ========== Switch Chrome on Linux to libc++ Design doc detailing the changes at [1], and announcement to chromium-dev at [2]. In addition, this CL updates perf_expectations to increase the number of allowed static initializers. libc++ adds some initializers from iostream.cpp. However, this case is net-neutral since the initializer used to live in libstdc++, but has been pushed into the chrome and nacl executables. The perf_expectations change also required adding NOPRESUBMIT=true to silence a bogus warning. [1] https://docs.google.com/document/d/1zmHUXlpGNXB433wHnr40dLwj7c-USsIVlyAeH7vOI... [2] https://groups.google.com/a/chromium.org/forum/?fromgroups#!topic/chromium-de... BUG=593874 R=thakis@chromium.org TBR=rdevlin.cronin@chromium.org CC=thestig@chromium.org NOPRESUBMIT=true CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_dbg_ng;master.tryserver.chromium.linux:linux_arm;master.tryserver.chromium.linux:linux_chromium_archive_rel_ng;master.tryserver.chromium.linux:linux_chromium_dbg_32_ng ==========
On 2017/06/29 18:51:44, jochen wrote: > Please ask rdevlin.cronin@ to review that file (or TBR it to them). I don't know > what this file is +rdevlin.cronin https://codereview.chromium.org/2933573002/diff/140001/build/config/c++/c++.gni File build/config/c++/c++.gni (right): https://codereview.chromium.org/2933573002/diff/140001/build/config/c++/c++.g... build/config/c++/c++.gni:16: (is_linux && !is_chromeos) || On 2017/06/29 18:30:54, Nico (vacation Jun 30-Jul 11) wrote: > The next line suggests that this wants to be is_linux && !is_chromeos && > (!is_chromecast || is_cast_desktop_build). You can also remove the next line > then. > > I think you can almost remove the whole rest of the line – we only use tsan and > msan on linux. This is probably also true for ubsan, and we don't want to use > custom_libcxx on mac (at least atm) so we don't need the libfuzzer bit. But > maybe safer to not do that in this change. Done. I'll hold off on changing the second half so I added a TODO in the meantime explaining what still needs to be cleaned up. https://codereview.chromium.org/2933573002/diff/140001/tools/perf_expectation... File tools/perf_expectations/perf_expectations.json (right): https://codereview.chromium.org/2933573002/diff/140001/tools/perf_expectation... tools/perf_expectations/perf_expectations.json:1: {"linux-release-64/sizes/chrome-si/initializers": {"reva": 480969, "revb": 480969, "type": "absolute", "better": "lower", "improve": 8, "regress": 8, "tolerance": 0, "sha1": "de779422"}, On 2017/06/29 18:30:54, Nico (vacation Jun 30-Jul 11) wrote: > Please say in the CL description that the c++ standard library contains a static > initializer and that this CL moves it from a .so into chrome. (In general, we > never accept CLs that increase this number, but in this case it's net-neutral so > it's ok.) Done.
The CQ bit was checked by thomasanderson@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
extensions/shell/installer lgtm (looks fine, but mostly trusting Nico). +Michael FYI.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by thomasanderson@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org Link to the patchset: https://codereview.chromium.org/2933573002/#ps180001 (title: "Simplify use_custom_libcxx conditions")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 180001, "attempt_start_ts": 1498772592631300, "parent_rev": "72014f6a8ab12897583185fc6c5b4752fcbaa278", "commit_rev": "39fcd6af4d8c9ddcf909b8d489c6ba684d046157"}
Message was sent while issue was closed.
Description was changed from ========== Switch Chrome on Linux to libc++ Design doc detailing the changes at [1], and announcement to chromium-dev at [2]. In addition, this CL updates perf_expectations to increase the number of allowed static initializers. libc++ adds some initializers from iostream.cpp. However, this case is net-neutral since the initializer used to live in libstdc++, but has been pushed into the chrome and nacl executables. The perf_expectations change also required adding NOPRESUBMIT=true to silence a bogus warning. [1] https://docs.google.com/document/d/1zmHUXlpGNXB433wHnr40dLwj7c-USsIVlyAeH7vOI... [2] https://groups.google.com/a/chromium.org/forum/?fromgroups#!topic/chromium-de... BUG=593874 R=thakis@chromium.org TBR=rdevlin.cronin@chromium.org CC=thestig@chromium.org NOPRESUBMIT=true CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_dbg_ng;master.tryserver.chromium.linux:linux_arm;master.tryserver.chromium.linux:linux_chromium_archive_rel_ng;master.tryserver.chromium.linux:linux_chromium_dbg_32_ng ========== to ========== Switch Chrome on Linux to libc++ Design doc detailing the changes at [1], and announcement to chromium-dev at [2]. In addition, this CL updates perf_expectations to increase the number of allowed static initializers. libc++ adds some initializers from iostream.cpp. However, this case is net-neutral since the initializer used to live in libstdc++, but has been pushed into the chrome and nacl executables. The perf_expectations change also required adding NOPRESUBMIT=true to silence a bogus warning. [1] https://docs.google.com/document/d/1zmHUXlpGNXB433wHnr40dLwj7c-USsIVlyAeH7vOI... [2] https://groups.google.com/a/chromium.org/forum/?fromgroups#!topic/chromium-de... BUG=593874 R=thakis@chromium.org TBR=rdevlin.cronin@chromium.org CC=thestig@chromium.org NOPRESUBMIT=true CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_dbg_ng;master.tryserver.chromium.linux:linux_arm;master.tryserver.chromium.linux:linux_chromium_archive_rel_ng;master.tryserver.chromium.linux:linux_chromium_dbg_32_ng Review-Url: https://codereview.chromium.org/2933573002 Cr-Commit-Position: refs/heads/master@{#483498} Committed: https://chromium.googlesource.com/chromium/src/+/39fcd6af4d8c9ddcf909b8d489c6... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:180001) as https://chromium.googlesource.com/chromium/src/+/39fcd6af4d8c9ddcf909b8d489c6...
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:180001) has been created in https://codereview.chromium.org/2965693002/ by ynovikov@chromium.org. The reason for reverting is: Breaks https://build.chromium.org/p/chromium.gpu.fyi/builders/GPU%20Linux%20Builder/.... |