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

Disable asm.dwarf #2600

Closed
wants to merge 1 commit into from
Closed

Disable asm.dwarf #2600

wants to merge 1 commit into from

Conversation

xarkes
Copy link
Member

@xarkes xarkes commented Feb 14, 2021

This commit sets the asm.dwarf option to false by default and adds a
checkbox in the configuration menu to enable it.

Your checklist for this pull request

Detailed description

By default, asm.dwarf is enabled. This adds a lot of verbosity to the code especially source code file lines and such which most of the time are not relevant during reverse engineering tasks. Disabling it by default makes the interface much cleaner.

image
image

Test plan (required)

  • Open a binary with dwarf information, make sure that e asm.dwarf is set to false
  • Enable it in the configuration panel and check that the extra comments appear/disappear as one would expect

This commit sets the asm.dwarf option to false by default and adds a
checkbox in the configuration menu to enable it.
@Maijin
Copy link
Member

Maijin commented Feb 14, 2021

I think would be nice to only enable the option if there is actual dwarf - maybe it should also notify the user?

@xarkes
Copy link
Member Author

xarkes commented Feb 14, 2021

I added screenshots in case it was not clear. I especially want to disable it when dwarf information is present, as you can see there are too many useless information.

@Maijin
Copy link
Member

Maijin commented Feb 14, 2021

I understand your PR - I'm suggesting two ideas to go a step further:

-> If dwarf not present, hide (gray out) the option from the menu
-> if dward present, add a pop-up at the opening or something that let the user know about dwarf. Like you say it's too verbose for now, so this can probably be for later.

@ITAYC0HEN
Copy link
Member

@xarkes do you wish to implement the popup in this PR or never? (sorry, I meant "later" :D )

@XVilka
Copy link
Member

XVilka commented Feb 16, 2021

Other tools also parse DWARF information by default if present. Why wouldn't you - it makes reverse engineering easier.

@XVilka
Copy link
Member

XVilka commented Feb 16, 2021

I think instead of asm.dwarf it is better to do the following:

  • Introduce dwarf.* config options in Rizin (we have ones for pdb.* anyway already)
  • Add dwarf.lines option to show the lines info
  • The rest of the information - types, function names/signatures, etc is more useful
  • PROFIT

@xarkes
Copy link
Member Author

xarkes commented Feb 16, 2021

Exactly, I think in the meantime we can merge or close this PR and open an issue on Rizin.

@ITAYC0HEN
Copy link
Member

Well, meantime you can enable it back by default but keep the preferences check box in there and then let's merge

@karliss
Copy link
Member

karliss commented Feb 16, 2021

I support keeping it on by default. The annoyingly verbose and useless case seems to happen when the file is compiled from assembly file (you seem to be looking at part of go runtime which is indeed written in assembly), as a result no line content because it would be more or less identical with disassembly and line numbers for every instruction because there is almost 1:1 match.

In case of executables or their parts compiled from C or other programming language it would be less dense and show the actual source code making it more useful. I would expect in most cases debug information to be either fully stripped or contain useful information. Is there a common case where it's only partially stripped leaving useless information?

@XVilka Does rizin really need two almost identical sets of options for controlling type of debug information displayed depending of whether it came from pdb or dwarf? Why not asm.debug.lines instead of asm.pdb.lines and asm.dwarf.lines?

@XVilka
Copy link
Member

XVilka commented Feb 18, 2021

@karliss you are right, there is no need for this and one variable like asm.debug.lines should be enough.

@xarkes
Copy link
Member Author

xarkes commented Feb 18, 2021

I prefer to close this PR rather than merging a temporary feature. For reference here is the issue I opened on Rizin: rizinorg/rizin#653

@xarkes xarkes closed this Feb 18, 2021
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.

5 participants