Disable branches #518

lievenhey · 2023-10-10T12:50:24Z

No description provided.

src/models/disassemblyoutput.cpp

GitMensch · 2023-10-11T11:13:02Z

LGTM in general.

What do you think about the idea to have the jumps in a separate "column"?
In this case the column would be auto-hidden/disabled if CanVisualize() == false (or the visualization was turned of in the settings) and otherwise the user would be able to show/hide that column at any time.

If you don't want to follow that route, then I think it would be useful to (additionally?) have a toggle to be able to switch this directly on the disassembly page to re-read with visualization and update.

lievenhey · 2023-10-11T11:49:22Z

What do you think about the idea to have the jumps in a separate "column"?

The more I think about it, the better it sounds. In the current version I completely reload the model which will reset the view if I would add a checkbox to toggle this feature. I originally didn't want to go that route since it involves a lot of dark magic (regexes) but it seems to be the better solution in the long run.

GitMensch · 2023-10-11T14:00:10Z

The current resulting AppImage is much faster, instead of 24 seconds it only needs 8-9 seconds for opening.
If I globally disable the jumps in the settings, then it gets down to 5-6 seconds.

Disabling the branch column "on demand" works fine. Note: the visible columns are not saved, if you disable it, then go back to the callee view, then use Disassemble on the same function the column is enabled again; same if the setting is of and the column is manually enabled.
I think the setting should be primarily about "is --visualize-jumps to be used or not" (with the nice check and note like in #520), not about the visibility of that column.

GitMensch · 2023-10-11T14:07:53Z

Just to recheck as it wasn't obvious from inspecting only the changes: is the new branches column "monospace text" only, or is it "syntax-highlighted", too? I think it may also work with non-monospace (then looking better and saving space - but I'm not sure...)

lievenhey · 2023-10-11T14:27:08Z

The current resulting AppImage is much faster, instead of 24 seconds it only needs 8-9 seconds for opening.
If I globally disable the jumps in the settings, then it gets down to 5-6 seconds.

Interesting seem like qt has a problem with layouting if there are a lots of whitespaces

Disabling the branch column "on demand" works fine. Note: the visible columns are not saved, if you disable it, then go back to the callee view, then use Disassemble on the same function the column is enabled again; same if the setting is of and the column is manually enabled.

I see this as an on demand feature for quick stuff. It is only saved, if you change it in the settings.

Just to recheck as it wasn't obvious from inspecting only the changes: is the new branches column "monospace text" only, or is it "syntax-highlighted", too? I think it may also work with non-monospace (then looking better and saving space - but I'm not sure...)

it is monospace only

GitMensch · 2023-10-11T15:01:27Z

Just to recheck as it wasn't obvious from inspecting only the changes: is the new branches column "monospace text" only, or is it "syntax-highlighted", too? I think it may also work with non-monospace (then looking better and saving space - but I'm not sure...)

it is monospace only

Can you give it a short try to see how it looks with the default font? If that looks better (or at least not worse and saves space), then this would be possibly a reasonable switch.

lievenhey · 2023-10-11T15:04:34Z

The current resulting AppImage is much faster, instead of 24 seconds it only needs 8-9 seconds for opening.
If I globally disable the jumps in the settings, then it gets down to 5-6 seconds.

That is way too long. It should only take a few milliseconds. I will ask some KDE People about this, since they wrote their own Layout Engine which they use in Kate. Kate can layout 2 million lines in like 10ms.

lievenhey · 2023-10-11T15:11:49Z

Can you give it a short try to see how it looks with the default font? If that looks better (or at least not worse and saves space), then this would be possibly a reasonable switch.

looks terrible since a space has a different width than a dash

src/models/disassemblyoutput.cpp

src/resultsdisassemblypage.cpp

GitMensch · 2023-10-11T15:52:05Z

The current resulting AppImage is much faster, instead of 24 seconds it only needs 8-9 seconds for opening.
If I globally disable the jumps in the settings, then it gets down to 5-6 seconds.

That is way too long. It should only take a few milliseconds. I will ask some KDE People about this, since they wrote their own Layout Engine which they use in Kate. Kate can layout 2 million lines in like 10ms.

