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: Implement Debugger (part 1 of ∞) #6076

Merged
merged 6 commits into from
Jan 3, 2018

Conversation

spycrab
Copy link
Contributor

@spycrab spycrab commented Sep 19, 2017

This implements

  • Windows
    • Registers
    • Watch
    • Breakpoints
  • Interface
    • Add ability to enable / disable debugging from the interface tab

The rest will be implemented in a later PR


RegisterColumn::RegisterColumn(RegisterType type, std::function<u64()> get,
std::function<void(u64)> set)
: m_type(type), m_get_register(get), m_set_register(set)

This comment was marked as off-topic.

setData(DATA_TYPE, static_cast<quint32>(type));
}

RegisterDisplay RegisterColumn::GetDisplay()

This comment was marked as off-topic.

case RegisterDisplay::Float:
{
float f = text().toFloat(&valid);
value = *reinterpret_cast<u32*>(&f);

This comment was marked as off-topic.

text = QString::number(static_cast<quint32>(m_value));
break;
case RegisterDisplay::Float:
text = QString::number(*reinterpret_cast<float*>(&m_value));

This comment was marked as off-topic.

#pragma once

#include <QDockWidget>
#include <QTableWidget>

This comment was marked as off-topic.

#include <QDockWidget>
#include <QTableWidget>

#include <functional>

This comment was marked as off-topic.

@spycrab spycrab force-pushed the qt_debugger branch 3 times, most recently from c49b96f to 67e089c Compare September 20, 2017 13:12
@aldelaro5
Copy link
Member

Just a little precision for the comments in RegisterColumn.h, the lr is the link register, the cr is the condition register, the fpsctr is the floating point status and control register and the ctr is the count register. I am not too knowledgeable about the other ones marked as ??

@sepalani
Copy link
Contributor

These are detailed in PPC user manuals. In some of them, there is an "Acronyms and Abbreviated Terms" table.

Term Meaning
MSR Machine state register
SRR Machine status save/restore register
SR Segment register
DSISR Register used for determining the source of a DSI exception
DAR Data address register

AddRegister(28, 5, RegisterType::dar, "DAR", [] { return PowerPC::ppcState.spr[SPR_DAR]; },
[](u64 value) { PowerPC::ppcState.spr[SPR_DAR] = value; });

// DAR

This comment was marked as off-topic.


namespace Config
{
const ConfigInfo<bool> DEBUGGER_SHOW_REGISTERS({System::Main, "Interface",

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

@@ -6,10 +6,13 @@
#include <QSettings>
#include <QSize>

#include <iostream>

This comment was marked as off-topic.

This comment was marked as off-topic.

@spycrab
Copy link
Contributor Author

spycrab commented Sep 21, 2017

@sepalani Actually looked at that table a bunch of times but was too lazy to fill them in thus far. Thanks for reminding me.

@leoetlino leoetlino added the WIP / do not merge Work in progress (do not merge) label Sep 21, 2017
@spycrab spycrab force-pushed the qt_debugger branch 2 times, most recently from 6eb593f to a9a01e9 Compare September 25, 2017 17:32
@JMC47
Copy link
Contributor

JMC47 commented Sep 26, 2017

There isn't very much feedback when turn on debug UI is hit. I know not saving it is probably fine, but, I didn't even notice it was working.

I can't seem to launch any games to see if the registers are actually working.

@spycrab
Copy link
Contributor Author

spycrab commented Sep 27, 2017

@JMC47 I think that issue will be gone once the toolbar buttons are implemented.

About the UI crashing when a game is launched: I can't confirm it on Linux, so it seems to be a platform specific issue. Will have to debug it under Windows later.

@leoetlino leoetlino added this to the Qt milestone Sep 27, 2017
@spycrab spycrab force-pushed the qt_debugger branch 6 times, most recently from 7385e2c to b1b07ec Compare October 5, 2017 13:10
@spycrab spycrab changed the title [WIP] Qt: Implement Debugger Qt: Implement Debugger (part 1 of ∞) Nov 19, 2017
@spycrab
Copy link
Contributor Author

spycrab commented Nov 19, 2017

Rebased and finished this off. I'll implement the other missing features / widgets in another PR.

@JosJuice JosJuice removed the WIP / do not merge Work in progress (do not merge) label Nov 19, 2017
@JMC47
Copy link
Contributor

JMC47 commented Nov 21, 2017

I tested this a bit more with the more complete setup. It appears setting breakpoints and such work. I do get a bit confused by the way you can move windows around, but it's actually more robust than WX and probably just something I should learn to handle.

@aldelaro5
Copy link
Member

@JMC47 actually, that was possible in wx, you could move the window around and dock them as well as having them float. However, it seems the Qt version offers more pleasing animation when you do so. You do need to move the header bar instead of the tab which threw me off a bit (I am very used to WX), but once I got that, it was fine.

Now, I just did some testing with this UI as I said, I am very used to the WX debugger and since this is a partial commit and I know Qt is in the work, I noticed a couple of issues, but I don't know if any of them are intentional or unintentional so I might as well list them.

  • The FPR columns in the register view are not wide enough, the FPR are 64 bit long AND there's also paired FPR so there shoudl be 2 columns wide enough to accomodate 16 hex character each, however, the current one only does for 8 of them which means you cannot see the entire value. I also seem to notice the option to resize any column has been removed which is a problem considering I set my debug font to be slightly wider than my monospace one. May I suggest to first make the FPR columns default width to be wider and then make every register value column resizable?
  • The breakpoint add dialog has 2 main problems. First, if I select memory breakpoint, uncheck all actions and triggers, go back to instruction breakpoint, enter an address, I am being told no trigger was selected which is a message that can't possibly happen on an instruction breakpoint. The other problem I noticed is the WX dialog used radio buttons for these because it is not possible to have a breakpoint with no action or trigger. In my opinion, this is a slightly better solution than having an error message to handle this case, it would lead the user to not do this error in any circumstances.
  • Memory breakpoints don't work, I tested using the RNG seed address of Super Mario Sunshine as well as Pokemon Colosseum, I can see it does get written to if I update the watch, but no breakpoints is triggered on write despite the fact that I checked in the code and the memcheck (as they are called in the code) does get added. Not sure if this one is normal, but I might as well tell it now.
  • If I add a memory breakpoint as soon as I see the UI and I don't do anything before that, it goes well. However, if I start a game, close the render window then attempt to add a memory breakpoint, Dolphin crashes. It seems to occur in a function to be ran as cpu thread when attempting to add the memcheck.

That seems to be what I noticed. I do like the use of docker here and how they move with better animation than WX. I also am for the merging of the instruction breakpoint and memory breakpoint, it actually feels weird I didn't thought about doing it now because it does feel simpler :) Other than the issues I mentioned, that makes me look forward to this debugger, it just feels a bit nicer to work with.

@lioncash
Copy link
Member

Just leaving this here in case it's useful, but x64dbg is also something that could be used as a reference/prior work for some things, particularly considering it's also written with Qt as well.

@spycrab
Copy link
Contributor Author

spycrab commented Nov 24, 2017

@lioncash I'll definitely have a look at it. Thanks.

@spycrab
Copy link
Contributor Author

spycrab commented Nov 24, 2017

@aldelaro5 Thanks for your feedback!

The FPR columns in the register view are not wide enough, the FPR are 64 bit long AND there's also paired FPR so there shoudl be 2 columns wide enough to accomodate 16 hex character each, however, the current one only does for 8 of them which means you cannot see the entire value. I also seem to notice the option to resize any column has been removed which is a problem considering I set my debug font to be slightly wider than my monospace one. May I suggest to first make the FPR columns default width to be wider and then make every register value column resizable?

Fixed the FPR column and made all columns resizable.

The breakpoint add dialog has 2 main problems. First, if I select memory breakpoint, uncheck all actions and triggers, go back to instruction breakpoint, enter an address, I am being told no trigger was selected which is a message that can't possibly happen on an instruction breakpoint. The other problem I noticed is the WX dialog used radio buttons for these because it is not possible to have a breakpoint with no action or trigger. In my opinion, this is a slightly better solution than having an error message to handle this case, it would lead the user to not do this error in any circumstances.

Using checkboxes now, so this is fixed as well.

I'm not sure about the last two problems.
Any ideas, @dolphin-emu/core-developers ?

After some asking around in IRC it seems like those last two issues don't seem to be Qt related, so I can't do anything about them.

@spycrab
Copy link
Contributor Author

spycrab commented Jan 1, 2018

Rebased and ready to roll.

@delroth delroth merged commit fd13851 into dolphin-emu:master Jan 3, 2018
@spycrab spycrab deleted the qt_debugger branch January 3, 2018 02:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

9 participants