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

improvements for the disassembler #457

Merged
merged 6 commits into from
Apr 26, 2023
Merged

improvements for the disassembler #457

merged 6 commits into from
Apr 26, 2023

Conversation

lievenhey
Copy link
Contributor

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 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
@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
Copy link
Member

@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

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.

@GitMensch
Copy link
Contributor

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

@milianw: Any chance to re-review?

@lievenhey
Copy link
Contributor Author

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

@GitMensch
Copy link
Contributor

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

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.

Copy link
Member

@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
Copy link
Contributor

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

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

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
@GitMensch
Copy link
Contributor

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?

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
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

done

@GitMensch
Copy link
Contributor

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

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

@GitMensch

This comment was marked as outdated.

@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
Member

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

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
Labels
None yet
Projects
None yet
3 participants