Better asm syntax highlighting by lievenhey · Pull Request #538 · KDAB/hotspot

lievenhey · 2023-10-23T16:31:41Z

No description provided.

lievenhey · 2023-10-23T16:36:55Z

left old, right new

lievenhey · 2023-10-23T16:38:12Z

TODO:

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

lievenhey · 2023-10-23T16:40:31Z

~~And I have to investigate this bug:~~ done

(text turns blueish if branches are shown)

GitMensch · 2023-10-23T19:30:38Z

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

lievenhey · 2023-10-24T08:21:22Z

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 · 2023-10-24T08:34:59Z

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.

milianw

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

src/models/disassemblymodel.cpp

src/models/formattingutils.h

src/models/highlightedtext.cpp

src/resultsdisassemblypage.cpp

tests/modeltests/tst_disassemblyoutput.cpp

src/models/disassemblyoutput.cpp

src/models/highlightedtext.cpp

tests/modeltests/tst_formatting.cpp

src/models/highlightedtext.cpp

GitMensch · 2023-11-30T13:43:55Z

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 · 2023-12-04T15:41:38Z

@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 · 2023-12-04T19:16:13Z

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
with

lievenhey · 2023-12-05T09:51:43Z

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

GitMensch · 2023-12-05T10:03:10Z

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 · 2023-12-05T10:04:33Z

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 · 2023-12-05T11:07:34Z

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

GitMensch · 2023-12-05T11:32:15Z

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.

GitMensch · 2023-12-05T13:34:46Z

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 · 2023-12-10T11:39:02Z

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 · 2023-12-10T21:33:47Z

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

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 · 2023-12-11T12:51:51Z

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

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

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

this patch adds a syntax highlighter that decoded ansi escape sequences and colors the text accordingly

QTextLayout::setFormats is really slow so this patch introduces a cache named HighlightedLine which will only call QTextLayout::setFormats if necessary

lievenhey · 2023-12-12T12:26:38Z

@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 · 2023-12-12T12:47:41Z

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 · 2023-12-19T09:59:47Z

@milianw can you merge that in, please?

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

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 requested a review from milianw October 23, 2023 16:47

lievenhey force-pushed the better-asm-syntax-highlighting branch 2 times, most recently from 5a909fb to 7e878ff Compare October 23, 2023 19:04

lievenhey force-pushed the better-asm-syntax-highlighting branch 8 times, most recently from 81a7e60 to fbfbec9 Compare October 26, 2023 14:25

milianw requested changes Oct 29, 2023

View reviewed changes

lievenhey mentioned this pull request Oct 30, 2023

Small fixups #546

Merged

lievenhey force-pushed the better-asm-syntax-highlighting branch 2 times, most recently from b4a785e to 5273ad4 Compare October 30, 2023 13:53

milianw requested changes Oct 30, 2023

View reviewed changes

lievenhey force-pushed the better-asm-syntax-highlighting branch 4 times, most recently from 9dd780e to 5b67e29 Compare October 31, 2023 11:01

lievenhey force-pushed the better-asm-syntax-highlighting branch from 8241728 to 565a132 Compare November 30, 2023 13:07

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

lievenhey force-pushed the better-asm-syntax-highlighting branch 2 times, most recently from 9ec76f9 to 5445957 Compare December 5, 2023 11:52

lievenhey mentioned this pull request Dec 5, 2023

add option to hide hexdump #566

Merged

lievenhey force-pushed the better-asm-syntax-highlighting branch from 5445957 to 341eed8 Compare December 5, 2023 12:11

lievenhey added 4 commits December 12, 2023 12:54

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

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

add ansi highlighter

5835827

this patch adds a syntax highlighter that decoded ansi escape sequences and colors the text accordingly

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 force-pushed the better-asm-syntax-highlighting branch from 341eed8 to 953d3e0 Compare December 12, 2023 11:54

milianw approved these changes Dec 19, 2023

View reviewed changes

milianw merged commit a8d1440 into master Dec 19, 2023

milianw deleted the better-asm-syntax-highlighting branch December 19, 2023 10:24

Conversation

lievenhey commented Oct 23, 2023

Uh oh!

lievenhey commented Oct 23, 2023

Uh oh!

lievenhey commented Oct 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lievenhey commented Oct 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

GitMensch commented Oct 23, 2023

Uh oh!

lievenhey commented Oct 24, 2023

Uh oh!

GitMensch commented Oct 24, 2023

Uh oh!

milianw left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

GitMensch commented Nov 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lievenhey commented Dec 4, 2023

Uh oh!

GitMensch commented Dec 4, 2023

Uh oh!

lievenhey commented Dec 5, 2023

Uh oh!

GitMensch commented Dec 5, 2023

Uh oh!

lievenhey commented Dec 5, 2023

Uh oh!

lievenhey commented Dec 5, 2023

Uh oh!

GitMensch commented Dec 5, 2023

Uh oh!

GitMensch commented Dec 5, 2023

Uh oh!

milianw commented Dec 10, 2023

Uh oh!

GitMensch commented Dec 10, 2023

Uh oh!

lievenhey commented Dec 11, 2023

Uh oh!

lievenhey commented Dec 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

GitMensch commented Dec 12, 2023

Uh oh!

GitMensch commented Dec 19, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

lievenhey commented Oct 23, 2023 •

edited

Loading

lievenhey commented Oct 23, 2023 •

edited

Loading

GitMensch commented Nov 30, 2023 •

edited

Loading

lievenhey commented Dec 12, 2023 •

edited

Loading