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.

Sign up for GitHub

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

Jump to bottom

Better asm syntax highlighting #538

Merged
merged 4 commits into from
Dec 19, 2023
Merged

Better asm syntax highlighting #538

merged 4 commits into from
Dec 19, 2023

Conversation

lievenhey
Copy link
Contributor

No description provided.

@lievenhey lievenhey force-pushed the better-asm-syntax-highlighting branch 2 times, most recently from e4e6820 to 29eddc8 Compare October 23, 2023 16:33
@lievenhey
Copy link
Contributor Author

left old, right new
grafik

@lievenhey
Copy link
Contributor Author

lievenhey commented Oct 23, 2023 •
edited
Loading

TODO:

  • test
  • hide format chooser if ansi codes are used nearly done, but search is also hidden
  • add option to hide hexdump

@lievenhey
Copy link
Contributor Author

lievenhey commented Oct 23, 2023 •
edited
Loading

And I have to investigate this bug: done
grafik
(text turns blueish if branches are shown)

This was linked to issues Oct 23, 2023
Disassembler performance is bad -> libQt5Gui #505
Closed
Use QTextLayout and KAbstractSyntaxHighlighting to improve performance #522
Closed
@lievenhey lievenhey requested a review from milianw October 23, 2023 16:47
@lievenhey lievenhey force-pushed the better-asm-syntax-highlighting branch 2 times, most recently from 5a909fb to 7e878ff Compare October 23, 2023 19:04
@GitMensch
Copy link
Contributor

I'm a bit confused, why does "Syntax Highlighting: None" have any color? But nice to see that those issues are tackled!

@lievenhey
Copy link
Contributor Author

I'm a bit confused, why does "Syntax Highlighting: None" have any color? But nice to see that those issues are tackled!

someone didn't implement synchronisation yet

@GitMensch
Copy link
Contributor

As this auto-drops the highlighting on the hex values and also changes the code involved here, I highly suggest to implement #527 (moving the hex parts out as requested by Milian) first. Could be done in this PR, too.

@lievenhey lievenhey force-pushed the better-asm-syntax-highlighting branch 8 times, most recently from 81a7e60 to fbfbec9 Compare October 26, 2023 14:25
milianw
milianw requested changes Oct 29, 2023
View reviewed changes
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.

the cleanup/fixup patches should go into a separate MR because they can land directly. the rest of this patch needs some cleanup

src/models/disassemblyoutput.cpp Outdated Show resolved Hide resolved
src/models/disassemblyoutput.cpp Outdated Show resolved Hide resolved
src/models/disassemblyoutput.cpp Outdated Show resolved Hide resolved
src/models/disassemblyoutput.cpp Outdated Show resolved Hide resolved
src/models/disassemblymodel.cpp Outdated Show resolved Hide resolved
src/models/formattingutils.h Outdated Show resolved Hide resolved
src/models/formattingutils.h Show resolved Hide resolved
src/models/highlightedtext.cpp Outdated Show resolved Hide resolved
src/models/highlightedtext.cpp Outdated Show resolved Hide resolved
src/resultsdisassemblypage.cpp Outdated Show resolved Hide resolved
@lievenhey lievenhey mentioned this pull request Oct 30, 2023
Merged
@lievenhey lievenhey force-pushed the better-asm-syntax-highlighting branch 2 times, most recently from b4a785e to 5273ad4 Compare October 30, 2023 13:53
milianw
milianw requested changes Oct 30, 2023
View reviewed changes
tests/modeltests/tst_disassemblyoutput.cpp Outdated Show resolved Hide resolved
src/models/disassemblyoutput.cpp Show resolved Hide resolved
src/models/disassemblyoutput.cpp Show resolved Hide resolved
src/models/disassemblyoutput.cpp Outdated Show resolved Hide resolved
src/models/highlightedtext.cpp Outdated Show resolved Hide resolved
tests/modeltests/tst_formatting.cpp Show resolved Hide resolved
src/models/highlightedtext.cpp Outdated Show resolved Hide resolved
@lievenhey lievenhey force-pushed the better-asm-syntax-highlighting branch 4 times, most recently from 9dd780e to 5b67e29 Compare October 31, 2023 11:01
@lievenhey lievenhey force-pushed the better-asm-syntax-highlighting branch from 8241728 to 565a132 Compare November 30, 2023 13:07
@GitMensch
Copy link
Contributor

GitMensch commented Nov 30, 2023 •
edited
Loading

Just wanted to share that #561 seems to be fixed with the last commit (only checked two different examples that did not work before) - if it should be then please add its reference to the commits / this PR.
Can you add the disassembly output provided in this issue to the testsuite?

#527 is definitely solved by the "add option to hide" commit and should be referenced there.

One downside: it seems that the syntax highlighting in the Disassembly does not work (AppImage).
Note that I do get a message

hotspot.disassemblyoutput: objdump binary does not support --disassembler-color: "/usr/bin/objdump"

@lievenhey
Copy link
Contributor Author

@GitMensch can check you the most recent appimage? I notices that QTextLayout is very slow (multiple seconds for 50k+ lines) and added a cache. You complained about slow performance so maybe this finally fixes it.

@GitMensch
Copy link
Contributor