Thank you. Note that these functions are quite huge, so I wouldn't blame it all to the syntax-highlighting / layouting... but then the layouting seems to be an issue, let's go on with this at #505.

src/models/disassemblyoutput.cpp

GitMensch

With the new logic of keeping the branches in a separate column, I think we could simplify the split.

Reasoning (please recheck if that's correct):

we have one big disassembly from objdump
at least currently the start of the assembly is always at the same line position (that would change if we have colored input because of terminal sequences)

In this case we can check the start of the assembly exactly one time on the first line of the assembly, then use that offset for each record.

That would drop the distance-call from each line, calculating it only once. preferably outside.

src/models/disassemblyoutput.cpp

lievenhey · 2023-10-12T13:05:45Z

I will have a look at coloured output soonish, so I would keep it as is. Especially since it is a cheap calculation.

GitMensch · 2023-10-12T13:11:38Z

This is now much better than before, I hope this gets pulled in and afterwards we may get a finished #494 (looks like 98% done) pulled in, too.

Thank you for the work on this!

GitMensch · 2023-10-12T13:13:54Z

I will have a look at coloured output soonish, so I would keep it as is. Especially since it is a cheap calculation.

Note that escape sequences would again match the hex-check :-)
But I totally agree that this can be revisited later.

lievenhey · 2023-10-12T13:16:56Z

This is now much better than before, I hope this gets pulled in and afterwards we may get a finished #494 (looks like 98% done) pulled in, too.

This will still take some time. We want to move the logic to the perfparser since it has all the logic for finding binaries etc.

milianw

I'm fine with this as an incremental step, but I dislike that toggling the visibility of the branch column directly is detached from the settings - can you please synchronize that to make it more obvious?

furthermore, I'm surprised to see that this patch has no test changes. That means this feature is currently completely untested? Can you please work on adding test coverage @lievenhey ?

If you prefer, you can merge this right away and work on both of the above in separate merge requests.

Thanks!

lievenhey · 2023-10-17T12:43:32Z

I will add a test and then merge this

I originaly named these seetings source path settings since they only influenced the source code search path so it didn't feel right to call them disassembly settings. But since this patch series introduces an option to turn of branches it now feels right to correct the name

If only one event was recorded it points to end. In this case end->time will be access which is undefined behavior

Since the introduction of the stack in the disasembler there is no longer a need to call showDisassembly since it will be called whenever the stack changes.

tests/modeltests/tst_disassemblyoutput.cpp

milianw

I'll fix the small nits myself now

tests/modeltests/tst_disassemblyoutput.cpp

sometimes there are too many branches, this patch allows the user to disable them fixes: #516

Skip whole test if it isn't found

Prefer VERIFY_OR_THROW to ensure the checks are also done in release builds without assertions enabled.

lievenhey requested a review from milianw October 10, 2023 12:50

lievenhey linked an issue Oct 10, 2023 that may be closed by this pull request

Disassembly view: allow to disable branches / jumps #516

Closed

GitMensch reviewed Oct 11, 2023

View reviewed changes

src/models/disassemblyoutput.cpp Outdated Show resolved Hide resolved

lievenhey force-pushed the disable-branches branch 4 times, most recently from 41bda57 to b2494e1 Compare October 11, 2023 13:12

lievenhey force-pushed the disable-branches branch from b2494e1 to cb8083e Compare October 11, 2023 14:28

milianw requested changes Oct 11, 2023

View reviewed changes

src/models/disassemblyoutput.cpp Outdated Show resolved Hide resolved

src/resultsdisassemblypage.cpp Show resolved Hide resolved

src/resultsdisassemblypage.cpp Outdated Show resolved Hide resolved

lievenhey force-pushed the disable-branches branch from cb8083e to 356119e Compare October 11, 2023 15:27

lievenhey force-pushed the disable-branches branch 2 times, most recently from 692e5b7 to 1e74256 Compare October 11, 2023 16:14

GitMensch reviewed Oct 11, 2023

View reviewed changes

src/models/disassemblyoutput.cpp Outdated Show resolved Hide resolved

milianw requested changes Oct 12, 2023

