Skip to content
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

Fix RemoveUninstantiablesPass in face of Proguard stripping #744

Closed

Conversation

justhecuke
Copy link

@justhecuke justhecuke commented Nov 17, 2022

This was the original problem that caused #718.

RemoveUninstantiablesPass is sensitive to the presence/absence of constructors in its analysis. Proguard will sometimes strip constructors out of classes and cause unsafe optimizations to occur where too many classes removed.

Helpfully, Proguard can output a usage.txt which lists the classes, methods, and fields that were stripped. Using that file, we can inform the optimization of the existence of constructors so that it is more conservative in what it removes.

Note that this was packaged together from a modified internal branch for public upstreaming so there might be some strangeness that needs to be polished.

Fix RemoveUninstantiables's parsing of usage.txt to deobfuscate

See merge request mobile_perf_infra/Redex!158
@NTillmann
Copy link
Contributor

I don't fully understand the motivation yet... If Proguard did strip the constructors of a class, then no instance of it can ever be created, and all remaining uses will fail at runtime. Or am I missing something?

@justhecuke
Copy link
Author

I don't fully understand the motivation yet... If Proguard did strip the constructors of a class, then no instance of it can ever be created, and all remaining uses will fail at runtime. Or am I missing something?

I'm actually confused just like you are. The fix to use usage.txt was done by someone else -- an intern under supervision of someone else -- and I never tracked down why they did it this way. My work on it was to add the obfuscated/deobfuscated layer. It is the case, though, that this fixes things for our app and is guarded by a flag so others do not have to use it.

Let me ask around and see what the motivation was.

@justhecuke
Copy link
Author

Seems like using usage.txt was a hack with no real theoretical basis. I'm working on another way to do things using IPReflectionAnalysis, adding some GSON reflection reachability analysis, and using ClassHierarchy to find children of reflected classes (important in some reflection frameworks that search for classes using Interfaces). Going to drop this change while I get that one ready, although I'm not sure you'll want to take it since it makes quite a few changes and adds IPReflectionAnalysis as a dependency to RemoveUninstantiablesPass.

@justhecuke justhecuke closed this Dec 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants