Skip to content

improvements for the disassembler#457

Merged
milianw merged 6 commits intomasterfrom
objdump-searchpath
Apr 26, 2023
Merged

improvements for the disassembler#457
milianw merged 6 commits intomasterfrom
objdump-searchpath

Conversation

@lievenhey
Copy link
Contributor

@lievenhey lievenhey commented Jan 5, 2023

Now the disassembler looks at symbol.actualPath and checks if that file exists (usualy located in .debug)
If the file does not exists it searches in debugPaths and then in extraLibPaths
If none is found it tries symbol.path (original lib file which may not include debug informations)

fixes #450

@lievenhey lievenhey requested a review from milianw January 5, 2023 15:46
@lievenhey lievenhey changed the title add debugPaths and extraLibPaths to search paths for disassembler improvements for the disassembler Jan 6, 2023
@lievenhey lievenhey force-pushed the objdump-searchpath branch 2 times, most recently from 531cdda to 27cf30a Compare January 6, 2023 13:58
@lievenhey lievenhey linked an issue Jan 6, 2023 that may be closed by this pull request
Disassembly window: change double-click to click as there is a double-use of that event #437
Closed
@lievenhey lievenhey force-pushed the objdump-searchpath branch 2 times, most recently from a478cb4 to d8e5806 Compare January 6, 2023 14:04
@lievenhey lievenhey linked an issue Jan 6, 2023 that may be closed by this pull request
Disassembly window: add "find" to source code view #425
Closed
@lievenhey lievenhey force-pushed the objdump-searchpath branch 2 times, most recently from 123394a to 46244df Compare January 6, 2023 15:51
@lievenhey lievenhey linked an issue Jan 6, 2023 that may be closed by this pull request
FR Disassembly View: include context menu to columns #433
Closed
GitMensch
GitMensch reviewed Jan 9, 2023
View reviewed changes
milianw
milianw requested changes Jan 10, 2023
View reviewed changes
Copy link
Collaborator

@milianw milianw left a comment

Choose a reason for hiding this comment

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

great stuff, but overall some changes are required

@GitMensch
Copy link
Contributor

GitMensch commented Jan 26, 2023

Note: If possible it would be nice to have the Dissassembly Window be split, so the sizes of "source" vs. "assembly" can be adjusted. The later often has < 60 characters (at least with C, is that different with C++?), while the code is much wider. As it is now there is often a lot of empty space on the right side while the left has the code elided - and the user seems to not able to adjust these sizes.

@GitMensch

This comment was marked as resolved.

Sign in to view
@GitMensch
Copy link
Contributor

GitMensch commented Jan 28, 2023

Nevermind, I've forgot the chmod +x and used an old version. The sliding with the new version works perfectly!
For the search there seems to be something missing in the UI (at least the menu, possibly?) for the search; I think a magnifying glass in the bottom bar which contains the syntax highlighting selection would be ideal.
Using [CTRL]+[F], then up/down buttons work fine (a "case-sensitive" check mark would be nice, though).

@GitMensch
Copy link
Contributor

GitMensch commented Feb 23, 2023

@milianw: Any chance to re-review?

@lievenhey
Copy link
Contributor Author

lievenhey commented Feb 28, 2023

there is now a magnifying glass and a checkbox for a case sensitive search

@GitMensch
Copy link
Contributor

GitMensch commented Feb 28, 2023

The adjusted search works good. The only missing thing seems to be some visual feedback, maybe just select the line with the result?

GitMensch
GitMensch reviewed Feb 28, 2023
View reviewed changes
Copy link
Contributor

@GitMensch GitMensch left a comment

Choose a reason for hiding this comment

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

Other than the "visual feedback" on the search and the minor README notes, I see no open points from a user-perspective.

milianw
milianw requested changes Mar 6, 2023
View reviewed changes
Copy link
Collaborator

@milianw milianw left a comment

Choose a reason for hiding this comment

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