View reviewed changes

src/models/disassemblyoutput.cpp Outdated Show resolved Hide resolved

lievenhey force-pushed the disable-branches branch 3 times, most recently from c2a4a84 to e7c1411 Compare October 12, 2023 12:39

GitMensch reviewed Oct 12, 2023

View reviewed changes

src/models/disassemblyoutput.cpp Show resolved Hide resolved

lievenhey force-pushed the disable-branches branch from e7c1411 to 1999318 Compare October 12, 2023 13:03

GitMensch mentioned this pull request Oct 12, 2023

Disassembler inlining #494

Draft

milianw approved these changes Oct 12, 2023

View reviewed changes

lievenhey force-pushed the disable-branches branch 3 times, most recently from a6dd6ba to 5eb7a57 Compare October 19, 2023 12:14

lievenhey added 4 commits October 19, 2023 14:18

fix invalid memory acces in timelinedelegate

d50fe2b

If only one event was recorded it points to end. In this case end->time will be access which is undefined behavior

fix disassemble called twice

39229ae

Since the introduction of the stack in the disasembler there is no longer a need to call showDisassembly since it will be called whenever the stack changes.

remove unused includes

ab06e32

lievenhey force-pushed the disable-branches branch from 5eb7a57 to b68e608 Compare October 19, 2023 12:19

milianw requested changes Oct 19, 2023

View reviewed changes

lievenhey force-pushed the disable-branches branch 3 times, most recently from 99d0f1a to 825d48b Compare October 20, 2023 10:43

milianw requested changes Oct 21, 2023

View reviewed changes

tests/modeltests/tst_disassemblyoutput.cpp Outdated Show resolved Hide resolved

tests/modeltests/tst_disassemblyoutput.cpp Outdated Show resolved Hide resolved

lievenhey and others added 3 commits October 21, 2023 14:05

add setting to disable branches in disassembler

c682645

sometimes there are too many branches, this patch allows the user to disable them fixes: #516

Centralize finding objdump binary

0adb8bf

Skip whole test if it isn't found

Don't use Q_ASSERT in tests

78f0642

Prefer VERIFY_OR_THROW to ensure the checks are also done in release builds without assertions enabled.

milianw force-pushed the disable-branches branch from 825d48b to 78f0642 Compare October 21, 2023 12:10

milianw approved these changes Oct 21, 2023

View reviewed changes

milianw merged commit deb59a5 into master Oct 21, 2023
23 checks passed

milianw deleted the disable-branches branch October 21, 2023 12:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disable branches #518

Disable branches #518

lievenhey commented Oct 10, 2023

GitMensch commented Oct 11, 2023

lievenhey commented Oct 11, 2023

GitMensch commented Oct 11, 2023

GitMensch commented Oct 11, 2023

lievenhey commented Oct 11, 2023

GitMensch commented Oct 11, 2023

lievenhey commented Oct 11, 2023

lievenhey commented Oct 11, 2023

GitMensch commented Oct 11, 2023

GitMensch left a comment

lievenhey commented Oct 12, 2023

GitMensch commented Oct 12, 2023

GitMensch commented Oct 12, 2023

lievenhey commented Oct 12, 2023

milianw left a comment

lievenhey commented Oct 17, 2023

milianw left a comment

Disable branches #518

Disable branches #518

Conversation

lievenhey commented Oct 10, 2023

GitMensch commented Oct 11, 2023

lievenhey commented Oct 11, 2023

GitMensch commented Oct 11, 2023

GitMensch commented Oct 11, 2023

lievenhey commented Oct 11, 2023

GitMensch commented Oct 11, 2023

lievenhey commented Oct 11, 2023

lievenhey commented Oct 11, 2023

GitMensch commented Oct 11, 2023

GitMensch left a comment

Choose a reason for hiding this comment

lievenhey commented Oct 12, 2023

GitMensch commented Oct 12, 2023

GitMensch commented Oct 12, 2023

lievenhey commented Oct 12, 2023

milianw left a comment

Choose a reason for hiding this comment

lievenhey commented Oct 17, 2023

milianw left a comment

Choose a reason for hiding this comment