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

Issue 2525583003: Revert of Change some source_sets to static_library to shrink binary (Closed)

Created:
4 years, 1 month ago by brucedawson
Modified:
4 years ago
CC:
native-client-reviews_googlegroups.com
Target Ref:
refs/heads/master
Project:
nacl
Visibility:
Public.

Description

Revert of Change some source_sets to static_library to shrink binary (patchset #1 id:1 of https://codereview.chromium.org/2519103003/ ) Reason for revert: It looks like this fails when rolled into Chromium, with this error: Undefined symbols for architecture x86_64: "_NaClSwitch", referenced from: _NaClSyscallSegRegsSaved in nacl_syscall_64.o (maybe you meant: _NaClSwitchAVX, _NaClSwitchSSE ) "_nacl_current_thread_tls_offset", referenced from: _NaClSyscallSeg in nacl_syscall_64.o _NaClGetTlsFastPath1 in nacl_syscall_64.o _NaClGetTlsFastPath1End in nacl_syscall_64.o ld: symbol(s) not found for architecture x86_64 from: https://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_com... https://codereview.chromium.org/2523593005/ with these gn settings: goma_dir = "/b/c/cipd/goma" is_component_build = true is_debug = true symbol_level = 1 use_goma = true Original issue's description: > Change some source_sets to static_library to shrink binary > > The gn generated binaries are bigger than the gyp generated binaries. > One of the known differences was due to the nacl_global_rng and > g_NaCl_log_gio global variables which are in gn's chrome.dll but not in > gyp's. In some cases these global variables can serve as effective > canaries - removing them can lead to much code going away as well. > > Changing from source_set to static_library means that the linker is not > required to link in the associated .obj files, which can save time and > space. > > In this case no code was removed so the space savings was small - just > the 1.5 KB of space consumed by the globals. > > R=phosek@chromium.org > BUG=630755 > > Committed: https://chromium.googlesource.com/native_client/src/native_client/+/508aaa7b011729ee6eaab3384b89a7bfd0ecba73 TBR=phosek@chromium.org # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG=630755 Committed: https://chromium.googlesource.com/native_client/src/native_client/+/e4a9254d3505ec07c15bb40597136e0062bec9dc

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+3 lines, -3 lines) Patch
M src/trusted/service_runtime/BUILD.gn View 2 chunks +2 lines, -2 lines 0 comments Download
M src/trusted/validator_x86/BUILD.gn View 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 11 (5 generated)
brucedawson
Created Revert of Change some source_sets to static_library to shrink binary
4 years, 1 month ago (2016-11-22 19:33:48 UTC) #2
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2525583003/1
4 years, 1 month ago (2016-11-22 19:33:56 UTC) #3
commit-bot: I haz the power
No L-G-T-M from a valid reviewer yet. CQ run can only be started by full ...
4 years, 1 month ago (2016-11-22 19:33:57 UTC) #5
Mark Seaborn
LGTM
4 years, 1 month ago (2016-11-22 19:35:00 UTC) #6
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2525583003/1
4 years ago (2016-11-22 21:32:26 UTC) #8
commit-bot: I haz the power
4 years ago (2016-11-22 21:32:33 UTC) #11
Message was sent while issue was closed.
Committed patchset #1 (id:1) as
https://chromium.googlesource.com/native_client/src/native_client/+/e4a9254d3...

Powered by Google App Engine
This is Rietveld 408576698