Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2288953002:
[Chromecast] Only include all libstdc++/libgcc symbols in executable as needed (Closed)
Description[Chromecast] Only include all libstdc++/libgcc symbols in executable as needed
All chromecast executable include all libstdc++/libgcc symbols by default
due to b/25566835. Added a config for --whole-archive for only ones that
depend on shared libs. ~700kb saving per executable.
BUG=internal b/30398176
Committed: https://crrev.com/92947f235fcf22912990029925b4154e7ea08f74
Cr-Commit-Position: refs/heads/master@{#416588}
Patch Set 1 #
Total comments: 4
Patch Set 2 : [Chromecast] Only include all libstdc++/libgcc symbols in executable as needed #Patch Set 3 : Moved more flags per comments by bcf #
Total comments: 4
Patch Set 4 : Fixed comments. #Patch Set 5 : Fixed comments #
Total comments: 3
Patch Set 6 : Updated conditional to exclude android build #
Total comments: 6
Patch Set 7 : Refactored conditional #
Messages
Total messages: 51 (22 generated)
Description was changed from ========== [Chromecast] Only include all libstdc++/libgcc symbols in executable as needed All chromecast executable include all libstdc++/libgcc symbols by default due to b/25566835. Added a config for --whole-archive for only ones that depend on shared libs. BUG=internal b/30398176 ========== to ========== [Chromecast] Only include all libstdc++/libgcc symbols in executable as needed All chromecast executable include all libstdc++/libgcc symbols by default due to b/25566835. Added a config for --whole-archive for only ones that depend on shared libs. BUG=internal b/30398176 ==========
antz@chromium.org changed reviewers: + bcf@chromium.org, halliwell@chromium.org, lcwu@chromium.org, slan@chromium.org, wzhong@chromium.org
The CQ bit was checked by antz@chromium.org to run a CQ dry run
Can you please list the saving in the commit message?
https://codereview.chromium.org/2288953002/diff/1/build/config/chromecast/BUI... File build/config/chromecast/BUILD.gn (left): https://codereview.chromium.org/2288953002/diff/1/build/config/chromecast/BUI... build/config/chromecast/BUILD.gn:40: if (current_cpu == "arm") { You should be able to move this whole block to executable_whole_archive_config, which will help save some more size. https://codereview.chromium.org/2288953002/diff/1/chromecast/crash/BUILD.gn File chromecast/crash/BUILD.gn (right): https://codereview.chromium.org/2288953002/diff/1/chromecast/crash/BUILD.gn#n... chromecast/crash/BUILD.gn:49: configs += [ "//build/config/chromecast:executable_whole_archive_config" ] How about put this under executable("crash_uploader") {
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
antz@chromium.org changed reviewers: + dpranke@chromium.org
https://codereview.chromium.org/2288953002/diff/1/build/config/chromecast/BUI... File build/config/chromecast/BUILD.gn (left): https://codereview.chromium.org/2288953002/diff/1/build/config/chromecast/BUI... build/config/chromecast/BUILD.gn:40: if (current_cpu == "arm") { On 2016/08/29 21:22:51, bcf wrote: > You should be able to move this whole block to executable_whole_archive_config, > which will help save some more size. Done. https://codereview.chromium.org/2288953002/diff/1/chromecast/crash/BUILD.gn File chromecast/crash/BUILD.gn (right): https://codereview.chromium.org/2288953002/diff/1/chromecast/crash/BUILD.gn#n... chromecast/crash/BUILD.gn:49: configs += [ "//build/config/chromecast:executable_whole_archive_config" ] On 2016/08/29 21:22:51, bcf wrote: > How about put this under > > executable("crash_uploader") { Done.
Description was changed from ========== [Chromecast] Only include all libstdc++/libgcc symbols in executable as needed All chromecast executable include all libstdc++/libgcc symbols by default due to b/25566835. Added a config for --whole-archive for only ones that depend on shared libs. BUG=internal b/30398176 ========== to ========== [Chromecast] Only include all libstdc++/libgcc symbols in executable as needed All chromecast executable include all libstdc++/libgcc symbols by default due to b/25566835. Added a config for --whole-archive for only ones that depend on shared libs. ~700kb saving per executable. BUG=internal b/30398176 ==========
The CQ bit was checked by antz@chromium.org to run a CQ dry run
lgtm. Are these the only executables which depend on shlibs? https://codereview.chromium.org/2288953002/diff/40001/build/config/chromecast... File build/config/chromecast/BUILD.gn (right): https://codereview.chromium.org/2288953002/diff/40001/build/config/chromecast... build/config/chromecast/BUILD.gn:37: # whole archiving libgcc.a and libstdc++ is needed if dynamic share libs are Super nit: Capitalize and use periods at end of sentences. https://codereview.chromium.org/2288953002/diff/40001/build/config/chromecast... build/config/chromecast/BUILD.gn:64: # static linking in order to prevent the executable from having a dynamic "Statically link libstdc++ and libgcc to avoid having a dynamic dependency on them."
On 2016/08/29 23:26:15, bcf wrote: > lgtm. Are these the only executables which depend on shlibs? > > https://codereview.chromium.org/2288953002/diff/40001/build/config/chromecast... > File build/config/chromecast/BUILD.gn (right): > > https://codereview.chromium.org/2288953002/diff/40001/build/config/chromecast... > build/config/chromecast/BUILD.gn:37: # whole archiving libgcc.a and libstdc++ is > needed if dynamic share libs are > Super nit: Capitalize and use periods at end of sentences. > > https://codereview.chromium.org/2288953002/diff/40001/build/config/chromecast... > build/config/chromecast/BUILD.gn:64: # static linking in order to prevent the > executable from having a dynamic > "Statically link libstdc++ and libgcc to avoid having a dynamic dependency on > them." Fixed. Those are the only ones in //chromecast. I will fix the internal ones once this change lands.
https://codereview.chromium.org/2288953002/diff/40001/build/config/chromecast... File build/config/chromecast/BUILD.gn (right): https://codereview.chromium.org/2288953002/diff/40001/build/config/chromecast... build/config/chromecast/BUILD.gn:64: # static linking in order to prevent the executable from having a dynamic On 2016/08/29 23:26:15, bcf wrote: > "Statically link libstdc++ and libgcc to avoid having a dynamic dependency on > them." Can you make this sentence more clear? By itself it doesn't make much sense right now. Something like I said in the above comment would work.
https://codereview.chromium.org/2288953002/diff/40001/build/config/chromecast... File build/config/chromecast/BUILD.gn (right): https://codereview.chromium.org/2288953002/diff/40001/build/config/chromecast... build/config/chromecast/BUILD.gn:64: # static linking in order to prevent the executable from having a dynamic On 2016/08/29 23:40:36, bcf wrote: > On 2016/08/29 23:26:15, bcf wrote: > > "Statically link libstdc++ and libgcc to avoid having a dynamic dependency on > > them." > > Can you make this sentence more clear? By itself it doesn't make much sense > right now. Something like I said in the above comment would work. Done.
The CQ bit was checked by antz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bcf@chromium.org Link to the patchset: https://codereview.chromium.org/2288953002/#ps80001 (title: "Fixed comments")
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
No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
https://codereview.chromium.org/2288953002/diff/80001/build/config/chromecast... File build/config/chromecast/BUILD.gn (right): https://codereview.chromium.org/2288953002/diff/80001/build/config/chromecast... build/config/chromecast/BUILD.gn:63: if (current_cpu == "arm") { Why just "arm" here? What is this conditional trying to say? For example, I think this would treat arm android builds differently than x86 android builds.
https://codereview.chromium.org/2288953002/diff/80001/build/config/chromecast... File build/config/chromecast/BUILD.gn (right): https://codereview.chromium.org/2288953002/diff/80001/build/config/chromecast... build/config/chromecast/BUILD.gn:63: if (current_cpu == "arm") { On 2016/08/30 22:22:53, slan wrote: > Why just "arm" here? What is this conditional trying to say? For example, I > think this would treat arm android builds differently than x86 android builds. This is only a problem on our Chromecast devices because we statically link libstdc++ . I believe android builds don't need this because cast_shell is built as a shared library rather than an executable.
https://codereview.chromium.org/2288953002/diff/80001/build/config/chromecast... File build/config/chromecast/BUILD.gn (right): https://codereview.chromium.org/2288953002/diff/80001/build/config/chromecast... build/config/chromecast/BUILD.gn:63: if (current_cpu == "arm") { On 2016/08/30 22:36:43, bcf wrote: > On 2016/08/30 22:22:53, slan wrote: > > Why just "arm" here? What is this conditional trying to say? For example, I > > think this would treat arm android builds differently than x86 android builds. > > This is only a problem on our Chromecast devices because we statically link > libstdc++ . > > I believe android builds don't need this because cast_shell is built as a shared > library rather than an executable. Thanks - that's a good point. However, this wouldn't apply to our MIPS linux builds, for instance, and I would really prefer to be consistent across architectures when possible. That will keep things clear in case we ever introduce an x86 Chromecast device. Are we trying to say !is_cast_desktop_build here? And if so, is there a disadvantage to using this configuration on desktop?
On 2016/08/30 22:46:51, slan wrote: > https://codereview.chromium.org/2288953002/diff/80001/build/config/chromecast... > File build/config/chromecast/BUILD.gn (right): > > https://codereview.chromium.org/2288953002/diff/80001/build/config/chromecast... > build/config/chromecast/BUILD.gn:63: if (current_cpu == "arm") { > On 2016/08/30 22:36:43, bcf wrote: > > On 2016/08/30 22:22:53, slan wrote: > > > Why just "arm" here? What is this conditional trying to say? For example, I > > > think this would treat arm android builds differently than x86 android > builds. > > > > This is only a problem on our Chromecast devices because we statically link > > libstdc++ . > > > > I believe android builds don't need this because cast_shell is built as a > shared > > library rather than an executable. > > Thanks - that's a good point. However, this wouldn't apply to our MIPS linux > builds, for instance, and I would really prefer to be consistent across > architectures when possible. That will keep things clear in case we ever > introduce an x86 Chromecast device. > > Are we trying to say !is_cast_desktop_build here? And if so, is there a > disadvantage to using this configuration on desktop? I'm not sure if our MIPS devices have the same issue (Although I imagine they would). If they don't need this fix, then this will unnecessarily increase the size of the cast receiver package. As I recall, this caused linker errors on desktop builds that we didn't think were worth fixing at the time. I would be fine with using !is_cast_desktop_build or removing the conditional if it works.
On 2016/08/30 23:02:52, bcf wrote: > On 2016/08/30 22:46:51, slan wrote: > > > https://codereview.chromium.org/2288953002/diff/80001/build/config/chromecast... > > File build/config/chromecast/BUILD.gn (right): > > > > > https://codereview.chromium.org/2288953002/diff/80001/build/config/chromecast... > > build/config/chromecast/BUILD.gn:63: if (current_cpu == "arm") { > > On 2016/08/30 22:36:43, bcf wrote: > > > On 2016/08/30 22:22:53, slan wrote: > > > > Why just "arm" here? What is this conditional trying to say? For example, > I > > > > think this would treat arm android builds differently than x86 android > > builds. > > > > > > This is only a problem on our Chromecast devices because we statically link > > > libstdc++ . > > > > > > I believe android builds don't need this because cast_shell is built as a > > shared > > > library rather than an executable. > > > > Thanks - that's a good point. However, this wouldn't apply to our MIPS linux > > builds, for instance, and I would really prefer to be consistent across > > architectures when possible. That will keep things clear in case we ever > > introduce an x86 Chromecast device. > > > > Are we trying to say !is_cast_desktop_build here? And if so, is there a > > disadvantage to using this configuration on desktop? > > I'm not sure if our MIPS devices have the same issue (Although I imagine they > would). If they don't need this fix, then this will unnecessarily increase the > size of the cast receiver package. > > As I recall, this caused linker errors on desktop builds that we didn't think > were worth fixing at the time. > > I would be fine with using !is_cast_desktop_build or removing the conditional if > it works. On certain platforms, such as x86-64 or aarch64, linking a static lib against a shared lib will cause relocation issues due to static lib does not contain positional independent code, and is resolved with -fPIC compilation flag. Source: https://www.technovelty.org//c/position-independent-code-and-x86-64-libraries... https://gcc.gnu.org/onlinedocs/gcc/Code-Gen-Options.html#Code-Gen-Options !is_cast_desktop_build may not be a sufficient guard since this could affect android builds. Instead we might hv to guard against either arch or targets being compiled as shared lib (which I don't exactly know how to, not knowing all the combinations)
The CQ bit was checked by antz@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.
On 2016/08/31 22:16:48, antz1 wrote: > On 2016/08/30 23:02:52, bcf wrote: > > On 2016/08/30 22:46:51, slan wrote: > > > > > > https://codereview.chromium.org/2288953002/diff/80001/build/config/chromecast... > > > File build/config/chromecast/BUILD.gn (right): > > > > > > > > > https://codereview.chromium.org/2288953002/diff/80001/build/config/chromecast... > > > build/config/chromecast/BUILD.gn:63: if (current_cpu == "arm") { > > > On 2016/08/30 22:36:43, bcf wrote: > > > > On 2016/08/30 22:22:53, slan wrote: > > > > > Why just "arm" here? What is this conditional trying to say? For > example, > > I > > > > > think this would treat arm android builds differently than x86 android > > > builds. > > > > > > > > This is only a problem on our Chromecast devices because we statically > link > > > > libstdc++ . > > > > > > > > I believe android builds don't need this because cast_shell is built as a > > > shared > > > > library rather than an executable. > > > > > > Thanks - that's a good point. However, this wouldn't apply to our MIPS linux > > > builds, for instance, and I would really prefer to be consistent across > > > architectures when possible. That will keep things clear in case we ever > > > introduce an x86 Chromecast device. > > > > > > Are we trying to say !is_cast_desktop_build here? And if so, is there a > > > disadvantage to using this configuration on desktop? > > > > I'm not sure if our MIPS devices have the same issue (Although I imagine they > > would). If they don't need this fix, then this will unnecessarily increase the > > size of the cast receiver package. > > > > As I recall, this caused linker errors on desktop builds that we didn't think > > were worth fixing at the time. > > > > I would be fine with using !is_cast_desktop_build or removing the conditional > if > > it works. > > On certain platforms, such as x86-64 or aarch64, linking a static lib against a > shared lib will cause relocation issues due to static lib does not contain > positional independent code, and is resolved with -fPIC compilation flag. > Source: > https://www.technovelty.org//c/position-independent-code-and-x86-64-libraries... > https://gcc.gnu.org/onlinedocs/gcc/Code-Gen-Options.html#Code-Gen-Options > !is_cast_desktop_build may not be a sufficient guard since this could affect > android builds. Instead we might hv to guard against either arch or targets > being compiled as shared lib (which I don't exactly know how to, not knowing all > the combinations) Anthony - I was not aware of the PIC issue; thanks for the links. It sounds as if this *is* actually a choice specific to the arm build. Given the information you've presented, landing this as-is lgtm.
On 2016/09/01 02:33:38, slan wrote: > On 2016/08/31 22:16:48, antz1 wrote: > > On 2016/08/30 23:02:52, bcf wrote: > > > On 2016/08/30 22:46:51, slan wrote: > > > > > > > > > > https://codereview.chromium.org/2288953002/diff/80001/build/config/chromecast... > > > > File build/config/chromecast/BUILD.gn (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2288953002/diff/80001/build/config/chromecast... > > > > build/config/chromecast/BUILD.gn:63: if (current_cpu == "arm") { > > > > On 2016/08/30 22:36:43, bcf wrote: > > > > > On 2016/08/30 22:22:53, slan wrote: > > > > > > Why just "arm" here? What is this conditional trying to say? For > > example, > > > I > > > > > > think this would treat arm android builds differently than x86 android > > > > builds. > > > > > > > > > > This is only a problem on our Chromecast devices because we statically > > link > > > > > libstdc++ . > > > > > > > > > > I believe android builds don't need this because cast_shell is built as > a > > > > shared > > > > > library rather than an executable. > > > > > > > > Thanks - that's a good point. However, this wouldn't apply to our MIPS > linux > > > > builds, for instance, and I would really prefer to be consistent across > > > > architectures when possible. That will keep things clear in case we ever > > > > introduce an x86 Chromecast device. > > > > > > > > Are we trying to say !is_cast_desktop_build here? And if so, is there a > > > > disadvantage to using this configuration on desktop? > > > > > > I'm not sure if our MIPS devices have the same issue (Although I imagine > they > > > would). If they don't need this fix, then this will unnecessarily increase > the > > > size of the cast receiver package. > > > > > > As I recall, this caused linker errors on desktop builds that we didn't > think > > > were worth fixing at the time. > > > > > > I would be fine with using !is_cast_desktop_build or removing the > conditional > > if > > > it works. > > > > On certain platforms, such as x86-64 or aarch64, linking a static lib against > a > > shared lib will cause relocation issues due to static lib does not contain > > positional independent code, and is resolved with -fPIC compilation flag. > > Source: > > > https://www.technovelty.org//c/position-independent-code-and-x86-64-libraries... > > https://gcc.gnu.org/onlinedocs/gcc/Code-Gen-Options.html#Code-Gen-Options > > !is_cast_desktop_build may not be a sufficient guard since this could affect > > android builds. Instead we might hv to guard against either arch or targets > > being compiled as shared lib (which I don't exactly know how to, not knowing > all > > the combinations) > > Anthony - I was not aware of the PIC issue; thanks for the links. It sounds as > if this *is* actually a choice specific to the arm build. Given the information > you've presented, landing this as-is lgtm. I think this should be good for all chromecast device build, I was not aware we have MIPS builds as you mentioned earlier, so I was struggling to come up with a good conditional. I will sync up with you offline tomorrow on this.
After offline discussion: 1. we will need to statically link against libgcc and libstdc++ for all chromecast devices, but not android, hence the conditional is updated. 2. Reached out the MIPS team for possible licensing concerns. They are dynamically linking these libs in their code base, but they will evaluate the implication and get back to us if they want this changed.
The CQ bit was checked by antz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from slan@chromium.org, bcf@chromium.org Link to the patchset: https://codereview.chromium.org/2288953002/#ps100001 (title: "Updated conditional to exclude android build")
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
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...)
antz@chromium.org changed reviewers: + brettw@chromium.org, scottmg@chromium.org
Need owners to lgtm, please review. :)
lgtm
Nit https://codereview.chromium.org/2288953002/diff/100001/build/config/chromecas... File build/config/chromecast/BUILD.gn (right): https://codereview.chromium.org/2288953002/diff/100001/build/config/chromecas... build/config/chromecast/BUILD.gn:65: if (current_cpu == "arm" && !is_android) { Define variable for this at top.
->dpranke for OWNERS who I think reviewed most of the chromecast/BUILD.gn changes. https://codereview.chromium.org/2288953002/diff/100001/build/config/chromecas... File build/config/chromecast/BUILD.gn (right): https://codereview.chromium.org/2288953002/diff/100001/build/config/chromecas... build/config/chromecast/BUILD.gn:37: # Whole archiving libgcc.a and libstdc++ is needed if dynamic share libs are "share" -> "shared" https://codereview.chromium.org/2288953002/diff/100001/build/config/chromecas... build/config/chromecast/BUILD.gn:40: if (current_cpu == "arm" && !is_android) { Isn't there an is_chromecast, if that's what's meant here? https://codereview.chromium.org/2288953002/diff/100001/build/config/chromecas... build/config/chromecast/BUILD.gn:65: if (current_cpu == "arm" && !is_android) { And here?
https://codereview.chromium.org/2288953002/diff/100001/build/config/chromecas... File build/config/chromecast/BUILD.gn (right): https://codereview.chromium.org/2288953002/diff/100001/build/config/chromecas... build/config/chromecast/BUILD.gn:37: # Whole archiving libgcc.a and libstdc++ is needed if dynamic share libs are On 2016/09/02 20:40:55, scottmg wrote: > "share" -> "shared" Done. https://codereview.chromium.org/2288953002/diff/100001/build/config/chromecas... build/config/chromecast/BUILD.gn:40: if (current_cpu == "arm" && !is_android) { On 2016/09/02 20:40:55, scottmg wrote: > Isn't there an is_chromecast, if that's what's meant here? is_chromecast is set for our android builds, which we do not want to statically link these libs. We want to do them for arm device builds.
lgtm
The CQ bit was checked by antz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from slan@chromium.org, bcf@chromium.org, wzhong@chromium.org Link to the patchset: https://codereview.chromium.org/2288953002/#ps120001 (title: "Refactored conditional")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [Chromecast] Only include all libstdc++/libgcc symbols in executable as needed All chromecast executable include all libstdc++/libgcc symbols by default due to b/25566835. Added a config for --whole-archive for only ones that depend on shared libs. ~700kb saving per executable. BUG=internal b/30398176 ========== to ========== [Chromecast] Only include all libstdc++/libgcc symbols in executable as needed All chromecast executable include all libstdc++/libgcc symbols by default due to b/25566835. Added a config for --whole-archive for only ones that depend on shared libs. ~700kb saving per executable. BUG=internal b/30398176 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== [Chromecast] Only include all libstdc++/libgcc symbols in executable as needed All chromecast executable include all libstdc++/libgcc symbols by default due to b/25566835. Added a config for --whole-archive for only ones that depend on shared libs. ~700kb saving per executable. BUG=internal b/30398176 ========== to ========== [Chromecast] Only include all libstdc++/libgcc symbols in executable as needed All chromecast executable include all libstdc++/libgcc symbols by default due to b/25566835. Added a config for --whole-archive for only ones that depend on shared libs. ~700kb saving per executable. BUG=internal b/30398176 Committed: https://crrev.com/92947f235fcf22912990029925b4154e7ea08f74 Cr-Commit-Position: refs/heads/master@{#416588} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/92947f235fcf22912990029925b4154e7ea08f74 Cr-Commit-Position: refs/heads/master@{#416588} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
