Simple chrome script is copying unnecessary files |
||||||||||||||||
Issue descriptionWhat steps will reproduce the problem? 1. Follow the simplechrome workflow [1] and build chrome 2. Deploy the binary to the device with deploy_chrome [1] http://www.chromium.org/chromium-os/how-tos-and-troubleshooting/building-chromium-browser What is the expected result? deploy_chrome just works What happens instead of that? deploy_chrome failed with "Not enough free space on the device" error. Here's some findings: On the device side (squawks): - I have a test image versioned R54-8662.0.0 (Chrome 54.0.2813.0) installed. - /opt/google/chrome/chrome is 115MB On the workstation side: - chrome binary is 161MB (synced at #409960 and stripped in the staging directory [1]) - There are many lib*_library.so such as libui_library.so in the Release directory but these weren't included in the output of 'ldd chrome' [1] You can check the contents in the staging directory created by deploy_chrome by by passing --staging-only --staging-dir=... --build-dir=... options. Two questions: 1) Why is chrome binary built locally considerably bigger? 2) Why are lib*_library.so built if these are not needed by the chrome binary? Another finding: The chrome binary contained a section named '.GCC.command.line' which was about 9MB. Stripping it reduced the binary size from 161MB to 152MB. 152MB is still big though.
,
Aug 8 2016
,
Aug 9 2016
Checked the contents in ".GCC.command.line" section. Found that the following flags were only present in the device binary:
-fno-asynchronous-unwind-tables
-fno-unwind-tables
-fpreprocessed
-fauto-profile
I added the first two flags into cros_target_extra_cflags, cros_v8_snapshot_extra_cflags, cros_target_extra_cppflags, cros_v8_snapshot_extra_cppflags by "gn args out_$SDK_BOARD/Release" and confirmed that the chrome binary built with these extra flags was shrunk to 146MB from 161MB in the stating directory largely thanks to the reduction in the .eh_frame section.
RAW OUTPUT:
% readelf --string-dump=.GCC.command.line R54-8665/google/chrome/chrome |egrep -o -- ' -[f][^ ]+' |sort
-fPIC
-fauto-profile=/build/squawks/tmp/portage/chromeos-base/chromeos-chrome-54.0.2813.0_rc-r1/work/afdo/chromeos-chrome-amd64-54.0.2808.0_rc-r1.afdo -fdata-sections
-ffunction-sections
-fno-asynchronous-unwind-tables
-fno-exceptions
-fno-ident
-fno-math-errno
-fno-omit-frame-pointer
-fno-reorder-blocks-and-partition
-fno-rtti
-fno-signed-zeros
-fno-strict-aliasing
-fno-threadsafe-statics
-fno-tree-vectorize
-fno-unwind-tables
-fomit-frame-pointer
-fpreprocessed
-frecord-gcc-switches
-frtti
-fstack-protector-strong
-fvisibility-inlines-hidden
-fvisibility=hidden
% readelf --string-dump=.GCC.command.line ~/squawks/local-build/chrome |egrep -o -- ' -[f][^ ]+' |sort
-fPIC
-fdata-sections
-ffunction-sections
-fno-exceptions
-fno-ident
-fno-math-errno
-fno-omit-frame-pointer
-fno-reorder-blocks-and-partition
-fno-rtti
-fno-signed-zeros
-fno-strict-aliasing
-fno-threadsafe-statics
-fno-tree-vectorize
-fomit-frame-pointer
-frecord-gcc-switches
-frtti
-fstack-protector-strong
-funwind-tables
-fvisibility-inlines-hidden
-fvisibility=hidden
% diff -u flags.device flags.local
--- flags.device 2016-08-09 14:31:40.503675805 +0900
+++ flags.local 2016-08-09 14:32:03.319625962 +0900
@@ -1,8 +1,6 @@
-fPIC
- -fauto-profile=/build/squawks/tmp/portage/chromeos-base/chromeos-chrome-54.0.2813.0_rc-r1/work/afdo/chromeos-chrome-amd64-54.0.2808.0_rc-r1.afdo
-fdata-sections
-ffunction-sections
- -fno-asynchronous-unwind-tables
-fno-exceptions
-fno-ident
-fno-math-errno
@@ -13,11 +11,10 @@
-fno-strict-aliasing
-fno-threadsafe-statics
-fno-tree-vectorize
- -fno-unwind-tables
-fomit-frame-pointer
- -fpreprocessed
-frecord-gcc-switches
-frtti
-fstack-protector-strong
+ -funwind-tables
-fvisibility-inlines-hidden
-fvisibility=hidden
THE NEW LOCAL-BUILT BINARY:
% python readable-readelf.py local-build5/chrome
105.76 MiB .text
12.65 MiB .rela.dyn
11.50 MiB .rodata
9.03 MiB .GCC.command.line
3.47 MiB .data.rel.ro
2.41 MiB .data.rel.ro.loca
1.10 MiB .bss
205.97 KiB .data
133.05 KiB .got
35.86 KiB google_malloc
32.98 KiB .dynsym
28.88 KiB .rela.plt
27.45 KiB .dynstr
19.27 KiB .plt
9.65 KiB .got.plt
2.75 KiB .gnu.version
1.60 KiB malloc_hook
1.12 KiB .gnu.version_r
880 B .dynamic
544 B .init_array
378 B .shstrtab
368 B .gnu.hash
260 B .eh_frame
111 B .comment
68 B .eh_frame_hdr
36 B .note.gnu.build-i
36 B .init
32 B .note.ABI-tag
28 B .note.gnu.gold-ve
28 B .interp
16 B .tdata
16 B .dtors
16 B .ctors
14 B .fini
8 B .tbss
8 B .jcr
0 B .tm_clone_table
0 B (null)
,
Aug 9 2016
Thanks Satoru for investigating this! The chroot envoironment -> Simple Chrome $GN_ARGS conversion is done in cros_chrome_sdk.py: https://cs.chromium.org/chromium/src/third_party/chromite/cli/cros/cros_chrome_sdk.py?q=chrome_sdk.py&sq=package:chromium&l=749 +hashimoto@ who did a lot of that work. I can take a look at making the change later today or (more likely) tomorrow.
,
Aug 9 2016
thanks for the finding Satoru. I am just a little bit surprised why this is happening for simple chrome. It should already be in the CFLAGS coming from the ebuild.
,
Aug 9 2016
llozano@ - Did you follow the link in comment #4? We modify the environment pretty heavily in the Simple Chrome script, I am not terribly surprised that some of it is not entirely correct. That said I haven't had a chance to examine what exactly is going on yet.
,
Aug 10 2016
Looks like that a nontrivial portion of the difference came from AFDO (-fauto-profile). I've done a size comparison recently on images built with and without AFDO. The comparison shows a ~21.33% (124061384 v.s. 149558904) difference on the binary size. WITHAFDO: /build/falco/opt/google/chrome/chrome : section size addr .interp 28 624 .note.ABI-tag 32 652 .note.gnu.build-id 36 684 .dynsym 34464 720 .dynstr 29079 35184 .gnu.hash 368 64264 .gnu.version 2872 64632 .gnu.version_r 1152 67504 .rela.dyn 13310592 68656 .rela.plt 30504 13379248 .init 36 13409752 .plt 20352 13409792 .text 85667318 13430144 malloc_hook 1393 99097472 google_malloc 3493 99098880 .fini 14 99102376 .rodata 11590608 99102464 .eh_frame 260 110693072 .eh_frame_hdr 68 110693332 .tdata 16 110698944 .tbss 8 110698960 .data.rel.ro.local 2535688 110699008 .ctors 16 113234696 .dtors 16 113234712 .jcr 8 113234728 .data.rel.ro 3650944 113234752 .init_array 464 116885696 .dynamic 880 116886160 .got 141816 116887040 .got.plt 10192 117028856 .tm_clone_table 0 117039104 .data 227168 117039104 .bss 2289760 117266304 .comment 178 0 .GCC.command.line 6796088 0 .note.gnu.gold-version 28 0 .gnu_debuglink 20 0 Total 126345959 WITHOUT AFDO: /build/falco/opt/google/chrome/chrome : section size addr .interp 28 624 .note.ABI-tag 32 652 .note.gnu.build-id 36 684 .dynsym 33864 720 .dynstr 28211 34584 .gnu.hash 368 62800 .gnu.version 2822 63168 .gnu.version_r 1168 65992 .rela.dyn 13285104 67160 .rela.plt 29760 13352264 .init 36 13382024 .plt 19856 13382064 .text 111142166 13401920 malloc_hook 1640 124544096 google_malloc 36723 124545760 .fini 14 124582484 .rodata 11621840 124582656 .eh_frame 260 136204496 .eh_frame_hdr 68 136204756 .tdata 16 136209664 .tbss 8 136209680 .data.rel.ro.local 2532936 136209728 .ctors 16 138742664 .dtors 16 138742680 .jcr 8 138742696 .data.rel.ro 3641088 138742720 .init_array 464 142383808 .dynamic 880 142384272 .got 141568 142385152 .got.plt 9944 142526720 .tm_clone_table 0 142536704 .data 227232 142536704 .bss 2289760 142763968 .comment 178 0 .GCC.command.line 6795941 0 .note.gnu.gold-version 28 0 .gnu_debuglink 20 0 Total 151844099
,
Aug 10 2016
- the .text section size difference is expected. The images built by the release builders (official workflow) are build with AFDO optimization which is smarter about inlining and reduces code size. We have been using AFDO for a while on the release images. - we should strip the GCC.command.line sections. - @satoru, does cros_deploy succeed after you workaround the issue with -fno-asynchronous-unwind-tables ?
,
Aug 10 2016
It looks like these are set intentionally on 'is_chrome_branded && is_official_build' only: -fno-asynchronous-unwind-tables -fno-unwind-tables I'm not sure where/how -fpreprocessed gets set and if that matters.
,
Aug 10 2016
laszio: Thanks. That explains the difference with the .text section in the two binaries. Your numbers (85667318 => 81.70 MiB with AFDO, 111142166 => 105.99 MiB) matches my numbers above pretty well. That said, can we use the AFDO data in the simplechrome workflow?
,
Aug 10 2016
llozano@, laszio@ - Is disabling AFDO in Simple Chrome necessary / expected?
,
Aug 10 2016
Just in case more people encountered the same problem and want a quick workaround, the following arguments can be added to deploy_chrome. You may need to 'mkdir -p /usr/local/chrome' on the dut first. --target-dir=/usr/local/chrome --mount-dir=/opt/google/chrome
,
Aug 10 2016
If using AFDO is impractical in simple chrome, we could either make those deploy_chrome arguments the default from simple chrome (and update it to mkdir -p /usr/local/chrome), or increase the partition size by ~30% of the chrome binary size for dev/test builds. WDYT?
,
Aug 10 2016
+ihf@
,
Aug 10 2016
Re #8, llozano: deploy_chrome still failed:
Not enough free space on the device. Required: 355 MiB, actual: 322 MiB.
This was because the Release directory contained lib*_library.so, that were not needed by the chrome binary, and these were copied to the staging directory. We need to figure out why these were built.
Anyway, with lib*_library.so from the Release directory, deploy_chrome succeeded (".GCC.command.line" section still remains).
,
Aug 10 2016
about #11, disabling AFDO in simple chrome is EXPECTED. It is a long story but when we use AFDO we disable -Werror and we dont want to do that for developer builds (like simple chrome). Now, not using AFDO in simple chrome is not something new. So, that is not the cause of why the image does not fit. The cause is something related to the GN migration (like the missing -fno-asynchronous-unwind-tables flag). Increasing the partition size should be fine but I assume the problem goes away if you fix the GN differences. @satorux, does the image fit when you add the flags like -fno-asynchronous-unwind-tables?
,
Aug 10 2016
sorry I missed #15. Yes, we need to figure out the differences in the libraries built...
,
Aug 10 2016
Re ".GCC.command.line", what's the best way to remove this section? If there is no need to record command line flags, dropping -frecord-gcc-switches from the build configs would be the way to go. Alternatively, we can strip the section as part of deploy_chrome's stating step for the simplechrome workflow. For the production binary, what'd be the right way?
,
Aug 10 2016
we cannot remove -frecord-gcc-switches. I believe some unit tests depend on it. to remove the section from the executable armv7a-cros-linux-gnueabi-objcopy -R .GCC.command.line but I dont think we should focus on this. Can you try to figure out the library issue which seems to be the main problem?
,
Aug 10 2016
Re #19, let me take a look at the library issue In summary, here's breakdown numbers of what we can shave from the simplechrome workflow: - .eh_frame* sections: ~15MB by adding -fno-*unwind-tables flags - .GCC.command.line section: ~9MB by stripping the section - lib*_library.so: 88MB [1] by not building these [1] built with the -fno-*unwind-tables flags
,
Aug 10 2016
Update: I've built chrome outside of the simplechrome chroot as follows: % gn gen out/Release --args='is_debug=false use_goma=true target_os="chromeos" is_component_build=false remove_webcore_debug_symbols=true' % ninja -j 500 -C out/Release chrome and confirmed that lib*_library.so were present in the Release directory. So the issue is not simplechrome specific.
,
Aug 10 2016
FWIW also reproduced with Linux chrome build: % gn gen out1/Release --args='is_debug=false use_goma=true target_os="linux" is_component_build=false remove_webcore_debug_symbols=true % ninja -j 500 -C out1/Release chrome % ls out1/Release/lib*_library.so out1/Release/libfont_service_library.so out1/Release/libtracing_library.so out1/Release/libui_library.so
,
Aug 10 2016
+sky@ - Looks like service binaries. Any idea why they're so huge? And do they need to be built by default on chromeos builds?
,
Aug 10 2016
I wouldn't expect the ui library to be huge. Will have to look into it. We want them built by default on chromeos so we can switch between chromeos on top of mus or classic chromeos.
,
Aug 10 2016
I'll try to verify this tomorrow, but my guess is we're linking in ash three times: normal ash, sysui and mash.
,
Aug 10 2016
sky: Good to know that these were built intentionally. In that case, I think deploy_chrome should exclude them when building a staging directory. Re binary sizes, here's what I see in the staging directory of the simplechrome workflow (binaries were stripped): % du -hc lib*_library.so 3.2M libapp_driver_library.so 15M libash_library.so 18M libash_sysui_library.so 4.3M libfont_service_library.so 12M libquick_launch_library.so 12M libtask_viewer_library.so 12M libtouch_hud_library.so 1.2M libtracing_library.so 12M libui_library.so 88M total
,
Aug 10 2016
Re excluding lib*_library.so when deploying, I took a look at the deploy_chrome code and found this change by stevenjb@: https://chromium-review.googlesource.com/#/c/344401/3/lib/chrome_util.py This change added the following: Path('*.so', exe=True, cond=[C.StagingFlagSet('gn'), C.GypSet('component', value='shared_library')]), This rule comes with some conditionals, but because --strict is not set by default, this ends up copying *.so files regardless of the value of 'component'.
,
Aug 10 2016
If I understand correctly, there are three modes for copying files (--strict and --sloppy are exclusive):
1) --strict - evaluate per-Path conditionals, and ensure that non-optional files exist
2) default - do not evaluate per-Path conditionals, and ensure that non-optional or non-conditional files exist
3) --sloppy - do not evaluate per-Path conditionals, and do not ensure that any files exist at all
Do we need the three modes? I found it difficult to understand the differences, and the logic to check existence of files looked subtle to me [1].
What if we just make the strict behaviors default and drop the --strict option?
[1] The logic is implemented as:
if ((strict and not path.optional) or
(not strict and not (path.optional or path.cond) and not sloppy)):
msg = ('%s does not exist and is required.\n'
'You can bypass this error with --sloppy.\n'
'Aborting copy...' % src)
,
Aug 10 2016
satorux, excluding from scripts sounds reasonable. I've updated the summary to reflect what is happening.
,
Aug 10 2016
Apologies, my change was an effort to get things working during the GN migration. Some (all?) of the .so files moved with GN. Clearly we need to go back and clean that up. I remember also being confused by the strict/sloppy logic, but I was also nervous about breaking the GYP logic and other uses of deploy_chrome.
,
Aug 10 2016
OK, so I took a closer look at how we are using 'strict' and 'sloppy' in chromite, and it's pretty confusing and, I think, much more complicated than it needs to be, particularly regarding conditions. I am sure there are historical reasons for this, but they no longer seem to apply to the existing conditions. I took a stab at a simplification here: https://chromium-review.googlesource.com/c/367921/
,
Aug 12 2016
Thank you for starting to work on the simplification! Added some comments to the code review.
,
Aug 15 2016
I put up two different CLs, one maintaining --strict (but changing what it means): https://chromium-review.googlesource.com/c/367921/ And one eliminating --strict entirely: https://chromium-review.googlesource.com/c/368821/ There are other possible solutions, but these seemed like the best options going forward. We still need to convert from using GYP flags to GN args in these scripts. Both changes will require removing --strict from the ebuild, but the meaning of --strict is extremely confusing currently. If you have an opinion about how to proceed, please comment here and/or in either or both CLs. Otherwise I will work with satorux@ to choose a solution.
,
Aug 23 2016
Hey Satoru, we need to wrap this up. Would you prefer to keep --strict, or eliminate it entirely? I think my preference is just to eliminate it, I don't think it serves any purpose and just adds confusion. Either way I will follow up on the corresponding CL (see previous comment). If anyone else has an opinion, speak now...
,
Aug 23 2016
Oops. Sorry for not being clear. I gave +1 to https://chromium-review.googlesource.com/#/c/368821/ (eliminating --strict) and I preferred this approach. I was hesitant to give +2 because I was not familiar enough with the code and usage.
,
Sep 2 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/chromite/+/46a84c3948d84322d95d79b7059bdc748f360b87 commit 46a84c3948d84322d95d79b7059bdc748f360b87 Author: Steven Bennetts <stevenjb@chromium.org> Date: Fri Aug 12 22:20:46 2016 deploy_chrome: Always test conditions and eliminate --strict Currently we include conditions in the strict/sloppy logic and vice versa, which is confusing and seems unnecessary. This also simplifies the logic for strict and sloppy. BUG= chromium:635374 TEST=./cbuildbot/run_tests scripts/deploy_chrome_unittest lib/chrome_util_unittest Change-Id: I39f0d9afcc67a5daaeff6880192d6d54eaa83ea6 Reviewed-on: https://chromium-review.googlesource.com/368821 Commit-Ready: Steven Bennetts <stevenjb@chromium.org> Tested-by: Steven Bennetts <stevenjb@chromium.org> Reviewed-by: Steven Bennetts <stevenjb@chromium.org> [modify] https://crrev.com/46a84c3948d84322d95d79b7059bdc748f360b87/scripts/deploy_chrome.py [modify] https://crrev.com/46a84c3948d84322d95d79b7059bdc748f360b87/lib/chrome_util_unittest.py [modify] https://crrev.com/46a84c3948d84322d95d79b7059bdc748f360b87/lib/chrome_util.py [modify] https://crrev.com/46a84c3948d84322d95d79b7059bdc748f360b87/scripts/deploy_chrome_unittest.py
,
Sep 26 2016
,
Oct 7 2016
,
Nov 19 2016
,
Jan 21 2017
,
Mar 4 2017
,
Apr 17 2017
,
May 30 2017
,
Aug 1 2017
,
Oct 14 2017
|
||||||||||||||||
►
Sign in to add a comment |
||||||||||||||||
Comment 1 by satorux@chromium.org
, Aug 8 2016I wrote a script to make it easier to read output of 'readelf -S' command (attached). Here's some findings: - The local's .text section is 24MiB bigger - The local's .GCC.command.line section is 6.4MB bigger (I guess this section can be removed) - The local's .eh_frame section is 12.6 MiB bigger (the device's is only 260 bytes) The raw output is as follows: BINARY ON THE DEVICE: % python readable-readelf.py R54-8665/google/chrome/chrome |head 81.99 MiB .text 12.69 MiB .rela.dyn 10.93 MiB .rodata 3.48 MiB .data.rel.ro 2.66 MiB .GCC.command.line 2.42 MiB .data.rel.ro.loca 2.18 MiB .bss 221.72 KiB .data 138.55 KiB .got 33.70 KiB .dynsym 29.81 KiB .rela.plt 28.44 KiB .dynstr 19.89 KiB .plt 9.96 KiB .got.plt 8.72 KiB google_malloc 2.81 KiB .gnu.version 1.39 KiB malloc_hook 1.12 KiB .gnu.version_r 880 B .dynamic 464 B .init_array 393 B .shstrtab 368 B .gnu.hash 260 B .eh_frame 111 B .comment 68 B .eh_frame_hdr 36 B .note.gnu.build-i 36 B .init 32 B .note.ABI-tag 28 B .note.gnu.gold-ve 28 B .interp 20 B .gnu_debuglink 16 B .tdata 16 B .dtors 16 B .ctors 14 B .fini 8 B .tbss 8 B .jcr 0 B .tm_clone_table 0 B (null) BINARY BUILT LOCALLY (and stripped): % python readable-readelf.py local-build/chrome |head 105.76 MiB .text 12.65 MiB .rela.dyn 12.63 MiB .eh_frame 11.50 MiB .rodata 9.03 MiB .GCC.command.line 3.47 MiB .data.rel.ro 2.41 MiB .data.rel.ro.loca 2.35 MiB .eh_frame_hdr 1.10 MiB .bss 205.97 KiB .data 133.05 KiB .got 35.86 KiB google_malloc 32.98 KiB .dynsym 28.88 KiB .rela.plt 27.45 KiB .dynstr 19.27 KiB .plt 9.65 KiB .got.plt 2.75 KiB .gnu.version 1.60 KiB malloc_hook 1.12 KiB .gnu.version_r 880 B .dynamic 544 B .init_array 378 B .shstrtab 368 B .gnu.hash 111 B .comment 36 B .note.gnu.build-i 36 B .init 32 B .note.ABI-tag 28 B .note.gnu.gold-ve 28 B .interp 16 B .tdata 16 B .dtors 16 B .ctors 14 B .fini 8 B .tbss 8 B .jcr 0 B .tm_clone_table 0 B (null)774 bytes
774 bytes View Download