New issue
Advanced search Search tips

Issue 648475 link

Starred by 3 users

Issue metadata

Status: Started
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 3
Type: Bug



Sign in to add a comment

Maybe there is a better way to find unused resources

Project Member Reported by michaelbai@chromium.org, Sep 20 2016

Issue description

Currently we rely on the compile warning to find out unused resources, search "enable_resource_whitelist_generation" to find out the detail, 

During the review of https://codereview.chromium.org/2343083003/diff/60001/android_webview/BUILD.gn , agrieve@ found out the resource defined in android_webview/ui/grit_components_whitelist.txt doesn't match with generated whilelist.

We should find out whether there is better way to find unused resources.
 
Status: Available (was: Untriaged)
Gradle has a "shrink_resources" compile step that looks through all integer constants that appear in the proguarded classes.dex, as well as in res/xml and AndroidManifest.xml, and uses these as its whitelist.

It's possible that we could do something similar on the native side.

Ideas:
#1 - Look for all constants within .data and .bss and interpret them as whitelist entries.
#2 - Have the #define's map to actual constants in a .cc file, and annotate the constants with __attribute__ ((section ("pak_constant_#####"))). Use --print-gc-sections to create a pak file blacklist.
#3 - Similar to #2, but have all constants in the same section. Use --relocatable to do a partial link and then read out the section values.
#4 - See if there's anything in the generated DWARF debug information that could be used to see which constants were removed.
Project Member

Comment 2 by sheriffbot@chromium.org, Sep 20 2017

Labels: Hotlist-Recharge-Cold
Status: Untriaged (was: Available)
This issue has been Available for over a year. If it's no longer important or seems unlikely to be fixed, please consider closing it out. If it is important, please re-triage the issue.

Sorry for the inconvenience if the bug really should have been left as Available. If you change it back, also remove the "Hotlist-Recharge-Cold" label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 3 by cmasso@google.com, Feb 14 2018

Components: Build

Comment 4 by cmasso@google.com, Feb 14 2018

Components: Tools>ClankTools
2 wouldn't work because some of the constants appear in integral-constant-expression contexts (e.g. cases in switch statements).

Here's another idea. It relies on this function template definition:

template <int X> __attribute__((used)) constexpr int magic_int() {
  return X;
}

Because of the used attribute, this function template causes debug info to be written to the output file whenever it is instantiated. (It also causes a function definition to be written, but they will be removed by --gc-sections -- debug info is immune to --gc-sections.) Because it is constexpr, it can be used in an integral-constant-expression. The definitions of resource ids look like this:

#define IDS_FOO magic_int<123>()

Then we can find all the valid resource ids by scanning the debug info in the unstripped .so file for functions named magic_int<X>.

I implemented the debug info embedding part of this here: https://chromium-review.googlesource.com/c/chromium/src/+/1162699

I used this command to create a whitelist:

$ strings -a o/a/lib.unstripped/libmonochrome.so o/a/android_clang_arm/lib.unstripped/libmonochrome.so | grep '^magic_int<' | grep -o '[0-9]\+' | sort -n | uniq > /tmp/whitelist

