Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(38)

Issue 1086973003: Misc undefined symbol fixes for Android+LTO. (Closed)

Created:
5 years, 8 months ago by pcc1
Modified:
5 years, 8 months ago
Reviewers:
Nico
CC:
chromium-reviews, skanuj+watch_chromium.org, melevin+watch_chromium.org, dhollowa+watch_chromium.org, dougw+watch_chromium.org, donnd+watch_chromium.org, jfweitz+watch_chromium.org, David Black, samarth+watch_chromium.org, kmadhusu+watch_chromium.org, Jered
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Misc undefined symbol fixes for Android+LTO. It appears that at least in some cases, the linker uses a stricter set of rules when linking LTO objects, which causes the link to fail in some cases where an unused function depends on an undefined symbol. This change fixes all such cases where the error caused the LTO build to fail, by either moving source files between lists, or by selectively disabling parts of source files depending on the configuration. There are more errors like this, which for some reason do not affect LTO. They can be inspected like this (with a regular Chromium/android build): $ ninja -C out/Release chrome_shell_apk $ nm out/Release/lib/libchromeshell.so | grep " U _Z" BUG=407544 R=thakis@chromium.org Committed: https://crrev.com/3a725956aff76414de8ecec2dcaa773b21fc8fe5 Cr-Commit-Position: refs/heads/master@{#325325}

Patch Set 1 #

Total comments: 2

Patch Set 2 : Move #include block down #

Unified diffs Side-by-side diffs Delta from patch set Stats (+40 lines, -39 lines) Patch
M chrome/browser/search/instant_service.h View 1 chunk +2 lines, -0 lines 0 comments Download
M chrome/browser/search/instant_service.cc View 1 5 chunks +26 lines, -18 lines 0 comments Download
M chrome/browser/ui/BUILD.gn View 1 chunk +0 lines, -5 lines 0 comments Download
M chrome/chrome_browser.gypi View 4 chunks +6 lines, -4 lines 0 comments Download
M chrome/chrome_browser_ui.gypi View 7 chunks +6 lines, -12 lines 0 comments Download

Messages

Total messages: 8 (2 generated)
pcc1
5 years, 8 months ago (2015-04-15 05:10:58 UTC) #1
Nico
lgtm https://codereview.chromium.org/1086973003/diff/1/chrome/browser/search/instant_service.cc File chrome/browser/search/instant_service.cc (right): https://codereview.chromium.org/1086973003/diff/1/chrome/browser/search/instant_service.cc#newcode23 chrome/browser/search/instant_service.cc:23: #endif // defined(ENABLE_THEMES) style nit: Move this down ...
5 years, 8 months ago (2015-04-15 16:29:49 UTC) #2
pcc1
https://codereview.chromium.org/1086973003/diff/1/chrome/browser/search/instant_service.cc File chrome/browser/search/instant_service.cc (right): https://codereview.chromium.org/1086973003/diff/1/chrome/browser/search/instant_service.cc#newcode23 chrome/browser/search/instant_service.cc:23: #endif // defined(ENABLE_THEMES) On 2015/04/15 16:29:49, Nico (away until ...
5 years, 8 months ago (2015-04-15 21:05:23 UTC) #3
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1086973003/20001
5 years, 8 months ago (2015-04-15 21:06:13 UTC) #6
commit-bot: I haz the power
Committed patchset #2 (id:20001)
5 years, 8 months ago (2015-04-15 22:21:17 UTC) #7
commit-bot: I haz the power
5 years, 8 months ago (2015-04-15 22:22:32 UTC) #8
Message was sent while issue was closed.
Patchset 2 (id:??) landed as
https://crrev.com/3a725956aff76414de8ecec2dcaa773b21fc8fe5
Cr-Commit-Position: refs/heads/master@{#325325}

Powered by Google App Engine
This is Rietveld 408576698