Qt/Debugger CodeWidget: Record and find specific functions by differencing

[dreamsyntax]

UPDATE 2021-12-28, See bottom comments for updated screenshots. This main post does NOT reflect the final state.

For original PR, see #7679
TryTwo has since gone dark.
All credit for initial version of CodeDiff to TryTwo.

The PR, has been changed quite a bit. See final comments in thread.

---original description---
Adds a button to the code widget to open the differencing window.

Uses the profiler to record the functions that run, then the user adds the recording to an exclude or include list. The include list has the excludes subtracted from them, then gets displayed. The lists are symbol-based to avoid capturing a function multiple times. Allows finding specific functions based on when they run.

For example, recording the player standing still then pressing exclude, followed by moving around then pressing include, will find functions related to movement and movement animations. Specifically looking for an action or event can often yield a small amount of functions being returned, which is quite useful.

The profiler is sort of hijacked for this. Building a specific routine for recording the functions might increase efficiency, but I'm not sure how to.
---end original description---

• Requires a saved symbol map

• Clicking an item in the Diff jumps to the instruction that was diff'd

• Right clicking allows:
  ** Go to start of function - Jumps CodeView to function start of the diff'd instruction
  ** Toggle blr - Sticks a blr at the start of the function, colors item red in the list
  ** Delete - Deletes item from the list

• This feature makes it easy to pinpoint functions that would otherwise be difficult with only memory scans.

Screenshots:

CodeWidget:

DiffDialog Start:

DiffDialog Post Recording & Exclude/Include w/Entry Context Menu; Also shows 'isRecording' button

[sepalani]

It can probably be a really nice feature but the Diff Window should be improved a little, IMHO.

• A label should be used (even when recording) for the Included/Excluded values.
• I feel the way these buttons are placed isn't intuitive. Buttons to Start/Stop recording and to Pause/Resume might be better.