Checked one extreme example (all with appimages):

  • "old" hotspot: ~110 seconds
  • current master: 48-60 seconds
  • this PR previous push: more seconds
  • this PR after your last commit: 11-12 seconds - but the syntax highlighting in the disassembly is "kind of gone", compare
    master with
    this PR's state

@lievenhey
Copy link
Contributor Author

That is most likely, because KColorScheme::ColorSet::Complementary is not available. I will think of another way to get colors.

@GitMensch
Copy link
Contributor

That is most likely, because KColorScheme::ColorSet::Complementary is not available. I will think of another way to get colors.

... and/or because of the following?

Note that I do get a message

hotspot.disassemblyoutput: objdump binary does not support --disassembler-color: "/usr/bin/objdump"

@lievenhey
Copy link
Contributor Author

Ohh, right, in that case it should fallback to using KSyntaxHighlighter. In that case it should use the original formatting. I will into it.

@lievenhey lievenhey force-pushed the better-asm-syntax-highlighting branch 2 times, most recently from 8963c85 to 23cc2fb Compare December 5, 2023 10:58
@lievenhey
Copy link
Contributor Author

@GitMensch I looked into it. Currently I only copy the font color from KSyntaxHighlighter::Format. If that is what you mean.

@GitMensch
Copy link
Contributor

Would there be any "thickness" it the objdump colored output that may should be copied over?

Note: in any case I'd "expect" to have the objdump coloring as an extra entry of the highlighting (create a manual list including all syntax highlighters that have "asm" in them, the add "objdump" on top, if it is available).
Last time checked there the original syntax highlighter doesn't work any more for disassembly at all.

@lievenhey lievenhey force-pushed the better-asm-syntax-highlighting branch 2 times, most recently from 9ec76f9 to 5445957 Compare December 5, 2023 11:52
@lievenhey lievenhey mentioned this pull request Dec 5, 2023
Merged
@lievenhey lievenhey force-pushed the better-asm-syntax-highlighting branch from 5445957 to 341eed8 Compare December 5, 2023 12:11
@GitMensch
Copy link
Contributor

If it is more likely to get included faster, then we may split this PR in 4:

  • separate hex column - at least according to my manual tests this seems to work fine
  • a later follow-up PR for a possible automated testcase for the above
  • move to QTextLayout including caching (seems to be finished and provide better performance - in this case an early distribution would be very useful) -> possibly in this PR
  • disassembly syntax highlighting with objdump

@milianw
Copy link
Member

milianw commented Dec 10, 2023

code lgtm, probably needs a rebase now but otherwise this should be ready to go in IIUC? @GitMensch is your highlighting bug resolved now in the latest iteration of this pull request?

@GitMensch
Copy link
Contributor

GitMensch is your highlighting bug resolved now in the latest iteration of this pull request?

No, sadly #561 is not yet solved. Just tested with objdump.log, generated by "GNU objdump (GNU Binutils for Debian) 2.35.2", which doesn't support --disassembler-color, with objdump -d -l -C --start-address 0x2ba15 --stop-address 0x2bccf --visualize-jumps /tmp/gnucobol-bug-hunting/libcob/.libs/libcob.so.4.2.0, result
broken disassembly

I guess that the actual highlighting is not the main problem but the splitting of the jump-visualisation and hex-values.

I'm keen to recheck after a rebase (shouldn't make a big difference, I guess) and even more after a fix to this.

Just a debugging note (you're likely aware of this): you may add an "objdump" script that only does a cat on this file, to see the result in your environment.

@lievenhey
Copy link
Contributor Author

Thanks, I finally understand your problem and managed to reproduce it.

@lievenhey
implement syntax highlighting with QTextLayout
1b6b76d 
Using QTextDocument was a little bit overkill since we don't need rich
text support. This replaces it with a simple QTextLayout implementation.

closed: #522, #505
@lievenhey
split canVisualizeJump into two functions
6dec3ba 
this allows for a simpler feature detection in objdump since we can now
fetch the help output once and then query it multiple times for
different arguments
@lievenhey
add ansi highlighter
5835827 
this patch adds a syntax highlighter that decoded ansi escape sequences
and colors the text accordingly
@lievenhey
use cache for layout since QTextLayout::setFormats is slow
953d3e0 
QTextLayout::setFormats is really slow so this patch introduces a cache
named HighlightedLine which will only call QTextLayout::setFormats if
necessary
@lievenhey lievenhey force-pushed the better-asm-syntax-highlighting branch from 341eed8 to 953d3e0 Compare December 12, 2023 11:54
@lievenhey
Copy link
Contributor Author

lievenhey commented Dec 12, 2023 •
edited
Loading

@GitMensch It seems that #566 fixed the issue, can you check again? At least I can't reproduce it anymore after that pr was merged. I checked your files too and they worked.

@GitMensch
Copy link
Contributor

Yes, it is and the current appimage from this PR works fine there, too.
Also tested with a bunch of other recordings. 🥳

As Milian is also fine with this PR it seems its ready for merge :-)

@GitMensch
Copy link
Contributor

@milianw can you merge that in, please?

@milianw milianw merged commit a8d1440 into master Dec 19, 2023
23 checks passed
@milianw milianw deleted the better-asm-syntax-highlighting branch December 19, 2023 10:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GitMensch GitMensch GitMensch requested changes

@milianw milianw milianw approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@lievenhey @GitMensch @milianw