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

Better disassembly #390

Merged
merged 4 commits into from
Sep 2, 2022
Merged

Better disassembly #390

merged 4 commits into from
Sep 2, 2022

Conversation

lievenhey
Copy link
Contributor

@lievenhey lievenhey commented May 31, 2022

A rewrite of the disassembly view
closes #9

@lievenhey
Copy link
Contributor Author

grafik

@GitMensch
Copy link
Contributor

This is marvelous! I'd really like to see that integrated.
[side note: is there still a CI building AppImage anywhere? old travis is gone and the last CI release is nearly 7 months old]

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 share code between the two models, maybe even extract the whole highlighting and color scheme into a shared single class and then reuse that instead of duplicating it.

src/models/codedelegate.cpp Outdated Show resolved Hide resolved
src/models/codedelegate.cpp Outdated Show resolved Hide resolved
src/models/codedelegate.cpp Outdated Show resolved Hide resolved
src/models/disassemblymodel.cpp Outdated Show resolved Hide resolved
src/models/disassemblymodel.h Outdated Show resolved Hide resolved
src/models/disassemblyoutput.h Outdated Show resolved Hide resolved
src/models/sourcecodemodel.cpp Show resolved Hide resolved
src/models/sourcecodemodel.cpp Outdated Show resolved Hide resolved
src/models/sourcecodemodel.cpp Outdated Show resolved Hide resolved
src/models/sourcecodemodel.h Outdated Show resolved Hide resolved
@lievenhey lievenhey force-pushed the better-disassembly branch 3 times, most recently from b7f7137 to 7c7a861 Compare June 23, 2022 10:58
@lievenhey
Copy link
Contributor Author

closes #9

@GitMensch
Copy link
Contributor

closes #9

Note: for GitHub to recognize this please edit the first post and add the information there

@lievenhey lievenhey force-pushed the better-disassembly branch 2 times, most recently from 7d426d8 to 2b6edde Compare June 23, 2022 12:19
src/models/codedelegate.cpp Outdated Show resolved Hide resolved
src/models/disassemblymodel.cpp Outdated Show resolved Hide resolved
src/models/disassemblymodel.cpp Outdated Show resolved Hide resolved
src/models/disassemblymodel.cpp Outdated Show resolved Hide resolved
src/models/disassemblyoutput.cpp Show resolved Hide resolved
src/models/highlighter.hpp Show resolved Hide resolved
src/models/highlighter.cpp Show resolved Hide resolved
src/models/sourcecodemodel.cpp Outdated Show resolved Hide resolved
@lievenhey lievenhey force-pushed the better-disassembly branch 7 times, most recently from 45e078e to 4c406f3 Compare June 29, 2022 11:30
src/models/sourcecodemodel.cpp Outdated Show resolved Hide resolved
src/models/sourcecodemodel.cpp Outdated Show resolved Hide resolved
src/models/sourcecodemodel.cpp Outdated Show resolved Hide resolved
src/models/sourcecodemodel.cpp Outdated Show resolved Hide resolved
src/models/sourcecodemodel.cpp Outdated Show resolved Hide resolved
tests/modeltests/tst_models.cpp Show resolved Hide resolved
CMakeLists.txt Show resolved Hide resolved
src/models/sourcecodemodel.h Outdated Show resolved Hide resolved
tests/modeltests/tst_models.cpp Show resolved Hide resolved
@lievenhey lievenhey force-pushed the better-disassembly branch 10 times, most recently from 99d0c44 to b7536d4 Compare June 30, 2022 11:08
@milianw
Copy link
Member

milianw commented Aug 28, 2022

@lievenhey moin, this is looking good and we could merge it as-is as it's an improvement.

But we definitely need to handle inlining better, see 72fec27 for some easy work from towards that direction. Could you look into improving this further based on the cpp-inlining example? Things that I'd like to see:

  • source listings for all files encountered in a function in a combined SourceCodeModel tree model, the file name on the first level and then second level shows source code lines like it does currently
  • combined source line costs when code gets inlined from somewhere, this screenshot should hopefully make this clear:

Screenshot_20220828_130753

note how the code is basically all within inlined code, so currently we show 0% cost everywhere which is super misleading

lievenhey and others added 4 commits August 30, 2022 09:55
this patch writes the source code in a QTextDocument and attaches a
KSyntaxHighlighter to it
Then it exposes the QTextLine to a custom delegate to render the colored
text

tst_models now requires a QCoreApplication, since QTextDocument wants to
read font files
When we use disassembly on the frame for
`std::__detail::_Mod<...>::__calc(...)` in `cpp-inlining` then
we would get `cpp-inlining/main.cpp` as main source file, but also
quite a few disassembly lines from other files that got inlined from
the libstdc++ library. These then polluted the disassembly view,
as it only showed the source lines from `main.cpp` and definitely
doesn't have 3k lines of code.

Eventually, it would be great if we could show all encountered
files, in a kind of tree view instead of just skipping these source
lines.
@milianw milianw merged commit f578fc2 into master Sep 2, 2022
@milianw milianw deleted the better-disassembly branch September 2, 2022 09:53
@GitMensch
Copy link
Contributor

This is really cool! The CI builds don't provide an AppImage, do they? 'd love to test the new annotation and disassembly.

@milianw
Copy link
Member

milianw commented Sep 2, 2022

Not yet, no - I'll build one manually next week (and do a proper new release too 🤞)

But note that there's still some work left to do in the disassembly wrt inline frames

@GitMensch
Copy link
Contributor

Just rechecking @milianw, does this mean we can expect Hotspot 1.4.0 "soonish"?

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.

annotate source
3 participants