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

Qt/Debugger: Implement "Code" widget #6386

Merged
merged 1 commit into from
Mar 15, 2018

Conversation

spycrab
Copy link
Contributor

@spycrab spycrab commented Feb 18, 2018

This needs thorough reviewing.

@spycrab
Copy link
Contributor Author

spycrab commented Feb 18, 2018

@aldelaro5 @MayImilae @JMC47

@spycrab spycrab force-pushed the qt_dbg_code branch 2 times, most recently from 962bc50 to 03f7743 Compare February 18, 2018 22:20
@aldelaro5
Copy link
Member

Ok, I pretty much tested everything I could as far as the UI/UX is concerned (haven't really checked the code though, not that good with Qt yet to review the code)

Here's what I got:

  • The step hotkeys don't work or aren't editable, I can't edit them in the hotkeys configuration page (there should be a tab for it)
  • The green PC highlight color is too bright and makes reading very hard when the font is white which it is on dark gtk theme. The colors were hardcoded in the wx one so it was always black on light green, but now it's white on bright green whcih is pretty much impossible to read
  • When a step is performed on a branch, the view isn't recentered on the new PC, it is when I follow branch though.
  • You can use the follow branch option anywhere, results may vary depending if it is actually a branch or not, it should be grayed out if it isn't.
  • You cannot resize the code view horixontally to take space over the 4 symbols panels, the reason I would want to do this is because the horizontal space is limited and I usually don't need much space for these 4 panels much. This is especially a problem as I don't have dolphin maximised when I use the debugger and I use the horizontal space for other stuff. I suggest doing like wx which is adding a splitter between the 4 panels and the code view
  • There is no indication about the current code line unless you click on it. This is confusing because say you follow branch or scroll, nothing tells me what is the current line, I basically have to guess according to the center. This is also annoying when you click set PC because you don't see the current line. It's kind of like a "clickless" selection if you want and the wx UI put a little frame around the line to indicate it.
  • It's impossible to add breakpoints by clicking on the left rim of the view, this is very important because it avoids typing the address manually and it is usually how I add instructionn breakpoints. It's much more convenient then adding them via the breakpoints panel.
  • When you add a breakpoint, it's shown in the code view as the entire line being highlighted in bright red. This is a problem because first, it's odd to have such a strong highlight and second, if it breaks, the PC becomes a breakpoint line so you get a line with 2 hgihlight and you just see red, there's no indication the current line is a breakpoint AND the PC, I suggest just putting a circle like in wx on the left rim.
  • The pause/play button in the toolbar doesn't update when the code breaks, it appears that the emulator is playing while it actually is paused and broke,
  • Maybe not too related to this PR, but you can edit the cells in the breakpoint panel when they should be read only.
  • When clicking, the line highlights as it is selected, but I don't see the point since a line could be the current line, be "selected" while no click has been done for example by scrolling. In the WX UI, when clickling, it just recenter and keep the frame I talked about earlier so I see the line is selected, but I don't see a full highlight on it, It's as if there was a hidden selection and a "true" selection which is kinda confusing.
  • When I click on a symbol on one of the 4 panels on the left, the only indicator that I actually clicked on it is just a very quick flash of the text, no highlight is being done making it not so obvious that I selected something.
  • I have no idea why you can select multiple lines in the code view by holding the click and drag the mouse, no options uses that.
  • In the search address bar, if you enter something valid and backspace everything, the text becomes red, but there's just nothing in it.
  • There's no colors between blocks when the symbols are loaded, they are essential because it allows to visually distinguish very quickly different near functions in a much better way than just the name.
  • There's some blank spaces at the bottom of the code view
  • MUCH better auto resizing, I like this much better that the symbol name gets automatically the closest to the end of the instruction line, makes reading a lot better and I end up saving a lot of horizontal space which is limited in the first place.
  • Seems like the function callers panel doesn't work, I haven't seen any symbols displayed there in my tests.
  • Inability to increase the font size of the debugger which you could in WX.

That's all I have to say. Overall, this is shaping to be MUCH better than the WX one because of all the extra space saved and the removal of several hardcoded visuals. By the way, about the branch lines, feel free to do whatever you want with them because they weren't that good in the wx one, they just disappeared if they were off screen.

@spycrab
Copy link
Contributor Author

spycrab commented Feb 19, 2018

The green PC highlight color is too bright and makes reading very hard when the font is white which it is on dark gtk theme. The colors were hardcoded in the wx one so it was always black on light green, but now it's white on bright green whcih is pretty much impossible to read

Should be more readable all around now.

You cannot resize the code view horixontally to take space over the 4 symbols panels, the reason I would want to do this is because the horizontal space is limited and I usually don't need much space for these 4 panels much. This is especially a problem as I don't have dolphin maximised when I use the debugger and I use the horizontal space for other stuff. I suggest doing like wx which is adding a splitter between the 4 panels and the code view

You can now! (It will also remember it)

There is no indication about the current code line unless you click on it. This is confusing because say you follow branch or scroll, nothing tells me what is the current line, I basically have to guess according to the center. This is also annoying when you click set PC because you don't see the current line. It's kind of like a "clickless" selection if you want and the wx UI put a little frame around the line to indicate it.

Should be clearer now.

It's impossible to add breakpoints by clicking on the left rim of the view, this is very important because it avoids typing the address manually and it is usually how I add instructionn breakpoints. It's much more convenient then adding them via the breakpoints panel.

Possible now

When you add a breakpoint, it's shown in the code view as the entire line being highlighted in bright red. This is a problem because first, it's odd to have such a strong highlight and second, if it breaks, the PC becomes a breakpoint line so you get a line with 2 hgihlight and you just see red, there's no indication the current line is a breakpoint AND the PC, I suggest just putting a circle like in wx on the left rim.

Fixed

The pause/play button in the toolbar doesn't update when the code breaks, it appears that the emulator is playing while it actually is paused and broke,

Might be fixed now.

When clicking, the line highlights as it is selected, but I don't see the point since a line could be the current line, be "selected" while no click has been done for example by scrolling. In the WX UI, when clickling, it just recenter and keep the frame I talked about earlier so I see the line is selected, but I don't see a full highlight on it, It's as if there was a hidden selection and a "true" selection which is kinda confusing.
When I click on a symbol on one of the 4 panels on the left, the only indicator that I actually clicked on it is just a very quick flash of the text, no highlight is being done making it not so obvious that I selected something.

It's more similar to Wx now.

In the search address bar, if you enter something valid and backspace everything, the text becomes red, but there's just nothing in it.

Fixed.

There's no colors between blocks when the symbols are loaded, they are essential because it allows to visually distinguish very quickly different near functions in a much better way than just the name.
There's some blank spaces at the bottom of the code view

There are colors now.

When a step is performed on a branch, the view isn't recentered on the new PC, it is when I follow branch though.

You can use the follow branch option anywhere, results may vary depending if it is actually a branch or not, it should be grayed out if it isn't.

Seems like the function callers panel doesn't work, I haven't seen any symbols displayed there in my tests.

Fixed.

Inability to increase the font size of the debugger which you could in WX.

You can now!

Edit:

The step hotkeys don't work or aren't editable, I can't edit them in the hotkeys configuration page (there should be a tab for it)

Editable and usable now!

@spycrab spycrab force-pushed the qt_dbg_code branch 4 times, most recently from de7053c to 718683b Compare February 19, 2018 22:24
@aldelaro5
Copy link
Member

aldelaro5 commented Feb 19, 2018

I tested some more and found more issues, but first:

The pause/play button in the toolbar doesn't update when the code breaks, it appears that the emulator is playing while it actually is paused and broke,

Might be fixed now.

Unfortunately, it is only partially. If I put a breakpoint near the PC enough, it's fine. However, if the breakpoint is very far form the PC, enough for the game to runs for a couple of seconds/1-2 minutes, nothing updates, the pause/play button AND the code view, but you can trigger an update manually by scrolling in the view.

You can use the follow branch option anywhere, results may vary depending if it is actually a branch or not, it should be grayed out if it isn't.

Fixed.

Well now you cannot use the option anywhere, it's grayed, even when actually on a branch :(

Ok so here's the new stuff:

  • Holding the click and dragging does weird things (it still triggers the event continuously, it should only trigger once). This is true for the code view, the left rim of the code view and all the symbols panels.
  • Symbols list are not sorted alphabetically which is important because the dummy generated symbols are pushed to the buttom, this is why they are named this way.
  • Scrolling should move by more, the wx used 3 and I realised that only doing it by 1 makes it super annoying especially when analysing long functions with hundreds of lines.
  • bl'ed function are not displayed instead of symbols name. Very important because it allows to see a bl call and its symbol so if I was working with generated symbols, I rename one and later on I see it gets called, I want to quickly see that this symbol that I renamed is called so I won't have to check inside that function as I already. figured it out. Brnached function are prexifed with --> to symbolise that. Same goes for regular branches, the symbol name is still the function itself because it branches inside that function, but it's prefixed with --> .
  • Resizing the game area doesn't really update the rendering area of the code view so if I wanted to give it more space, I would have to resize and then pause + play which is sometime impossible (cause the asm would have to be read right now)
  • Resizing the window completely screws up the code view size. Like it's not well visible while the wx one resized dynamically.
  • There's a minimal size cap on the game size, should not be there because if I am analysing a function, I really do not care for the game rendering area so I am fine that it takes a minimal horixontal space,
  • No coloring on the branches. The wx one they put a magenta like colors on the addresses of the branches and blr had a slightly darker color. This was done because these instructions are special: they are function calls and returns. As I tested this UI, I realised I missed them a lot because it's a really nice visual indficator. Same goes for regular branches btw which can go somewhere else in the code.
  • The size of the instruction column needs at least one more to accomodate rlwinm. and paired singles instructions. rlwinm. is an rlwinm, but it does an implicit compare with 0 with the result and it takes more character than what was anticipated because I see a weird spacing on these kinds of columns. However, after a quick search I found that paired singles instructions goes even further beyond that limit, like this one: http://wiibrew.org/wiki/Paired_single#ps_merge00 so the size of that coulumn should be able to accomodate as much characters as these paired singles ones.
  • You can highligh and select a column by clicking on the boundaries of them. It shows a blue highlight color and the area to click seems pretty precise, but it's generally close to the boundaries of the cells.
  • When you right click on a line, it recenters the view, but it really shouldn't do that, it should just display the options for that line,

That's all I have for now.

@spycrab
Copy link
Contributor Author

spycrab commented Feb 19, 2018

Can't comment on your first two points until tomorrow, sorry.

  1. Why would you want to click and drag anyway? To scroll? You can either do that using the mouse wheel, your direction keys or page up down.

4., 5., 6., 10., 13. I understand those and will fix them tomorrow

  1. ,8., 11. ,12. I'd need screenshots to better understand those

  2. Unrelated but I might as well try to fix that while I'm at it.

@aldelaro5
Copy link
Member

  1. Why would you want to click and drag anyway? To scroll? You can either do that using the mouse wheel, your direction keys or page up down.

What was wrong with click and drag was the fact it's just really weird as it triggers events much more than once. (like it constantly scrolls one way, add remove breakpoints at sporadious interval or select constantly a symbol).

But to answer the question, I would do that on WX if I need to recenter the view to a precise instruction that happens to be near the current center. What the WX one does is if you click, but not release is it puts a thinner frame on the line you would click and if you drag, it would drag the frame too, it follows the line the cursor is on. Only when you release the click is when it will recenter the view to that line so it became a bit usefull if I wanted to recenter the view to that line only. It is obviously not for scrolling. To be honest, this is mostly a small convenient feature that I use from time to time as I realised it can be nice to have, but not essential, you could just click the right line the first time to do the same thing.

  1. here's a typical scenario, I launch the game, pause it and the code view doesn't have enough horizontal space like so:

screenshot from 2018-02-19 18-37-24

However, this happens if I resize the code view to have more space so I can read things:

screenshot from 2018-02-19 18-38-53

On WX, this would always show the extra space and I guess it "resize" dynamically, but thinking about it, I am not even sure it resizes, it might just render the view and have only a part visible and you would have to resize to see more of it.

  1. From this maximised shot:

screenshot from 2018-02-19 18-42-55

everything's fine, but if I put map the window to the right and expand it a bit (so the window now has less horizontal space):

screenshot from 2018-02-19 18-43-16

On wx, maybe the game view might be weird (such as showing nothing), but the code view should be perfectly readable and resized accordingly.

  1. Here's a rlwinm. line along with other random lines near:

screenshot from 2018-02-19 18-47-26

You can see the odd spacing there, I assume it has to do with the column's width because it seems like non dotted rlwinm snuggly fit. If it is, then a ps_merge00 would be worse.

  1. It's a bit hard to show WHERE exactly to click, but if I click close to the boundaries of the instruction column, this happens:

screenshot from 2018-02-19 18-50-14

It's kind of tricky to trigger that, but I guess clickling a bunch at precise place, eventually it will show up.

@spycrab
Copy link
Contributor Author

spycrab commented Feb 20, 2018

Tried to address everything except for:

  1. There's a minimal size cap on the game size, should not be there because if I am analysing a function, I really do not care for the game rendering area so I am fine that it takes a minimal horixontal space,

Which I couldn't figure out, so I'll try to fix that later. For the time being you could use a external render window or undock the code widget.

  1. Well now you cannot use the option anywhere, it's grayed, even when actually on a branch :(

It doesn't currently restrict it to branch instructions only. Could you provide me with a list of all branch instructions where it should be available?

@spycrab spycrab force-pushed the qt_dbg_code branch 2 times, most recently from 9db4aeb to 37eb336 Compare February 20, 2018 09:17
@aldelaro5
Copy link
Member

Well now you cannot use the option anywhere, it's grayed, even when actually on a branch :(

It doesn't currently restrict it to branch instructions only. Could you provide me with a list of all branch instructions where it should be available?

Well, there's a lot, what I realised is this is what the WX implementation does (so it enables the options if this returns something other than 0)

https://github.com/dolphin-emu/dolphin/blob/master/Source/Core/DolphinWX/Debugger/CodeView.cpp#L190

But if you would like to do it differently, I wouldn't necessarly try to do it by list because there's a lot of them (branch, branch + link, branch under some conditions, branch to some special things, branch to lr, etc...._). What I would try to do is check the instruction in hex and check the bits that tells this is a branch instruction.

...

I think the former solution might be simpler, only branches instructions have that -> notation anyway. I guess I can check how it determines whether to put it or not, but if it's already determined by the string, it would be a bit weird to check this in this convoluted way.

if (symbol)
{
std::string text;
text = text + symbol->name + "\r\n";

This comment was marked as off-topic.


void CodeViewWidget::SetAddress(u32 address)
{
if (m_address != address)

This comment was marked as off-topic.

void CodeWidget::Step()
{
Common::Event sync_event;
if (CPU::IsStepping())

This comment was marked as off-topic.


void CodeWidget::StepOver()
{
if (CPU::IsStepping())

This comment was marked as off-topic.


void CodeWidget::StepOut()
{
if (CPU::IsStepping())

This comment was marked as off-topic.

bool good;
unsigned long address = text.toUInt(&good, 16);

if (good && address <= std::numeric_limits<u32>::max())

This comment was marked as off-topic.

QString text = QInputDialog::getText(
this, tr("Input"), tr("Only export symbols with prefix:\n(Blank for all symbols)"));

if (!text.isEmpty())

This comment was marked as off-topic.

@@ -6,6 +6,7 @@

#include <memory>

#include <QFont>

This comment was marked as off-topic.

const u32 address = m_context_item->data(Qt::UserRole).toUInt();

Symbol* symbol = g_symbolDB.GetSymbolFromAddr(address);
if (symbol)

This comment was marked as off-topic.

AddWidget(tr("Wii and Wii Remote"), new HotkeyWii(this));
AddWidget(tr("Graphics"), new HotkeyGraphics(this));
AddWidget(tr("3D"), new Hotkey3D(this));
AddWidget(tr("Save and Load State"), new HotkeyStates(this));
setWindowTitle(tr("Hotkey Settings"));

This comment was marked as off-topic.

@spycrab spycrab force-pushed the qt_dbg_code branch 2 times, most recently from c18aefe to 8e64a0d Compare February 21, 2018 12:23
@spycrab
Copy link
Contributor Author

spycrab commented Feb 21, 2018

@aldelaro5 Should work properly now!

@aldelaro5
Copy link
Member

@aldelaro5 Should work properly now!

Actually, it works, but not exactly how it should. Currently, it allows to branch and it will branch if you use the option, but only if the current selected line is a branch. What should be happening is when you right click on a line that is a branch, it should allow you to follow the branch, even if the line isn't selected and isn't the center. It would be too annoying to have to recenter before branching so that would be why you should be able to do it that way.

@spycrab spycrab force-pushed the qt_dbg_code branch 2 times, most recently from edeb784 to 9e371bb Compare February 21, 2018 17:25
@spycrab
Copy link
Contributor Author

spycrab commented Feb 21, 2018

@aldelaro5 Fixed. Also Fixed the color stuff you mentioned.

@spycrab
Copy link
Contributor Author

spycrab commented Feb 22, 2018

@aldelaro5 got anything else?

@aldelaro5
Copy link
Member

@spycrab only thing I noticed is the magenta color. I think I wasn't explicit enough when I said it, but I meant the instructions PARAMETERS column, not the instruction column.

Basically, this:

screenshot from 2018-02-22 14-33-30

Other than that, so far, not really.

@spycrab
Copy link
Contributor Author

spycrab commented Feb 22, 2018

@aldelaro5 Actually forgot to commit that, my bad.

Take your time and if you don't find anything else approve the PR so I know you're okay with it.

Thanks for your feedback.

@aldelaro5
Copy link
Member

Unfortunately, this is not ready for merge, new issues were introduced as well as existing ones seems to not have been addressed yet.

  • None of the stepping commands work, this is odd, they used to, but right now, it's as if it was resuming the emulation then pausing it. Further inspection in the code shows that it is likely caused by the fact the state is set to running then paused (I commented the lines, it worked properly afterwards).
  • Left clicking on a line should recenter the view, currently, it just selects the line.

Here are the issues yet to be addressed:

  • You can highligh and select a column by clicking on the boundaries of them. It shows a blue highlight color and the area to click seems pretty precise, but it's generally close to the boundaries of the cells.
  • Resizing the game area doesn't really update the rendering area of the code view so if I wanted to give it more space, I would have to resize and then pause + play which is sometime impossible (cause the asm would have to be read right now)
  • bl'ed function are not displayed instead of symbols name. Very important because it allows to see a bl call and its symbol so if I was working with generated symbols, I rename one and later on I see it gets called, I want to quickly see that this symbol that I renamed is called so I won't have to check inside that function as I already. figured it out. Brnached function are prexifed with --> to symbolise that. Same goes for regular branches, the symbol name is still the function itself because it branches inside that function, but it's prefixed with --> .
  • Scrolling should move by more, the wx used 3 and I realised that only doing it by 1 makes it super annoying especially when analysing long functions with hundreds of lines.
  • I have no idea why you can select multiple lines in the code view by holding the click and drag the mouse, no options uses that.

This one is partially addressed (the symbols panel still has that problem):

  • Holding the click and dragging does weird things (it still triggers the event continuously, it should only trigger once). This is true for the code view, the left rim of the code view and all the symbols panels.

@spycrab
Copy link
Contributor Author

spycrab commented Feb 25, 2018

Left clicking on a line should recenter the view, currently, it just selects the line.

As discussed in IRC.

None of the stepping commands work, this is odd, they used to, but right now, it's as if it was resuming the emulation then pausing it. Further inspection in the code shows that it is likely caused by the fact the state is set to running then paused (I commented the lines, it worked properly afterwards).

Fixed (AFAICT).

Scrolling should move by more, the wx used 3 and I realised that only doing it by 1 makes it super annoying especially when analysing long functions with hundreds of lines.

I changed it for the up and down keys but forgot about doing this for mouse scrolling, fixed now.

Resizing the game area doesn't really update the rendering area of the code view so if I wanted to give it more space, I would have to resize and then pause + play which is sometime impossible (cause the asm would have to be read right now)

I didn't clarify this in the first time I addressed this (see that for possible "fixes"): That's something too complicated for me to fix right now so I'll have to postpone it (also not really related to this PR).

I have no idea why you can select multiple lines in the code view by holding the click and drag the mouse, no options uses that.

Overlooked that, sorry. Fixed now.

Holding the click and dragging does weird things (it still triggers the event continuously, it should only trigger once). This is true for the code view, the left rim of the code view and all the symbols panels.

I've talked about this before but a TL;DR: I think this is mostly working as intended and I don't get why you'd want to do this.

bl'ed function are not displayed instead of symbols name. Very important because it allows to see a bl call and its symbol so if I was working with generated symbols, I rename one and later on I see it gets called, I want to quickly see that this symbol that I renamed is called so I won't have to check inside that function as I already. figured it out. Brnached function are prexifed with --> to symbolise that. Same goes for regular branches, the symbol name is still the function itself because it branches inside that function, but it's prefixed with --> .

Really strange, works fine here. Edit: Or I'm misinterpreting what you mean. Screenshots of How it looks now vs How it should look would help a lot!

@spycrab spycrab force-pushed the qt_dbg_code branch 2 times, most recently from f8a62d8 to 4272ff7 Compare February 26, 2018 07:41
@container1234
Copy link
Contributor

I tested on Windows 7 and found some issues.

  • When instruction breakpoint is hit, dolphin pauses on wrong address.
  • When instruction breakpoint is set from code window, it won't be displayed on breakpoints window until new breakpoint is set from breakpoints window.
  • When memory breakpoint is hit, dolphin UI is not updated and have to press pause button to update manually.
  • The table is too dark.
  • Row height is bit too long.
    snapcrab_noname_2018-2-26_18-25-32_no-00
  • Stepping commands take a little long time than wx (It may be because of my poor PC).

@spycrab
Copy link
Contributor Author

spycrab commented Feb 26, 2018

Some stuff is still broken. Will address those once I have time.

@spycrab
Copy link
Contributor Author

spycrab commented Feb 28, 2018

@container1234 @aldelaro5 Should be all addressed now, except for

When memory breakpoint is hit, dolphin UI is not updated and have to press pause button to update manually.

That will be addressed in the upcoming Memory Widget PR.

@leoetlino leoetlino added this to the Qt milestone Mar 9, 2018
@aldelaro5
Copy link
Member

Sorry for the delay, but I finally had time to review again and I noticed one problem:

I actually really like how you can hide each of the 4 symbols panels on the left of the code view, but I realised that if you do so, it's pretty confusing to figure out what was hidden and how to unhide it (you just see an awkward series of lines if you collapses some). What I suggest is using a collapsable view where the header is always visible, kind of like this: http://oi42.tinypic.com/11jn6ko.jpg This is especially fitting with the panels being laied down veritcally and it still saves enough space for it to matter.

Other than that, everything seems alright.

@spycrab
Copy link
Contributor Author

spycrab commented Mar 15, 2018

What I suggest is using a collapsable view where the header is always visible, kind of like this: http://oi42.tinypic.com/11jn6ko.jpg

While that would be nice. I don't think it can be done easily with Qt.

@container1234
Copy link
Contributor

The table becomes dark after symbols are generated.
1
2

@Helios747 Helios747 merged commit 1c3cc26 into dolphin-emu:master Mar 15, 2018
@spycrab spycrab deleted the qt_dbg_code branch March 15, 2018 18:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
6 participants