Skip to content

Commit

Permalink
Reland "Android: Check R8 output for references to missing symbols"
Browse files Browse the repository at this point in the history 
This reverts commit 7f7b0e8.

Reason for reland: Updated allow-list

Original change's description:
> Revert "Android: Check R8 output for references to missing symbols"
>
> This reverts commit 67f02bb.
>
> Reason for revert: The new check breaks the android build on perf waterfall, e.g.: https://ci.chromium.org/p/chrome/builders/ci/android-builder-perf/240223
>
> Original change's description:
> > Android: Check R8 output for references to missing symbols
> >
> > javac prevents missing symbols via compiler errors, but they
> > can still occur by including prebuilts with incomplete deps,
> > or by using jar_excluded_patterns to remove a class that is
> > actually used.
> >
> > Bug: 1142530
> > Change-Id: Idebb28c9e1133d9021d899c570e35a5ff16d5f14
> > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2510236
> > Commit-Queue: Andrew Grieve <agrieve@chromium.org>
> > Reviewed-by: Peter Kotwicz <pkotwicz@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#824739}
>
> TBR=agrieve@chromium.org,pkotwicz@chromium.org
>
> Change-Id: I20033723611ea07fcae98b7824402da89001dd8c
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Bug: 1142530
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2523395
> Reviewed-by: Wenbin Zhang <wenbinzhang@google.com>
> Commit-Queue: Wenbin Zhang <wenbinzhang@google.com>
> Cr-Commit-Position: refs/heads/master@{#824954}

TBR=agrieve@chromium.org,pkotwicz@chromium.org,wenbinzhang@google.com

NOTRY=true  # Already went through CQ in different CL.

Bug: 1142530
Change-Id: I52ccfacc28c38db741d08ba10d00bb133ca89004
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2523532
Commit-Queue: Andrew Grieve <agrieve@chromium.org>
Reviewed-by: Peter Kotwicz <pkotwicz@chromium.org>
Reviewed-by: Andrew Grieve <agrieve@chromium.org>
Cr-Commit-Position: refs/heads/master@{#824959}
  • Loading branch information
@agrieve
agrieve authored and Commit Bot committed Nov 6, 2020
1 parent b29bb27 commit 1f56ae5
Show file tree
Hide file tree
Showing 5 changed files with 127 additions and 22 deletions.
122 changes: 112 additions & 10 deletions build/android/gyp/proguard.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -84,9 +84,9 @@ def _ParseOptions():
action='store_true',
help='Disable the outlining optimization provided by R8.')
parser.add_argument(
'--disable-checkdiscard',
action='store_true',
help='Disable -checkdiscard directives')
'--disable-checks',
action='store_true',
help='Disable -checkdiscard directives and missing symbols check')
parser.add_argument('--sourcefile', help='Value for source file attribute')
parser.add_argument(
'--force-enable-assertions',
Expand Down Expand Up @@ -233,14 +233,9 @@ def _OptimizeWithR8(options,
tmp_mapping_path,
]

if options.disable_checkdiscard:
if options.disable_checks:
# Info level priority logs are not printed by default.
cmd += [
'--map-diagnostics:'
'com.android.tools.r8.errors.CheckDiscardDiagnostic',
'error',
'info',
]
cmd += ['--map-diagnostics:CheckDiscardDiagnostic', 'error', 'info']

if options.desugar_jdk_libs_json:
cmd += [
Expand Down Expand Up @@ -329,6 +324,103 @@ def _OptimizeWithR8(options,
out_file.writelines(l for l in in_file if not l.startswith('#'))


def _CheckForMissingSymbols(r8_path, dex_files, classpath, warnings_as_errors):
cmd = build_utils.JavaCmd(warnings_as_errors) + [
'-cp', r8_path, 'com.android.tools.r8.tracereferences.TraceReferences',
'--map-diagnostics:MissingDefinitionsDiagnostic', 'error', 'warning'
]

for path in classpath:
cmd += ['--lib', path]
for path in dex_files:
cmd += ['--source', path]

def stderr_filter(stderr):
ignored_lines = [
# Summary contains warning count, which our filtering makes wrong.
'Warning: Tracereferences found',

# TODO(agrieve): Create interface jars for these missing classes rather
# than allowlisting here.
'dalvik/system',
'libcore/io',
'sun/misc/Unsafe',

# Found in: com/facebook/fbui/textlayoutbuilder/StaticLayoutHelper
('android/text/StaticLayout;<init>(Ljava/lang/CharSequence;IILandroid'
'/text/TextPaint;ILandroid/text/Layout$Alignment;Landroid/text/'
'TextDirectionHeuristic;FFZLandroid/text/TextUtils$TruncateAt;II)V'),

# Found in
# com/google/android/gms/cast/framework/media/internal/ResourceProvider
# Missing due to setting "strip_resources = true".
'com/google/android/gms/cast/framework/R',

# Found in com/google/android/gms/common/GoogleApiAvailability
# Missing due to setting "strip_drawables = true".
'com/google/android/gms/base/R$drawable',

# Explicictly guarded by try (NoClassDefFoundError) in Flogger's
# PlatformProvider.
'com/google/common/flogger/backend/google/GooglePlatform',
'com/google/common/flogger/backend/system/DefaultPlatform',

# trichrome_webview_google_bundle contains this missing reference.
# TODO(crbug.com/1142530): Fix this missing reference properly.
'org/chromium/base/library_loader/NativeLibraries',

# Currently required when enable_chrome_android_internal=true.
'com/google/protos/research/ink/InkEventProto',
'ink_sdk/com/google/protobuf/Internal$EnumVerifier',
'ink_sdk/com/google/protobuf/MessageLite',
'com/google/protobuf/GeneratedMessageLite$GeneratedExtension',

# Referenced from GeneratedExtensionRegistryLite.
# Exists only for Chrome Modern (not Monochrome nor Trichrome).
# TODO(agrieve): Figure out why. Perhaps related to Feed V2.
('com/google/wireless/android/play/playlog/proto/ClientAnalytics$'
'ClientInfo'),

# TODO(agrieve): Exclude these only when use_jacoco_coverage=true.
'Ljava/lang/instrument/ClassFileTransformer',
'Ljava/lang/instrument/IllegalClassFormatException',
'Ljava/lang/instrument/Instrumentation',
'Ljava/lang/management/ManagementFactory',
'Ljavax/management/MBeanServer',
'Ljavax/management/ObjectInstance',
'Ljavax/management/ObjectName',
'Ljavax/management/StandardMBean',
]

had_unfiltered_items = ' ' in stderr
stderr = build_utils.FilterLines(
stderr, '|'.join(re.escape(x) for x in ignored_lines))
if stderr:
if ' ' in stderr:
stderr = """
DEX contains references to non-existent symbols after R8 optimization.
Tip: Build with:
is_java_debug=false
treat_warnings_as_errors=false
enable_proguard_obfuscation=false
and then use dexdump to see which class(s) reference them.
E.g.:
third_party/android_sdk/public/build-tools/*/dexdump -d \
out/Release/apks/YourApk.apk > dex.txt
""" + stderr
elif had_unfiltered_items:
# Left only with empty headings. All indented items filtered out.
stderr = ''
return stderr

logging.debug('cmd: %s', ' '.join(cmd))
build_utils.CheckOutput(cmd,
print_stdout=True,
stderr_filter=stderr_filter,
fail_on_output=warnings_as_errors)


def _CombineConfigs(configs, dynamic_config_data, exclude_generated=False):
ret = []

Expand Down Expand Up @@ -458,6 +550,16 @@ def main():
_OptimizeWithR8(options, proguard_configs, libraries, dynamic_config_data,
print_stdout)

if not options.disable_checks:
logging.debug('Running tracereferences')
all_dex_files = []
if options.output_path:
all_dex_files.append(options.output_path)
if options.dex_dests:
all_dex_files.extend(options.dex_dests)
_CheckForMissingSymbols(options.r8_path, all_dex_files, options.classpath,
options.warnings_as_errors)

for output in options.extra_mapping_output_paths:
shutil.copy(options.mapping_output, output)

Expand Down
7 changes: 4 additions & 3 deletions build/config/android/internal_rules.gni
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1245,8 +1245,9 @@ if (enable_java_templates) {
_args += [ "--disable-outlining" ]
}

if (defined(invoker.disable_checkdiscard) && invoker.disable_checkdiscard) {
_args += [ "--disable-checkdiscard" ]
if (defined(invoker.enable_proguard_checks) &&
!invoker.enable_proguard_checks) {
_args += [ "--disable-checks" ]
}

if (defined(invoker.is_static_library) && invoker.is_static_library) {
Expand Down Expand Up @@ -1487,8 +1488,8 @@ if (enable_java_templates) {
"data",
"data_deps",
"deps",
"disable_checkdiscard",
"disable_r8_outlining",
"enable_proguard_checks",
"expected_proguard_config",
"expected_proguard_config_base",
"is_static_library",
Expand Down
15 changes: 7 additions & 8 deletions build/config/android/rules.gni
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2139,8 +2139,8 @@ if (enable_java_templates) {
# (optional).
# product_config_java_packages: Optional list of java packages. If given, a
# ProductConfig.java file will be generated for each package.
# disable_checkdiscard: Turn off -checkdiscard directives in the proguard
# step.
# enable_proguard_checks: Turns on -checkdiscard directives and missing
# symbols check in the proguard step (default=true).
# disable_r8_outlining: Turn off outlining during the proguard step.
# annotation_processor_deps: List of java_annotation_processor targets to
# use when compiling the sources given to this target (optional).
Expand Down Expand Up @@ -2387,7 +2387,6 @@ if (enable_java_templates) {

if (_is_static_library_provider) {
_shared_resources_allowlist_target = _resource_ids_provider_dep
disable_checkdiscard = true
} else if (defined(invoker.shared_resources_allowlist_target)) {
_shared_resources_allowlist_target =
invoker.shared_resources_allowlist_target
Expand Down Expand Up @@ -2902,8 +2901,8 @@ if (enable_java_templates) {
if (defined(invoker.negative_main_dex_globs)) {
not_needed(invoker, [ "negative_main_dex_globs" ])
}
if (defined(invoker.disable_checkdiscard)) {
not_needed(invoker, [ "disable_checkdiscard" ])
if (defined(invoker.enable_proguard_checks)) {
not_needed(invoker, [ "enable_proguard_checks" ])
}
if (defined(invoker.disable_r8_outlining)) {
not_needed(invoker, [ "disable_r8_outlining" ])
Expand All @@ -2922,9 +2921,9 @@ if (enable_java_templates) {
dex(_final_dex_target_name) {
forward_variables_from(invoker,
[
"disable_checkdiscard",
"disable_r8_outlining",
"dexlayout_profile",
"enable_proguard_checks",
])
min_sdk_version = _min_sdk_version
proguard_enabled = _proguard_enabled
Expand Down Expand Up @@ -3477,13 +3476,13 @@ if (enable_java_templates) {
"data_deps",
"deps",
"dexlayout_profile",
"disable_checkdiscard",
"disable_r8_outlining",
"dist_ijar_path",
"enable_lint",
"enable_jetify",
"enable_multidex",
"enable_native_mocks",
"enable_proguard_checks",
"enforce_resource_overlays_in_tests",
"expected_android_manifest",
"expected_android_manifest_base",
Expand Down Expand Up @@ -3831,7 +3830,7 @@ if (enable_java_templates) {
if (defined(invoker.proguard_configs)) {
proguard_configs += invoker.proguard_configs
}
disable_checkdiscard = true
enable_proguard_checks = false
if (defined(invoker.final_apk_path)) {
_final_apk_path = final_apk_path
} else {
Expand Down
1 change: 1 addition & 0 deletions components/cronet/android/BUILD.gn
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1188,6 +1188,7 @@ if (!is_component_build) {
"//base/android/proguard/chromium_apk.flags",
"//testing/android/proguard_for_test.flags",
]
enable_proguard_checks = false
}
}

Expand Down
4 changes: 3 additions & 1 deletion remoting/android/remoting_apk_tmpl.gni
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,9 @@ template("remoting_apk_tmpl") {
if (!is_java_debug) {
proguard_enabled = true
enable_multidex = false
disable_checkdiscard = true

# -checkdiscard checks fail due to -dontoptimize.
enable_proguard_checks = false
if (!defined(proguard_configs)) {
proguard_configs = []
}
Expand Down

0 comments on commit 1f56ae5

Please sign in to comment.