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

Support ARM assembly. #25

Merged
merged 1 commit into from
Mar 27, 2022
Merged

Support ARM assembly. #25

merged 1 commit into from
Mar 27, 2022

Conversation

yugr
Copy link
Collaborator

@yugr yugr commented Mar 17, 2022

No description provided.

@codecov
Copy link

codecov bot commented Mar 17, 2022

Codecov Report

Merging #25 (4259c94) into main (9c59f8e) will increase coverage by 0.55%.
The diff coverage is 83.75%.

@@            Coverage Diff             @@
##             main      #25      +/-   ##
==========================================
+ Coverage   74.27%   74.82%   +0.55%     
==========================================
  Files           4        4              
  Lines         381      437      +56     
==========================================
+ Hits          283      327      +44     
- Misses         98      110      +12     
Impacted Files Coverage Δ
src/asm2cfg/command_line.py 0.00% <0.00%> (ø)
src/gdb_asm2cfg.py 75.47% <66.66%> (-2.79%) ⬇️
src/asm2cfg/asm2cfg.py 77.98% <88.40%> (+0.80%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9c59f8e...4259c94. Read the comment docs.

@@ -23,6 +23,7 @@ def escape(instruction):
instruction = instruction.replace('|', r'\|')
instruction = instruction.replace('{', r'\{')
instruction = instruction.replace('}', r'\}')
instruction = instruction.replace(' ', ' ')
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

LHS is tabulation character.

Copy link
Owner

@Kazhuu Kazhuu left a comment

Choose a reason for hiding this comment

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

Great start! I left few comments that I think should be checked. So this PR will add support for objdump style for ARM, not GDB disassembly support? If so could you add more test cases that test the special cases as well? More so because when other people touch upon this and they don't have ARM platform to test with, then the tests make sure everything is in working order :)

src/asm2cfg/asm2cfg.py Outdated Show resolved Hide resolved
class Instruction:
"""
Represents a single assembly instruction with it operands, location and
optional branch target
"""
def __init__(self, body, text, lineno, address, opcode, ops, target, imm): # pylint: disable=too-many-arguments
def __init__(self, body, text, lineno, address, opcode, ops, target, imm, target_info): # noqa
Copy link
Owner

Choose a reason for hiding this comment

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

Indeed this class start to have too many parameters. I guess it's okay for now but definitely this needs some attention in the future.

src/asm2cfg/asm2cfg.py Outdated Show resolved Hide resolved
@yugr
Copy link
Collaborator Author

yugr commented Mar 19, 2022

So this PR will add support for objdump style for ARM, not GDB disassembly support?

Actually I tested with simple GDB disassemblies and it worked.

@yugr
Copy link
Collaborator Author

yugr commented Mar 19, 2022

As a side note, here's how one can test ARM code on Ubuntu:

$ sudo apt-get install qemu-user gcc-arm-linux-gnueabi g++-arm-linux-gnueabi binutils-arm-linux-gnueabi libc6-armel-cross libc6-dev-armel-cross gdb-multiarch
$ arm-linux-gnueabi-gcc prog.c
$ qemu-arm -L /usr/arm-linux-gnueabi -g 1234 ./a.out &
$ gdb-multiarch
(gdb) set arch armv7
(gdb) file a.out
(gdb) target remote :1234
(gdb) b main
(gdb) c

Copy link
Owner

@Kazhuu Kazhuu left a comment

Choose a reason for hiding this comment

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

I tested this locally and works good. Good job on this one! Thanks again and sorry for the late review!

Couple of things that I'll keep in my mind when continue working on this:

  • Add instructions how to test arm platform on Ubuntu.
  • Add more about arm support to readme.
  • Improve error message when wrong target flag is used that maybe you should try other target instead.
  • Provide -t flag beside --target flag.
  • Split big asm2cfg to other files

@Kazhuu Kazhuu merged commit b3281cf into Kazhuu:main Mar 27, 2022
@Kazhuu Kazhuu mentioned this pull request Mar 27, 2022
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.

2 participants