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

update 8051 anal plugin #15321

Merged
merged 9 commits into from
Oct 22, 2019
Merged

update 8051 anal plugin #15321

merged 9 commits into from
Oct 22, 2019

Conversation

hmht
Copy link
Contributor

@hmht hmht commented Oct 21, 2019

handle 8051 ANAL_OP DISASM

The 8051 assembler has been R_APId, #included and called to fill
op->mnemonic, which stops a bunch of warnings from appearing
whenever a bunch of disassembly appears on the screen.

The disassembler is called because of comments in r_anal.h: op->mnemonic
should contain the entire disassembly, not just the mnemonic.
Here's hoping the mnemonics and arguments will get split eventually.

typdef some RAnal enums

In trying to make my analysis push out more info, it's difficult to
understand what all these ints mean, while ACTUALLY they should be
filled with enum values. By using the enum names, that's made clear
immediately.

r2's style is typedef over enum name, so that's what I did.

the typedef-instead-of-int I added here isn't consistently propagated,
and has caused warnings about unhandled cases-in-switch, at least some
of which should just get a default: case added, but I'd rather leave it
to the domain experts, or my future self when I become that domain
expert.

add more basic info

cycles, cond, eob, nopcode, family, id

hmht added 6 commits October 21, 2019 12:39
The 8051 assembler has been R_APId, #included in the local scope, and
called to fill op->mnemonic, which stops a bunch of warnings from
appearing whenever a bunch of disassembly appears on the screen.

The disassembler is called because of comments in r_anal.h: op->mnemonic
should contain the entire disassembly, not just the mnemonic.
Here's hoping the mnemonics and arguments will get split eventually.
In trying to make my analysis push out more info, it's difficult to
understand what all these ints mean, while ACTUALLY they should be
filled with enum values. By using the enum names, that's made clear
immediately.

r2's style is typedef over `enum name`, so that's what I did.

the typedef-instead-of-int I added here isn't consistently propagated,
and has caused warnings about unhandled cases-in-switch, at least some
of which should just get a default: case added, but I'd rather leave it
to the domain experts, or my future self when I become that domain
expert.
it's pretty much static data with very meagre duplication, putting it in
a switch-case is unwieldy.
op->cond, eob, nopcode, family, id

they seem mostly useless, but it's not much work to add, so...
@lgtm-com
Copy link

lgtm-com bot commented Oct 21, 2019

This pull request introduces 1 alert when merging 9d956c5 into 44b7de0 - view on LGTM.com

new alerts:

  • 1 for Function declared in block

@radare
Copy link
Collaborator

radare commented Oct 21, 2019

Screenshot 2019-10-21 at 15 44 12

Copy link
Collaborator

@radare radare left a comment

Choose a reason for hiding this comment

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

.

libr/anal/p/anal_8051.c Outdated Show resolved Hide resolved
:)

Clearly I was using MASK_VAL incorrectly, the only hint to its use was:

> // It fills RAnalop->dst/src info

so even though it's named "VAL", it might not actually need
to fill RAnalop->val...

Nor ptr, and definitely not jump and fail, which means we don't have use
for it yet! Not until I actually implement ->dst/src
@radare
Copy link
Collaborator

radare commented Oct 22, 2019

if the code calling ranalop needs VAL to work, it should be passing |VAL in the mask

@radare radare merged commit 3a0a477 into radareorg:master Oct 22, 2019
@marcograss
Copy link
Contributor

this broke the build on macOS btw

@radare
Copy link
Collaborator

radare commented Oct 23, 2019 via email

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