From 67f02bbaaba9da8463e82b6ca40dfaf79ff73be3 Mon Sep 17 00:00:00 2001 From: Andrew Grieve Date: Fri, 6 Nov 2020 04:51:06 +0000 Subject: [PATCH] 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 Reviewed-by: Peter Kotwicz Cr-Commit-Position: refs/heads/master@{#824739} --- build/android/gyp/proguard.py | 121 ++++++++++++++++++++++-- build/config/android/internal_rules.gni | 7 +- build/config/android/rules.gni | 15 ++- components/cronet/android/BUILD.gn | 1 + remoting/android/remoting_apk_tmpl.gni | 4 +- 5 files changed, 126 insertions(+), 22 deletions(-) diff --git a/build/android/gyp/proguard.py b/build/android/gyp/proguard.py index 66b6cabc063d0a..19b5ad0e6e1b8f 100755 --- a/build/android/gyp/proguard.py +++ b/build/android/gyp/proguard.py @@ -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', @@ -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 += [ @@ -329,6 +324,102 @@ 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;(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', + + # 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 = [] @@ -458,6 +549,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) diff --git a/build/config/android/internal_rules.gni b/build/config/android/internal_rules.gni index 2241b5873c2c91..e7005b336c99e3 100644 --- a/build/config/android/internal_rules.gni +++ b/build/config/android/internal_rules.gni @@ -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) { @@ -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", diff --git a/build/config/android/rules.gni b/build/config/android/rules.gni index 1a0623edd2c71a..f547bf8e960db8 100644 --- a/build/config/android/rules.gni +++ b/build/config/android/rules.gni @@ -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). @@ -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 @@ -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" ]) @@ -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 @@ -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", @@ -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 { diff --git a/components/cronet/android/BUILD.gn b/components/cronet/android/BUILD.gn index 5461d0e8abbfb6..b5462483305506 100644 --- a/components/cronet/android/BUILD.gn +++ b/components/cronet/android/BUILD.gn @@ -1188,6 +1188,7 @@ if (!is_component_build) { "//base/android/proguard/chromium_apk.flags", "//testing/android/proguard_for_test.flags", ] + enable_proguard_checks = false } } diff --git a/remoting/android/remoting_apk_tmpl.gni b/remoting/android/remoting_apk_tmpl.gni index 80fb2e6e1cc8f7..7e7805157b42bb 100644 --- a/remoting/android/remoting_apk_tmpl.gni +++ b/remoting/android/remoting_apk_tmpl.gni @@ -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 = [] }