and verified that it was almost the same as the one created using the existing pipeline (except for IDS_VERSION_UI_64BIT, which the existing pipeline adds manually).
If we wanted this to work well with --gc-sections (i.e. so we could drop resources referred to by gc'd functions), I think we'd need some sort of compiler extension. The extension would allow us to write something conceptually like magic_int, except that as a side effect of instantiating the template the compiler would emit a global variable whose value is the resource id in a section which would be classified as debug info (so that it can be stripped) and associated [1] with the function or global variable that instantiated the template. (If the template instantiation appears in multiple functions or global variables, we would need an associated section for each one.) Then we can discover the whitelist by reading the section in the unstripped binary.

[1] https://llvm.org/docs/LangRef.html#associated-metadata
Status: Available (was: Untriaged)
That's awesome that you got a prototype to work!

We can expand to a clang plugin in the future if needed, but #5 being simple and achieving the same result as now seems like a clear win!

Owner: p...@chromium.org
Status: Started (was: Available)
https://chromium-review.googlesource.com/c/chromium/src/+/1174012
implements the rest of c#5.
Project Member

Comment 9 by bugdroid1@chromium.org, Aug 17

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f26ebd65019e400184adad499c73264140c8fa96

commit f26ebd65019e400184adad499c73264140c8fa96
Author: Peter Collingbourne <pcc@chromium.org>
Date: Fri Aug 17 18:24:22 2018

grit: Replace current resource whitelisting implementation with a better one.

The new implementation works by embedding debug info in object
files instead of parsing compiler warning output. As a result,
our official builds are much less noisy. See the new header file
ui/base/resource/whitelist.h for details on how it works.

It is also simpler because it does not rely on wrapper scripts for
collecting the set of whitelisted resource ids. As a result we no
longer need, and can remove, the wrapper scripts for the compiler
and ar.

It is also theoretically more precise because it only whitelists
resource ids mentioned in archive members that were chosen by the
linker, although I didn't see an improvement in monochrome. It may
be possible to gain even more precision by extending the compiler
(see comments on bug) but first we'd need to investigate whether that
would be worth it.

Bug: 648475, 684788
Cq-Include-Trybots: luci.chromium.try:ios-simulator-full-configs;master.tryserver.chromium.mac:ios-simulator-cronet
Change-Id: I12d83a8aaffa42c3afcdcc834f7dd7536b7a9248
Reviewed-on: https://chromium-review.googlesource.com/1174012
Reviewed-by: Eugene But <eugenebut@chromium.org>
Reviewed-by: Richard Coles <torne@chromium.org>
Reviewed-by: Nico Weber <thakis@chromium.org>
Reviewed-by: agrieve <agrieve@chromium.org>
Reviewed-by: Steven Holte <holte@chromium.org>
Commit-Queue: Peter Collingbourne <pcc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#584129}
[modify] https://crrev.com/f26ebd65019e400184adad499c73264140c8fa96/android_webview/BUILD.gn
[modify] https://crrev.com/f26ebd65019e400184adad499c73264140c8fa96/build/android/resource_sizes.py
[delete] https://crrev.com/a8ae8cd395ac425d4a521cbbec134c95c3e0e2f9/build/toolchain/gcc_ar_wrapper.py
[delete] https://crrev.com/a8ae8cd395ac425d4a521cbbec134c95c3e0e2f9/build/toolchain/gcc_compile_wrapper.py
[modify] https://crrev.com/f26ebd65019e400184adad499c73264140c8fa96/build/toolchain/gcc_solink_wrapper.py
[modify] https://crrev.com/f26ebd65019e400184adad499c73264140c8fa96/build/toolchain/gcc_toolchain.gni
[modify] https://crrev.com/f26ebd65019e400184adad499c73264140c8fa96/build/toolchain/nacl/BUILD.gn
[modify] https://crrev.com/f26ebd65019e400184adad499c73264140c8fa96/build/toolchain/wrapper_utils.py
[modify] https://crrev.com/f26ebd65019e400184adad499c73264140c8fa96/chrome/BUILD.gn
[modify] https://crrev.com/f26ebd65019e400184adad499c73264140c8fa96/chrome/android/BUILD.gn
[modify] https://crrev.com/f26ebd65019e400184adad499c73264140c8fa96/chrome/browser/ui/webui/settings/about_handler.cc
[modify] https://crrev.com/f26ebd65019e400184adad499c73264140c8fa96/chrome/browser/ui/webui/version_ui.cc
[modify] https://crrev.com/f26ebd65019e400184adad499c73264140c8fa96/components/variations/service/generate_ui_string_overrider.py
[modify] https://crrev.com/f26ebd65019e400184adad499c73264140c8fa96/ios/chrome/browser/ui/webui/version_ui.mm
[modify] https://crrev.com/f26ebd65019e400184adad499c73264140c8fa96/tools/grit/grit/format/gen_predetermined_ids.py
[modify] https://crrev.com/f26ebd65019e400184adad499c73264140c8fa96/tools/grit/grit/format/gen_predetermined_ids_unittest.py
[modify] https://crrev.com/f26ebd65019e400184adad499c73264140c8fa96/tools/grit/grit/format/rc_header.py
[modify] https://crrev.com/f26ebd65019e400184adad499c73264140c8fa96/tools/grit/grit/format/rc_header_unittest.py
[modify] https://crrev.com/f26ebd65019e400184adad499c73264140c8fa96/tools/grit/grit/node/misc.py
[modify] https://crrev.com/f26ebd65019e400184adad499c73264140c8fa96/tools/grit/grit/node/misc_unittest.py
[modify] https://crrev.com/f26ebd65019e400184adad499c73264140c8fa96/tools/grit/grit/tool/build.py
[modify] https://crrev.com/f26ebd65019e400184adad499c73264140c8fa96/tools/grit/grit_rule.gni
[modify] https://crrev.com/f26ebd65019e400184adad499c73264140c8fa96/tools/mb/mb_config.pyl
[modify] https://crrev.com/f26ebd65019e400184adad499c73264140c8fa96/tools/resources/generate_resource_whitelist.gni
[modify] https://crrev.com/f26ebd65019e400184adad499c73264140c8fa96/tools/resources/generate_resource_whitelist.py
[modify] https://crrev.com/f26ebd65019e400184adad499c73264140c8fa96/ui/base/BUILD.gn
[add] https://crrev.com/f26ebd65019e400184adad499c73264140c8fa96/ui/base/resource/whitelist.h

Project Member

Comment 10 by bugdroid1@chromium.org, Aug 24

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/069256a2001d1abc491d4a89e594069057351ca4

commit 069256a2001d1abc491d4a89e594069057351ca4
Author: agrieve <agrieve@chromium.org>
Date: Fri Aug 24 05:06:28 2018

diagnose_bloat.py: Remove symbol_level=0 now that it's required

Bug: 648475

Change-Id: Ibbafd96a020850881c0c515e0e4dd71ee530c099
Reviewed-on: https://chromium-review.googlesource.com/1187922
Reviewed-by: Peter Collingbourne <pcc@chromium.org>
Commit-Queue: agrieve <agrieve@chromium.org>
Cr-Commit-Position: refs/heads/master@{#585682}
[modify] https://crrev.com/069256a2001d1abc491d4a89e594069057351ca4/tools/binary_size/diagnose_bloat.py

Sign in to add a comment