What does the "Reset All" button do? Does it clear the list (or exclude all these items though the Included/Excluded values aren't updated)?

Is there any use-case for "toggle BLR"? Can't it be replaced with "Toggle Breakpoint" instead. Usually, you can see the caller using the callstack and don't need to patch the function with a BLR which can often crash the game.

Here is also an idea (that might be addressed in another PR) to improve this feature:

• Add a button to filter by hit (like Cheat Engine ultimap)

[dreamsyntax]

• A label should be used (even when recording) for the Included/Excluded values.

I don't understand. Do you mean dynamically populating?

• I feel the way these buttons are placed isn't intuitive. Buttons to Start/Stop recording and to Pause/Resume might be better.

This can definitely be improved to have more features. But as a first version this meets its intended purpose (I have used it for a year in misc hacking projects).

What does the "Reset All" button do? Does it clear the list (or exclude all these items though the Included/Excluded values aren't updated)?

This resets all the lists. I will add the label update, good catch.

Is there any use-case for "toggle BLR"? Can't it be replaced with "Toggle Breakpoint" instead. Usually, you can see the caller using the callstack and don't need to patch the function with a BLR which can often crash the game.

This is a misunderstanding the intended use of the feature. Toggle BLR is the main reason this exists.
The general flow of using this is:

• Generate the list
• Create a savestate
• blr an item in the list, check if the behavior you are searching for exists. If it doesnt, you found a potentially related function to what you are looking for. From here you inspect the function and make asm patches etc as needed.
• If the behavior still exists, load state, try next function.
• Rinse and repeat

Here is also an idea (that might be addressed in another PR) to improve this feature:

• Add a button to filter by hit (like Cheat Engine ultimap)

To clarify, when you say filter by hit you mean filtering by the caller? I have a few ideas of how this can be improved. I have a very basic save/load of the list implemented for the scenario of Dolphin crashing (which happens a lot when doing deep dive game debugging), but the robustness is lacking so it is not included in this PR.

[sepalani]

Include/Exclude size label

A label should be used (even when recording) for the Included/Excluded values.

I don't understand. Do you mean dynamically populating?

I was referring to m_exclude_size_label and m_include_size_label being replaced from text ("Exclude"/"Include") to bare numbers without keeping the text. IMO, it's very misleading. I also doubt these count values are useful, especially when thread scheduling can pollute the result with threads being resumed at different time. Unless you know precisely when threads and other asynchronous tasks might interrupt you, I don't think it's worth keeping. I don't mind having them if you address the label issue.

Toggle BLR

This is a misunderstanding the intended use of the feature. Toggle BLR is the main reason this exists.
The general flow of using this is:

• Generate the list
• Create a savestate
• blr an item in the list, check if the behavior you are searching for exists. If it doesnt, you found a potentially related function to what you are looking for. From here you inspect the function and make asm patches etc as needed.
• If the behavior still exists, load state, try next function.
• Rinse and repeat

I see, that makes sense. You can leave it as is, then.

UI/UX issue

Regarding my UI/UX issue, if we have a look at CE Ultimap: https://wiki.cheatengine.org/index.php?title=Ultimap1

The recording part and the filtering part of the ultimap is crystal clear. Buttons are also disabled when there aren't usable which isn't the case in your PR.

Add a button to filter by hit (like Cheat Engine ultimap)

My statement refers to the "Filter our routine(s) where callcount is not" button from CE. As stated, you can leave it for a follow-up PR if you want.

In sum

IMHO, this PR shouldn't be merged as long as these UI/UX issues aren't addressed:

• Address the size label issue
• Disable buttons when they shouldn't be used
• Make the "recording part" clear (ideally, with a start, pause/resume, stop button)
• Make the "filtering part" clear

Suggestions:

• The list should be name "Matching results" or something along those lines
• Add a "Filter our routine(s) where callcount is not" button
• Add a help button (displaying the info at the start)

Regarding "making the recording/filtering part clear" you can use GroupBox or any other Qt features that are fitting.

[AdmiralCurtiss]

Yeah I played with this for a bit and while it works, it's definitely a bit rough. Some annoyances I ran across:

• As a colorblind person, the red color for whether we're currently skipping this function is very hard to see. Please add a column with an icon indicating this or something like that.
• Loading a savestate doesn't seem to update the rows to indicate if they're still blr-replaced or not.
• Renaming a symbol doesn't update the corresponding symbol in the search table.
• Similarly, a 'rename this function' option in the context menu would be nice.
• I don't actually understand what the 'address' column is, exactly. I figured it would be the start of the function, but that doesn't seem to be the case, it just seems like a random address within the function?
• Hitting 'toggle blr' a second time doesn't seem to actually revert the blr injection.

[AdmiralCurtiss]

Oh and I just right-clicked on the table header and hit 'go to start of function' (meant to click the first row) and then dolphin crashed.

[dreamsyntax]

Thanks for the clarification sepalani. The original PR actually labeled the Include/Exclude as "Code did not get executed" and "Code has been executed". I will revert to that and address your other points.

AdmiralCurtiss

• As a colorblind person, the red color for whether we're currently skipping this function is very hard to see. Please add a column with an icon indicating this or something like that.
  Similarly, a 'rename this function' option in the context menu would be nice.

Good points. Edit: This would require a larger change, saving for another PR

• Loading a savestate doesn't seem to update the rows to indicate if they're still blr-replaced or not.

This is not the intended result. The red/indication of a past blr is supposed to be independent of current game state. Think of it checklist and you are marking items you have interacted with. Its very rare you would blr multiple functions when searching.

• Renaming a symbol doesn't update the corresponding symbol in the search table.

A downside to the way this is currently implemented is expects the symbol map to not change. Typically in my flow if I rename a symbol I have determined its purpose and thus can ignore it in the Diff window list.

• I don't actually understand what the 'address' column is, exactly. I figured it would be the start of the function, but that doesn't seem to be the case, it just seems like a random address within the function?

This is the address that was diff'd. Think of the scenario of a game with DEBUG flag in a file that is read at init. The same symbol may have a ble/bne that skips over a segment of code when DEBUG is false, but ignores the branch when true. The 'address' is the line of code that was different in such executions. In general, we only care about the function start for most cases but its useful if dealing with very long functions near the mainloop.

• Hitting 'toggle blr' a second time doesn't seem to actually revert the blr injection.

I'm not sure if TryTwo planned to eventually add this. But given that game behavior will commonly break when blr'ing, it makes little sense to allow restoration. Do you have a suggestion on what to name this?
"patch to blr"
"set blr at start of function"

Oh and I just right-clicked on the table header and hit 'go to start of function' (meant to click the first row) and then dolphin crashed.

You likely renamed the symbol for the function in the table. If the symbol name no longer exists in the table a crash occurs. Edit: This is not what you meant, I found your issue. Addressed both the issue you ran into and the one I thought it was.

Thank you for the review

[dreamsyntax]

IMHO, this PR shouldn't be merged as long as these UI/UX issues aren't addressed:

• Address the size label issue
• Disable buttons when they shouldn't be used
• Make the "recording part" clear (ideally, with a start, pause/resume, stop button)
• Make the "filtering part" clear

Suggestions:

• The list should be name "Matching results" or something along those lines
• Add a "Filter our routine(s) where callcount is not" button
• Add a help button (displaying the info at the start)

Regarding "making the recording/filtering part clear" you can use GroupBox or any other Qt features that are fitting.

---

On 'adding an element to the results' instead of coloring red to show Blr, the way its currently done is with just one String tabbed out. It would be quite awkward to inject it after a symbol name given some symbol maps have very long names for functions. I'm looking to address this in an alternative way in my second PR with the Pause/Stop/Resume

I have attempted a start/stop/pause, but the way JitProfiler is used makes this a bit more complicated. I'm holding off on adding this behavior for another PR. As is the behavior meets the minimum requirements as a useful tool for modders and mappers, even if simple.

To support clarity of what's happening when Recording, I modified the help text that starts when Diff is first launched.
Recording currently does not support pausing. When you stop recording, the recorded results are erased. You can only get results by clicking 'Code did not get executed' and 'Code has been executed'. The disabled buttons helps reinforce how usage is expected to the user.

On the topic of the 'Help' I considered moving it to another Dialog or calling MessageBox, but ultimately I think it serves best to 'be in your face' when you launch Diff, especially if you have not used Ultimap before. But this is an opinion and if the majority prefer it gone I'm fine with that.

In sum:

• I renamed Include/Exclude to match the original counterpart
• Fixed the misc crashes (nullchecks).
• Rename list to match behavior
• Disable/Enable buttons dependent on when its intended to be used
• Removed some unused QElements / cleanup
• Renamed Toggle Blr to Set Blr
• List renamed
• Experimented with pause/stop/resume etc, but complexity makes me want this moved to the next PR

Supporting images:

[dreamsyntax]

@sepalani requesting rebuild for bot and your thoughts if this is ready.
I will rebase to latest commit if necessary (again)

[TryTwo]

Hey thanks for working on this!! I haven't had time to do anything and compiling the current dolphin stuff is now giving me troubles. Plus quick repeated pauses/debug updates are crashing the dev builds for me, so I can't tell when or if my code is breaking something.

I realized my function finder here might be doing one thing wrong. It's dropping included functions we no longer want, but instead it should be moving them to the exclude list so they never return. Not certain if that was a possible outcome, but if you hit include multiple times you always want functions that have only run each time include was pressed, not just during some of those times. I've fixed this and rewrote the logic entirely, making it clearer. For extra clarity, I removed some optimizations that were probably unnecessary. It should be given a thorough test to make sure I didn't mess up a case (like start -> exclude -> exclude -> include),

Update comparison logic (expand)

void CodeDiffDialog::OnIncludeExclude(bool include)
{
    bool isize = m_include.size() != 0;
    bool xsize = m_exclude.size() != 0;
    Profiler::ProfileStats prof_stats;
    auto& blockstats = prof_stats.block_stats;
    JitInterface::GetProfileResults(&prof_stats);
    std::vector<Diff> current;
    std::vector<Diff> new_exclude;
    current.reserve(20000);
    new_exclude.reserve(20000);

    // Convert blockstats to smaller struct Diff. Exclude repeat functions via symbols.
    for (auto& iter : blockstats)
    {
      Diff tmp_diff;
      std::string symbol = g_symbolDB.GetDescription(iter.addr);
      if (!std::any_of(current.begin(), current.end(),
                       [&symbol](Diff& v) { return v.symbol == symbol; }))
      {
        tmp_diff.symbol = symbol;
        tmp_diff.addr = iter.addr;
        tmp_diff.hits = iter.run_count;
        current.push_back(tmp_diff);
      }
    }

    // Could add address based difference instead of symbols. Probably need second function.
    // Sort for lower_bound.
    sort(current.begin(), current.end(),
         [](const Diff& v1, const Diff& v2) { return (v1.symbol < v2.symbol); });

    // If both lists are empty, write and skip.
    if (!isize && !xsize)
    {
      if (include)
        m_include = current;
      else
        m_exclude = current;
      return;
    }

    // We only want symbols that appear every time Include is pressed. Therefore, we add symbols
    // that only appear some of the time to the exclude list.
    if (include && isize)
    {
      // Compare include with current.
      for (auto& iter : m_include)
      {
        if (!std::any_of(current.begin(), current.end(),
                         [&](Diff& v) { return v.symbol == iter.symbol; }))
          new_exclude.push_back(iter);
      }

      for (auto& iter : current)
      {
        if (!std::any_of(m_include.begin(), m_include.end(),
                         [&](Diff& v) { return v.symbol == iter.symbol; }))
          new_exclude.push_back(iter);
      }
    }

    // Update exclude list.
    // !xsize = exclude list empty. !include = exclude.
    if (!xsize && !include)
    {
      m_exclude.swap(current);
    }
    else if (!xsize && include && new_exclude.size() != 0)
    {
      m_exclude.swap(new_exclude);
    }
    else if (xsize)
    {
      for (auto& iter : (include ? new_exclude : current))
      {
        auto pos = lower_bound(m_exclude.begin(), m_exclude.end(), iter.symbol, AddrOP);

        if (pos->symbol != iter.symbol)
        {
          m_exclude.insert(pos, iter);
        }
      }
    }

    // If exclude pressed and there is no include list, we're done.
    if (!include && !isize)
      return;

    // Update include list
    if (!isize && include)
      m_include.swap(current);

    // Compare include with exclude.

    // Alt method, probably worse:
    // std::vector<Diff> tmp_swap;
    // for (auto& iter : m_include)
    //{
    //  if (!std::any_of(m_exclude.begin(), m_exclude.end(),
    //                   [&](Diff& v) { return v.symbol == iter.symbol; }))
    //    tmp_swap.push_back(iter);
    //}
    // m_include.swap(tmp_swap);

    for (auto& list : m_exclude)
      m_include.erase(std::remove_if(m_include.begin(), m_include.end(),
                                     [&](Diff const& v) { return v.symbol == list.symbol; }),
                      m_include.end());
}

If it is slow, it might be possible to wrap the whole thing in a thread code:
Core::RunAsCPUThread([&] { code });
but I'm never certain when it'll work right. Maybe after the .reserve(20000) lines to avoid the Jit call?

I also have an instruction tracing PR that I didn't PR because it requires some gekko instruction consistency fixes. I might try to get that up if you want to help with it.

[dreamsyntax]

Hey thanks for working on this!! I haven't had time to do anything and compiling the current dolphin stuff is now giving me troubles. Plus quick repeated pauses/debug updates are crashing the dev builds for me, so I can't tell when or if my code is breaking something.

I'm not having this issue personally.

I realized my function finder here might be doing one thing wrong. It's dropping included functions we no longer want, but instead it should be moving them to the exclude list so they never return. Not certain if that was a possible outcome, but if you hit include multiple times you always want functions that have only run each time include was pressed, not just during some of those times. I've fixed this and rewrote the logic entirely, making it clearer. For extra clarity, I removed some optimizations that were probably unnecessary. It should be given a thorough test to make sure I didn't mess up a case (like start -> exclude -> exclude -> include),

I've been using the current implementation for over a year in a few projects, and have always been able to identify code I'm looking for with this tool, so I'm skeptical there is an issue.

Even with the above information I still think this PR should get into master, this is a highly used feature for game modders. A follow up PR can get the pause/resume (cheat engine like functionality) + any optimizations etc we can do.

[TryTwo]

Ok, I was going cross-eyed trying to read my original optimized logic. If it's already working as expected, then it's probably best not to change it now.

[dreamsyntax]

Updated to latest master

[dreamsyntax]

Update to 12489 (latest as of 2020-08-28).
Fixed merge conflicts with new QtMoc project setup.

@TryTwo once this is merged do you have any interest in making the coloring change and 'pause' feature?

[TryTwo]

Yeah I can work on it. What should pause do? Are you holding the current pending recording, but stopping recording, then resuming and continuing to add to the current pending later? Is that actually needed?

/edit What should we do for color? We can strikethrough, bold, use a different color...

[dreamsyntax]

Updated to 12728 (2020-10-03)

Yeah I can work on it. What should pause do? Are you holding the current pending recording, but stopping recording, then resuming and continuing to add to the current pending later? Is that actually needed?

/edit What should we do for color? We can strikethrough, bold, use a different color...

Strike through seems to be a decent compromise. We could keep the red but additionally strike through or italicize.

I don't actually need the behavior, but yes you described pause as sepalani suggested, trying to get closer feature parity with Cheat Engine Ultimap.

Updated the help string to be formatted nicer:

I see the automated build is failing, so merging to latest master and double checking I didn't miss the local clang lint.
https://dolphin.ci/#/builders/21/builds/1692
https://dolphin.ci/#/builders/12/builds/1692

Strange. Failing on some moc within the builder.

Requesting a re-run on the new commit I just pushed.

Update 2020-10-07:
Re-run finished successfully.

[dreamsyntax]

Rebase over Dolphin-13399 (Dec 31 2020)

[dreamsyntax]

Unclear if this ever happens, but if symbol_diff doesn't include an entry for original_includes.symbol,
...

I think I misunderstand the above.
symbol_diff could have elements that do not exist in original_includes. Would that be the scenario you are referring to?
I'll add the check.

[AdmiralCurtiss]

No, other way around. If symbol_diff does not have a value that matches (or compares greater than) original_includes.symbol -- say, an empty symbol_diff, for example.

[dreamsyntax]

No, other way around. If symbol_diff does not have a value that matches (or compares greater than) original_includes.symbol -- say, an empty symbol_diff, for example.

I'm thinking that is not possible under the current function but I could be missing something. I don't see a harm in keeping it.

• Added the NoEditTriggers to the QTableWidget to prevent user editing fields (could lead to crashes, also is pointless to leave in)
• OnDelete was deleting from the list even if the list is empty. I don't think it's necessary to validate which element is being deleted, so for now doing a empty() call.
• Exclude " --- " from UpdateItem (invalid address scenario, also covers "No possible functions left. Reset." staying when clicked.

[dreamsyntax]

If everything looks good, let me know and I'll squash back to one commit.
Leaving as is so the recent changes can be reviewed first.

Rebased on latest master.

[TryTwo]

Is it just doing total hits?

I think you can just toss Total Hits in the struct. Update it once when m_include is initially made (total hits = hits), then do the += hits update to total hits rather than hits. Maybe also hits = hits for recent hits rather than first hits.

[dreamsyntax]

Maybe I'm missing something, but I'm having trouble setting a default size.

^ default

Added the new column for Total Hits vs Hits (recent)

Since we save geometry once a user adjusts it how they like it will persist.

[TryTwo]

Looks great to me! What size are you having trouble with and what command are you using?

[dreamsyntax]

Looks great to me! What size are you having trouble with and what command are you using?

I just meant the default state will be cutting off the Inspected column (image 1 in my comment above yours)
Its not really a big deal, I just thought when the Diff Window first appears it should show all the columns in full

[AdmiralCurtiss]

Looks great to me! What size are you having trouble with and what command are you using?

I just meant the default state will be cutting off the Inspected column (image 1 in my comment above yours) Its not really a big deal, I just thought when the Diff Window first appears it should show all the columns in full

You're not setting a size for the dialog, so it uses the default, which is pretty small. You may want to add a resize(600, 400); or similar after setLayout().

[AdmiralCurtiss]

Please also squish the comments once you think you're done with changes.

[dreamsyntax]

Note:
Lint wants QDialog as the first include for the .h for some reason

Here's the commit for the recent changes for easy viewing.
0d65af3

Let me know if the includes split is not how you wanted it.

[AdmiralCurtiss]

Note: Lint wants QDialog as the first include for the .h for some reason

Because there's no empty lines between. It will sort the files alphabetically, but not across empty lines.

Looks fine to me now otherwise, and even that is kinda whatever.

[dreamsyntax]

Rebased on master. Repushed (mainly because the pipeline was not showing up for some reason)
Here's the final version screenshots

[sepalani]

LGTM.

I played with it, checked the total hits/hits columns make sense. I didn't experience issues with it while including and excluding entries and got the results I expected.

[JosJuice]

Sorry for catching this late, but when trying to translate Dolphin's strings I came across this sentence that was added by this PR:

"Includes (Code has been executed) should have short recordings focusing on what you want."

What does this mean? I can't make sense of the sentence structure.

[TryTwo]

The "Code has been executed" button should be used on the shortest possible interval, to avoid catching irrelevant functions.

i.e. Record -> trigger action quickly -> press code has been executed quickly.

"Code has not been executed" should follow the opposite rule, being used over long intervals where the desired action wasn't triggered, to catch and eliminate everything you don't want.

[JosJuice]

Okay, so "include" is a noun, and "Code has been executed" is a clarification of what "include" refers to?

[TryTwo]

"Includes" is a noun that refers to items for/on the include list (list of possible functions you are looking for), which is updated with the "code has been executed button". So yeah, a clarification. I think the line could be rewritten a bit to be more clear maybe.