please write better commit messages

@GitMensch GitMensch mentioned this pull request Mar 16, 2023
Merged
@GitMensch
Copy link
Contributor

GitMensch commented Mar 24, 2023

Just wondering: would it be more useful to split this PR into multiple parts? It solves multiple issues an it may be faster (or only more work, not sure) to get some implemented/fixed if the number of changes that need to be reviewed are smaller?

@lievenhey lievenhey force-pushed the objdump-searchpath branch from 78b4fc7 to 9ed3227 Compare April 14, 2023 12:47
@lievenhey lievenhey force-pushed the objdump-searchpath branch 3 times, most recently from 9ed3227 to 695a871 Compare April 14, 2023 12:49
@lievenhey
Copy link
Contributor Author

lievenhey commented Apr 14, 2023

I removed the search stuff. This should now only contain reviewed code.
I will add the search again in a new PR.

@GitMensch
Copy link
Contributor

GitMensch commented Apr 14, 2023

Thank you very much!

@lievenhey lievenhey force-pushed the objdump-searchpath branch 2 times, most recently from 40a94ec to 4256b03 Compare April 18, 2023 06:47
@lievenhey lievenhey removed a link to an issue Apr 18, 2023
Disassembly window: add "find" to source code view #425
Closed
@GitMensch
Copy link
Contributor

GitMensch commented Apr 19, 2023

If it isn't possible to re-review the reviewed code / merge that into master - could you please rebase the branch so the resulting appimage has both?

@lievenhey
add debugPaths and extraLibPaths to search paths for disassembler
6684819 
Now the disassembler looks at symbol.actualPath and checks if that file
exists (usualy located in .debug)
If the file does not exists it searches in debugPaths and then in
extraLibPaths
If none is found it tries symbol.path (original lib file which may not
include debug informations)

fixes #450
@lievenhey
add costheader to disassembly views
b070ca1 
closes #433
@lievenhey
add simple navigation to the disassembler
1f8a77b 
closes #437

changes
@lievenhey
update readme with disassembler features
e0b827b
@lievenhey
remove unused function declaration
4c59bcf
@lievenhey
allow resizing of sourcecode / disassembly view
bfc620e 
Add a QSplitter to allow the user to customize the size of the views.
@lievenhey lievenhey force-pushed the objdump-searchpath branch from 4256b03 to bfc620e Compare April 20, 2023 12:31
@lievenhey
Copy link
Contributor Author

lievenhey commented Apr 20, 2023

done

@GitMensch
Copy link
Contributor

GitMensch commented Apr 21, 2023

Thank you, the resulting AppImage works fine.

Would it be reasonable to tackle #418 in this PR, too? That issue is about adding settings and use them in the disassembler view.

@lievenhey
Copy link
Contributor Author

lievenhey commented Apr 24, 2023

6684819a should already fix that. That commit allows the disassembler to search --debugPaths and --extraLibPaths.

@GitMensch

This comment was marked as outdated.

Sign in to view
@milianw milianw merged commit 03b3dec into master Apr 26, 2023
@milianw milianw deleted the objdump-searchpath branch April 26, 2023 19:29
@milianw
Copy link
Collaborator

milianw commented Apr 26, 2023

Thank you for your work @lievenhey as always, and sorry for taking so long with this one!

@GitMensch
Copy link
Contributor

GitMensch commented Apr 26, 2023 •
edited
Loading

Would it be reasonable to tackle #418 in this PR, too? That issue is about adding settings and use them in the disassembler view.

6684819a should already fix that. That commit allows the disassembler to search --debugPaths and --extraLibPaths.

That seems to handle only binaries, from -help:

--extraLibPaths Colon separated list of extra paths to find libraries.

but in any case that's something for another PR now :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@milianw milianw milianw requested changes

+1 more reviewer

@GitMensch GitMensch GitMensch left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

3 participants

@lievenhey @GitMensch @milianw