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

Add ability to add comments through the sym file #17

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rbong
Copy link
Contributor

@rbong rbong commented Jan 5, 2021

I'm working on a large disassembly project and I'd like to be able to share it and add comments, but I can't include the disassembled binary with anything I share for licensing reasons. This branch adds the ability to insert comments into the assembly file via specially formatted comments in the symbol file.

There are some restrictions. You currently can't put comments before labels, only after. You cannot put comments at the end of lines. If a comment is in the middle of data or an instruction, the comment is just placed before the line instead of breaking anything up.

This was the simplest solution I could find. Please let me know if you'd like me to change how comments are added, if you'd like this change at all.

@mattcurrie
Copy link
Owner

Hey, sorry just saw this PR! I think it would be great to include the abililty to add comments to the disassembly.

Do you have any ideas how we might support comments being (optionally) placed before labels, and at the end of lines?

Also I'm not sure if your current PR includes it, but it would be good to be able to support multiline comments, and have the option for a comment to be indented to align with source code.

@rbong
Copy link
Contributor Author

rbong commented Feb 13, 2021

Do you have any ideas how we might support comments being (optionally) placed before labels, and at the end of lines?

Maybe options could go before the address?

;; opt1,opt2 02:791a

This is keeping with the options for labels, ex. the options w128,pe4 in .image:500:w128,pe4, but since there's no label to attach the options to, before the address is the only special area where options can be added.

You could also add options right after the address, but this is maybe a little misleading that this has something to do with the address of the comment:

;; 02:791a:opt1,opt2

Let me know which one you like better or if you have another idea.

it would be good to be able to support multiline comments

What is the syntax for multiline comments in this flavor of sym files, or is there one?

Concerning conflicts, I'll fix those up when I get a chance. There's also a bug or two that I've encountered while disassembling.

@rbong
Copy link
Contributor Author

rbong commented Mar 26, 2021

Hey, on my master branch I now have the ability to overwrite left and right operands using the following format:

;; 00:0000:l_op LEFT_OPERAND
;; 00:0000:r_op RIGHT_OPERAND

Might want to consider future cases like this when thinking about how the syntax is going to work. Maybe something like this will work well:

;; 00:0000:(type):(options) (body)

Examples:

;; 00:0000:comment This is a comment
;; 00:0000 This is a comment (uses default type)
;; 00:0000:comment:before_label This is a comment that goes before the label (has an option)
;; 00:0000::before_label This is a comment before the label (uses default type, has options)

Not sure how to do multi-line comments still.

@ISSOtm
Copy link
Collaborator

ISSOtm commented Jul 1, 2021

I think the functionality present as-is is good enough, and is worth merging after rebasing.
Sure, it may be improvable, but such additions can always be merged in a second wave, instead of this never being merged at all ^^

@rbong
Copy link
Contributor Author

rbong commented Jul 5, 2021

Yeah, my only concern is merging syntax that later has to be changed. It seems like it's good though so if @mattcurrie is happy I can fix conflicts and it should be good to merge.

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