-
-
Notifications
You must be signed in to change notification settings - Fork 8
feat(r8-tests): Add R8 ambiguous tests #71
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
base: master
Are you sure you want to change the base?
Conversation
| "; | ||
|
|
||
| let expected = "\ | ||
| com.android.tools.r8.CompilationException: foo[parens](Source:3) |
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.
R8's expected output looks like this:
"com.android.tools.r8.CompilationException: foo[parens](Source:3)",
--
" at com.android.tools.r8.R8.bar(R8.java:7)",
" <OR> at com.android.tools.r8.R8.foo(R8.java:7)",
" at com.android.tools.r8.R8.bar(R8.java:8)",
" <OR> at com.android.tools.r8.R8.foo(R8.java:8)",
" at com.android.tools.r8.R8.main(Unknown Source)",
"Caused by: com.android.tools.r8.CompilationException: foo[parens](Source:3)",
" at com.android.tools.r8.R8.bar(R8.java:9)",
" <OR> at com.android.tools.r8.R8.foo(R8.java:9)",
" ... 42 more
but our UI does not really support the <OR> case, so I'm unsure if we should tackle this now? Ideally though, we should, because the current remapped stacktrace can be quite misleading.
|
|
||
| let expected = "\ | ||
| com.android.tools.r8.CompilationException: | ||
| at com.android.tools.r8.R8.foo(R8.java:42) |
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.
Similar here -- retrace actually expands this to mention all of the potential inline frames:
"com.android.tools.r8.CompilationException:",
--
" at com.android.tools.r8.R8.foo(R8.java:42)",
" <OR> at com.android.tools.r8.R8.foo(R8.java:43)",
" <OR> at com.android.tools.r8.R8.foo(R8.java:44)",
" at com.android.tools.r8.R8.bar(R8.java:32)",
" at com.android.tools.r8.R8.baz(R8.java:10)
| "; | ||
|
|
||
| let expected = "\ | ||
| java.lang.IndexOutOfBoundsException |
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.
Here the order in retrace is different:
"java.lang.IndexOutOfBoundsException",
--
"\tat some.inlinee1(some.java:10)",
"\t<OR> at some.inlinee2(some.java:20)",
"\tat com.android.tools.r8.Internal.foo(Internal.java:10)",
"\t<OR> at com.android.tools.r8.Internal.foo(Internal.java:42)
seems like retrace has two alternative expansion paths for a single obfuscated frame:
Alt A: some.inlinee1 → Internal.foo(:10)
Alt B: some.inlinee2 → Internal.foo(:42)
When it prints with <OR>, it’s not printing “Alt A then Alt B”, but a column-wise merge by stack depth:
depth 0: inlinee1
depth 0 alt: inlinee2
depth 1: Internal.foo(:10)
depth 1 alt: Internal.foo(:42)
I guess this also depends on the decision if we're going to implement alternative stacktraces or not
|
@loewenheim I've updated tests to match the exact expected output from retrace, and have a couple of questions that we need to make a decision on, hence re-requested a review |
|
I would honestly not support these alternative stacktraces for now—I don't understand how they work and can't see how we would consume them. |
Part of #40
There are failing tests in this PR that need fixes in the mapper/cache itself, so I will open a follow-up PR that targets this branch.
R8 Retrace Test Coverage