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

improve smali printer to show bytecode #1126

Merged
merged 2 commits into from
Mar 2, 2021

Conversation

busmaker
Copy link
Contributor

@busmaker busmaker commented Mar 2, 2021

#1114
All the instruction decoding jobs had been done by @skylot since don't know when, I just put them together to print smali code.

added 3 columns to the output,

  • first column is a file offset within the dex file
  • the second is bytecode
  • the third one is the code offset within the method.

and added an addtional font setting it contains fixed-width fonts, for alignment of output.

smali_bytecode_present

@lgtm-com
Copy link

lgtm-com bot commented Mar 2, 2021

This pull request introduces 1 alert when merging 4dd7ef1 into 3a69ac2 - view on LGTM.com

new alerts:

  • 1 for Array index out of bounds

Copy link
Contributor

@Surendrajat Surendrajat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice job @lbj-the-goat. I've a few comments:

  • I think ByteCode has a very different meaning in Java world. Here simply using Bytes or OpCode Bytes is more relevent.
  • Does the last comment ie. field/method offset (eg. method@0123) represent absolute address in dex? Or related to something? I may be mistaken about something here :)
  • How much work might it require for synchronizing smali and java methods? Like scrolling in some other method moves cursor in related java code too. I'm sure it won't work for innerclasses right now. Just a thought.

@busmaker
Copy link
Contributor Author

busmaker commented Mar 2, 2021

Nice job @lbj-the-goat. I've a few comments:

  • I think ByteCode has a very different meaning in Java world. Here simply using Bytes or OpCode Bytes is more relevent.
  • Does the last comment ie. field/method offset (eg. method@0123) represent absolute address in dex? Or related to something? I may be mistaken about something here :)
  • How much work might it require for synchronizing smali and java methods? Like scrolling in some other method moves cursor in related java code too. I'm sure it won't work for innerclasses right now. Just a thought.
  • Okay, I think Btys is better.
  • it's an offset to the start of the code item or something, I added it because the output of dexdump has it, more importantly it looks pretty pro, haha...
  • It's a good idea, It might take a while, I just simply print the result now, it needs attach line info to do it.

swithed line 62 and line 63, to get the proper bytes, insnStart must to be set before start to decode.
@Surendrajat
Copy link
Contributor

Surendrajat commented Mar 2, 2021

Okay, I think Btys is better.

Cool. This will avoid the confusion.

more importantly it looks pretty pro, haha...

Yeah. Then it needs to go in 🤣 (seriously though I still have no idea what it is :))

I just simply print the result now, it needs attach line info to do it.

I see. Cool. It'll be a nice feature TBH. Till now Smali pane is not so useful for me.

@busmaker busmaker requested a review from skylot March 2, 2021 12:54
@lgtm-com
Copy link

lgtm-com bot commented Mar 2, 2021

This pull request introduces 1 alert when merging 598ec29 into 3a69ac2 - view on LGTM.com

new alerts:

  • 1 for Array index out of bounds

@skylot skylot merged commit 6508638 into skylot:master Mar 2, 2021
@busmaker busmaker deleted the master_smali_bytecode branch March 2, 2021 13:04
@Surendrajat
Copy link
Contributor

Seems like we're going to have Bytecodes for now.

@skylot
Copy link
Owner

skylot commented Mar 2, 2021

Seems like we're going to have Bytecodes for now.

Oops! Did I merged too early? Sorry 😞

@Surendrajat
Copy link
Contributor

Since @lbj-the-goat agreed for Bytes, I was thinking so. But it can be done in a follow up PR too.

@busmaker
Copy link
Contributor Author

busmaker commented Mar 2, 2021

Seems like we're going to have Bytecodes for now.

Oops! Did I merged too early? Sorry 😞

@Surendrajat Haha.. I was typing and then I saw it merged, so just continued to catch up some TV shows

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