-
-
Notifications
You must be signed in to change notification settings - Fork 8
feat(r8-tests): Add R8 source file tests #74
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
romtsn
wants to merge
3
commits into
rz/feat/r8-tests-synthetic
Choose a base branch
from
rz/feat/r8-tests-source-files
base: rz/feat/r8-tests-synthetic
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,60 @@ | ||
| # R8 Retrace: Source File Edge Cases — Remaining Fixes | ||
|
|
||
| After normalizing fixture indentation (member mappings use 4 spaces), `tests/r8-source-file-edge-cases.rs` has **4 passing** and **3 failing** tests. | ||
|
|
||
| This note documents, **one-by-one**, what still needs fixing in the crate (not in the fixtures) to make the remaining tests pass. | ||
|
|
||
| ## 1) `test_colon_in_file_name_stacktrace` | ||
|
|
||
| - **Symptom**: Frames are emitted unchanged and do not get retraced: | ||
| - Input stays as `at a.s(:foo::bar:1)` / `at a.t(:foo::bar:)`. | ||
| - **Root cause**: `src/stacktrace.rs::parse_frame` splits `(<file>:<line>)` using the **first** `:`. | ||
| - For `(:foo::bar:1)`, the first split produces `file=""` and `line="foo::bar:1"`, so parsing the line number fails and the whole frame is rejected. | ||
| - **Fix needed**: | ||
| - In `parse_frame`, split `file:line` using the **last** colon (`rsplit_once(':')`) so file names can contain `:` (Windows paths and this fixture). | ||
| - Treat an empty or non-numeric suffix after the last colon as “no line info” (line `0`) instead of rejecting the frame. | ||
|
|
||
| ## 2) `test_file_name_extension_stacktrace` | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. skip this |
||
|
|
||
| This failure is due to two independent gaps. | ||
|
|
||
| ### 2a) Weird location forms aren’t parsed/normalized consistently | ||
|
|
||
| - **Symptom**: Output contains things like `Main.java:` and `R8.foo:0` instead of normalized `R8.java:0` for “no line” cases. | ||
| - **Root cause**: `parse_frame` only supports: | ||
| - `(<file>:<number>)`, or | ||
| - `(<file>)` (treated as line `0`), | ||
| and it currently rejects or mis-interprets inputs like: | ||
| - `(Native Method)`, `(Unknown Source)`, `(Unknown)`, `()` | ||
| - `(Main.java:)` (empty “line” part) | ||
| - `(Main.foo)` (no `:line`, but also not a normal source file extension) | ||
| - **Fix needed**: | ||
| - Make `parse_frame` permissive for these Java stacktrace forms and interpret them as a parsed frame with **line `0`** so remapping can then replace the file with the mapping’s source file (here: `R8.java`). | ||
| - Also apply the “split on last colon” rule from (1) so `file:line` parsing is robust. | ||
|
|
||
| ### 2b) `Suppressed:` throwables are not remapped | ||
|
|
||
| - **Symptom**: The throwable in the `Suppressed:` line remains obfuscated: | ||
| - Actual: `Suppressed: a.b.c: You have to write the program first` | ||
| - Expected: `Suppressed: foo.bar.baz: You have to write the program first` | ||
| - **Root cause**: `src/mapper.rs::remap_stacktrace` remaps: | ||
| - the first-line throwable, and | ||
| - `Caused by: ...`, | ||
| but it does **not** detect/handle `Suppressed: ...`. | ||
| - **Fix needed**: | ||
| - Add handling for the `Suppressed: ` prefix analogous to `Caused by: `: | ||
| - parse the throwable after the prefix, | ||
| - remap it, | ||
| - emit with the same prefix. | ||
|
|
||
| ## 3) `test_class_with_dash_stacktrace` | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. skip this |
||
|
|
||
| - **Symptom**: An extra frame appears: | ||
| - Actual includes `Unused.staticMethod(Unused.java:0)` in addition to `I.staticMethod(I.java:66)`. | ||
| - **Root cause**: The mapping includes synthesized metadata (`com.android.tools.r8.synthesized`) and multiple plausible remapped frames, including synthesized “holder/bridge” frames. | ||
| - Today we emit all candidates rather than preferring non-synthesized frames. | ||
| - **Fix needed**: | ||
| - Propagate the synthesized marker into `StackFrame.method_synthesized` during mapping. | ||
| - When multiple candidate remapped frames exist for one obfuscated frame, **filter synthesized frames** if any non-synthesized frames exist (or apply an equivalent preference rule). | ||
|
|
||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,215 @@ | ||
| //! Tests for R8 retrace "Source File Edge Cases" fixtures. | ||
| //! | ||
| //! These tests are ported from the upstream R8 retrace fixtures in: | ||
| //! `src/test/java/com/android/tools/r8/retrace/stacktraces/`. | ||
| #![allow(clippy::unwrap_used)] | ||
|
|
||
| use proguard::{ProguardCache, ProguardMapper, ProguardMapping}; | ||
|
|
||
| fn assert_remap_stacktrace(mapping: &str, input: &str, expected: &str) { | ||
| let mapper = ProguardMapper::from(mapping); | ||
| let actual = mapper.remap_stacktrace(input).unwrap(); | ||
| assert_eq!(actual.trim_end(), expected.trim_end()); | ||
|
|
||
| let mapping = ProguardMapping::new(mapping.as_bytes()); | ||
| let mut buf = Vec::new(); | ||
| ProguardCache::write(&mapping, &mut buf).unwrap(); | ||
| let cache = ProguardCache::parse(&buf).unwrap(); | ||
| cache.test(); | ||
|
|
||
| let actual = cache.remap_stacktrace(input).unwrap(); | ||
| assert_eq!(actual.trim_end(), expected.trim_end()); | ||
| } | ||
|
|
||
| // ============================================================================= | ||
| // ColonInFileNameStackTrace | ||
| // ============================================================================= | ||
|
|
||
| const COLON_IN_FILE_NAME_MAPPING: &str = "\ | ||
| some.Class -> a: | ||
| # {\"id\":\"sourceFile\",\"fileName\":\"Class.kt\"} | ||
| 1:3:int strawberry(int):99:101 -> s | ||
| 4:5:int mango(float):121:122 -> s | ||
| int passionFruit(float):121:121 -> t | ||
| "; | ||
|
|
||
| #[test] | ||
| fn test_colon_in_file_name_stacktrace() { | ||
| let input = r#" at a.s(:foo::bar:1) | ||
| at a.t(:foo::bar:) | ||
| "#; | ||
|
|
||
| let expected = r#" at some.Class.strawberry(Class.kt:99) | ||
| at some.Class.passionFruit(Class.kt:121) | ||
| "#; | ||
|
|
||
| assert_remap_stacktrace(COLON_IN_FILE_NAME_MAPPING, input, expected); | ||
| } | ||
|
|
||
| // ============================================================================= | ||
| // UnicodeInFileNameStackTrace | ||
| // ============================================================================= | ||
|
|
||
| const UNICODE_IN_FILE_NAME_MAPPING: &str = "\ | ||
| some.Class -> a: | ||
| # {\"id\":\"sourceFile\",\"fileName\":\"Class.kt\"} | ||
| 1:3:int strawberry(int):99:101 -> s | ||
| 4:5:int mango(float):121:122 -> s | ||
| "; | ||
|
|
||
| #[test] | ||
| fn test_unicode_in_file_name_stacktrace() { | ||
| let input = r#" at a.s(Blåbærgrød.jàvà:1) | ||
| "#; | ||
|
|
||
| // Normalize indentation to this crate's output (`" at ..."`) | ||
| let expected = r#" at some.Class.strawberry(Class.kt:99) | ||
| "#; | ||
|
|
||
| assert_remap_stacktrace(UNICODE_IN_FILE_NAME_MAPPING, input, expected); | ||
| } | ||
|
|
||
| // ============================================================================= | ||
| // MultipleDotsInFileNameStackTrace | ||
| // ============================================================================= | ||
|
|
||
| const MULTIPLE_DOTS_IN_FILE_NAME_MAPPING: &str = "\ | ||
| some.Class -> a: | ||
| # {\"id\":\"sourceFile\",\"fileName\":\"Class.kt\"} | ||
| 1:3:int strawberry(int):99:101 -> s | ||
| 4:5:int mango(float):121:122 -> s | ||
| "; | ||
|
|
||
| #[test] | ||
| fn test_multiple_dots_in_file_name_stacktrace() { | ||
| let input = r#" at a.s(foo.bar.baz:1) | ||
| "#; | ||
|
|
||
| let expected = r#" at some.Class.strawberry(Class.kt:99) | ||
| "#; | ||
|
|
||
| assert_remap_stacktrace(MULTIPLE_DOTS_IN_FILE_NAME_MAPPING, input, expected); | ||
| } | ||
|
|
||
| // ============================================================================= | ||
| // FileNameExtensionStackTrace | ||
| // ============================================================================= | ||
|
|
||
| const FILE_NAME_EXTENSION_MAPPING: &str = "\ | ||
| foo.bar.baz -> a.b.c: | ||
| R8 -> R8: | ||
| "; | ||
|
|
||
| #[test] | ||
| fn test_file_name_extension_stacktrace() { | ||
| let input = r#"a.b.c: Problem when compiling program | ||
| at R8.main(App:800) | ||
| at R8.main(Native Method) | ||
| at R8.main(Main.java:) | ||
| at R8.main(Main.kt:1) | ||
| at R8.main(Main.foo) | ||
| at R8.main() | ||
| at R8.main(Unknown) | ||
| at R8.main(SourceFile) | ||
| at R8.main(SourceFile:1) | ||
| Suppressed: a.b.c: You have to write the program first | ||
| at R8.retrace(App:184) | ||
| ... 7 more | ||
| "#; | ||
|
|
||
| let expected = r#"foo.bar.baz: Problem when compiling program | ||
| at R8.main(R8.java:800) | ||
| at R8.main(R8.java:0) | ||
| at R8.main(R8.java:0) | ||
| at R8.main(R8.kt:1) | ||
| at R8.main(R8.java:0) | ||
| at R8.main(R8.java:0) | ||
| at R8.main(R8.java:0) | ||
| at R8.main(R8.java:0) | ||
| at R8.main(R8.java:1) | ||
| Suppressed: foo.bar.baz: You have to write the program first | ||
| at R8.retrace(R8.java:184) | ||
| ... 7 more | ||
| "#; | ||
|
|
||
| assert_remap_stacktrace(FILE_NAME_EXTENSION_MAPPING, input, expected); | ||
| } | ||
|
|
||
| // ============================================================================= | ||
| // SourceFileNameSynthesizeStackTrace | ||
| // ============================================================================= | ||
|
|
||
| const SOURCE_FILE_NAME_SYNTHESIZE_MAPPING: &str = "\ | ||
| android.support.v7.widget.ActionMenuView -> mapping: | ||
| 21:21:void invokeItem():624 -> a | ||
| android.support.v7.widget.ActionMenuViewKt -> mappingKotlin: | ||
| 21:21:void invokeItem():624 -> b | ||
| "; | ||
|
|
||
| #[test] | ||
| fn test_source_file_name_synthesize_stacktrace() { | ||
| let input = r#" at mapping.a(AW779999992:21) | ||
| at noMappingKt.noMapping(AW779999992:21) | ||
| at mappingKotlin.b(AW779999992:21) | ||
| "#; | ||
|
|
||
| let expected = r#" at android.support.v7.widget.ActionMenuView.invokeItem(ActionMenuView.java:624) | ||
| at noMappingKt.noMapping(AW779999992:21) | ||
| at android.support.v7.widget.ActionMenuViewKt.invokeItem(ActionMenuView.kt:624) | ||
| "#; | ||
|
|
||
| assert_remap_stacktrace(SOURCE_FILE_NAME_SYNTHESIZE_MAPPING, input, expected); | ||
| } | ||
|
|
||
| // ============================================================================= | ||
| // SourceFileWithNumberAndEmptyStackTrace | ||
| // ============================================================================= | ||
|
|
||
| const SOURCE_FILE_WITH_NUMBER_AND_EMPTY_MAPPING: &str = "\ | ||
| com.android.tools.r8.R8 -> com.android.tools.r8.R8: | ||
| 34:34:void com.android.tools.r8.utils.ExceptionUtils.withR8CompilationHandler(com.android.tools.r8.utils.Reporter,com.android.tools.r8.utils.ExceptionUtils$CompileAction):59:59 -> a | ||
| 34:34:void runForTesting(com.android.tools.r8.utils.AndroidApp,com.android.tools.r8.utils.InternalOptions):261 -> a | ||
| "; | ||
|
|
||
| #[test] | ||
| fn test_source_file_with_number_and_empty_stacktrace() { | ||
| let input = r#" at com.android.tools.r8.R8.a(R.java:34) | ||
| at com.android.tools.r8.R8.a(:34) | ||
| "#; | ||
|
|
||
| let expected = r#" at com.android.tools.r8.utils.ExceptionUtils.withR8CompilationHandler(ExceptionUtils.java:59) | ||
| at com.android.tools.r8.R8.runForTesting(R8.java:261) | ||
| at com.android.tools.r8.utils.ExceptionUtils.withR8CompilationHandler(ExceptionUtils.java:59) | ||
| at com.android.tools.r8.R8.runForTesting(R8.java:261) | ||
| "#; | ||
|
|
||
| assert_remap_stacktrace(SOURCE_FILE_WITH_NUMBER_AND_EMPTY_MAPPING, input, expected); | ||
| } | ||
|
|
||
| // ============================================================================= | ||
| // ClassWithDashStackTrace | ||
| // ============================================================================= | ||
|
|
||
| const CLASS_WITH_DASH_MAPPING: &str = "\ | ||
| # {\"id\":\"com.android.tools.r8.mapping\",\"version\":\"1.0\"} | ||
| Unused -> I$-CC: | ||
| # {\"id\":\"com.android.tools.r8.synthesized\"} | ||
| 66:66:void I.staticMethod() -> staticMethod | ||
| 66:66:void staticMethod():0 -> staticMethod | ||
| # {\"id\":\"com.android.tools.r8.synthesized\"} | ||
| "; | ||
|
|
||
| #[test] | ||
| fn test_class_with_dash_stacktrace() { | ||
| let input = r#"java.lang.NullPointerException | ||
| at I$-CC.staticMethod(I.java:66) | ||
| at Main.main(Main.java:73) | ||
| "#; | ||
|
|
||
| let expected = r#"java.lang.NullPointerException | ||
| at I.staticMethod(I.java:66) | ||
| at Main.main(Main.java:73) | ||
| "#; | ||
|
|
||
| assert_remap_stacktrace(CLASS_WITH_DASH_MAPPING, input, expected); | ||
| } |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
needs fixing