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

Run indexJob before rename #910

Merged
merged 1 commit into from
Jul 3, 2020
Merged

Run indexJob before rename #910

merged 1 commit into from
Jul 3, 2020

Conversation

bet4it
Copy link
Contributor

@bet4it bet4it commented Apr 19, 2020

The Rename function needs usageInfo to change all the references of the name changed. We need to make sure indexJob has completed before Rename.
Fix #907.

@bet4it
Copy link
Contributor Author

bet4it commented Apr 19, 2020

@S-trace

@skylot
Copy link
Owner

skylot commented Apr 19, 2020

@bet4it looks like your change fixes the issue, but I don't think that decompile all classes to just get dependencies is a good idea. It is better to just reload code for all currently decompiled classes and as I remember that was a goal @S-trace tried to achieve.
Anyway, thanks for the help! I will keep this PR open until @S-trace reviews it or I made a fix for that.

@S-trace
Copy link
Contributor

S-trace commented May 13, 2020

I don't think we really need to decompile classes to get usage information.

This should be possible via parsing raw DEX code, just like simple grep -Flr 'Lsome/FancyClass;' command in the apktool project gives the list of classes which have the references to the some.FancyClass class.

@skylot What do you think about this approach?

@skylot
Copy link
Owner

skylot commented May 13, 2020

@S-trace I agree. Actually, right now I am working on my own dex parsing library which will allow fast traversing through instructions and decode only interesting ones (which contains references to other classes/methods/fields) so we can collect dependencies before decompilation.

@S-trace
Copy link
Contributor

S-trace commented May 13, 2020

@skylot Great!

How do you think, should I try to implement dependencies collection for current dx-based implementation, or just wait for your new DEX parser?

We don't need full search index for renaming, we just need the list of classes having references to the class which has the renamed entity (class/method/field), so I think this can be implemented using a Map<Long, int[][]> stored somewhere.
This map will contain DexID<<32 + ClassID as a key, and 2d array of DEX IDs and Class IDs inside each DEX. as a value.

So, the reverse dependencies lookup it will be possible without complete classes traversal, just by Map.get(DexID<<32 + ClassID), and storage will be very compact and fast because it will be based on int arrays, not Objects.

This Map can be filled using temporary Map<Long, Set<Integer>[]> Map (which maps each full ClassID to the array of Sets (one Set for each DEX id) and then (when all DEX files were loaded) this temporary Map could be flattened to final Map<Long, int[][]>.

What do you think about this way to store deps for renaming functionality?

UPD2:
0001-core-ReverseDependencies-A-basic-idea-of-reverse-dep.patch.txt
Here is a basic implementation of this idea (tracks the methods args types and return value type, fields types and inheritance of classes, does not track references in methods bodies), implements toString() to dump internal state and getReverseDependencies(ClassNode) to get the Set of classes names which have references to a given class.
Could you please review it and maybe give me a bit of advice about tracking refs inside methods bodies?

Thank you.

@skylot
Copy link
Owner

skylot commented May 14, 2020

@S-trace

How do you think, should I try to implement dependencies collection for current dx-based implementation, or just wait for your new DEX parser?

I don't want to implement the same thing twice, so there is no point in doing it right now.

Map<Long, int[][]> - is very low level and has no advantage over simple List<ClassNode> inside ClassNode class like it is already done for "direct" dependencies. The main issue is that we need to convert from ids to ClassNode node objects every time and it kills all performance gain from using primitive arrays.
Also, I want to move all raw dex info and methods into new lib, so I will remove DexNode class and all low-level dex ids.

Could you please review it and maybe give me a bit of advice about tracking refs inside methods bodies?

Currently, such tracking implemented in DependencyCollector but it works on high-level instruction objects which require to load all method instructions, so now it runs as a visitor in class decompilation.

Also, I want to mention that dependencies info will be used not only for rename feature but to resolve other issues like:

  • detect and inline anonymous classes (to be sure it used only in one place)
  • classes decompilation order, especially to solve indeterministic results in multi-threaded decompilation
  • maybe "find usage" in jadx-gui
  • other issues which require class or methods modifications

@S-trace
Copy link
Contributor

S-trace commented May 14, 2020

Ok, so I think this pull request resolves the issue for now.

Attached rebased over #935 patch from this pull request.
@bet4it - could you please apply it to avoid merge conflicts?
You can use git push --force to update the branch for this pull request - it will update the pull request automatically.

But I have one question about the visual part:
For now, rename dialog looks really weird for me (screenshot attached).
I think it should not be that high - @bet4it maybe, you can make it slimmer?

Thank you.

0001-Run-indexJob-before-rename.patch.txt
Screenshot_20200514_224517

@bet4it
Copy link
Contributor Author

bet4it commented May 15, 2020

I didn't change the size of the dialog, jadx uses the size of the dialog at last time.
If you switch to your commit, the dialog should still be so high.
And you can resize this dialog to make it smaller.
Maybe we need to lock the size of dialog so user can't resize it, but I have no idea how to do it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants