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

Assertion and panic alert improvements #10209

Merged
merged 13 commits into from Jan 9, 2022

Conversation

Pokechu22
Copy link
Contributor

@Pokechu22 Pokechu22 commented Nov 11, 2021

  • The failed condition and the relevant function are now included in the assert message. This applies even when using ASSERT_MSG; now, all relevant information is shown with that too.
  • ASSERT_MSG now uses fmt. I've updated all uses of this (and in some cases added additional details to the message).
  • The CHECK macro in the D3D backends has been removed and replaced with ASSERT_MSG.
  • I added an HRWrap struct that can be used to format HRESULT in messages; a fmt::formatter exists for it. This uses _com_result based on this. This gives MUCH more useful information (compared to both just a hex value, and to no information at all (which was the norm for most uses)) and it should be fairly user-friendly. I put this in D3DCommon.h, but maybe this should be somewhere else so that it could be used in other code. This is now located in Common/HRWrap.h, and used by a few places outside of D3D. D3D11 and D3D12 both have their own variants that call GetDeviceRemovedReason when needed.
  • The legacy non-format variants of PanicAlert have been removed; they're no longer used by anything.
  • ASSERT_MSG's first parameter now indicating the log type is now actually used.
  • PanicAlertFmtT now actually works. Yes, this was broken. The values were extracted for translation beforehand, but they were never actually translated at runtime. You can test this by modifying dsp_rom.bin so that it has an invalid hash, and then setting the language to French; the message will still be in English despite a translation for it existing. This will hopefully make it easier for people to understand these messages (though it may make things a bit harder for support).
Before
diff --git a/Source/Core/Core/Core.cpp b/Source/Core/Core/Core.cpp
index 622d7f9d90..9174a0ca94 100644
--- a/Source/Core/Core/Core.cpp
+++ b/Source/Core/Core/Core.cpp
@@ -20,6 +20,7 @@
 
 #include "AudioCommon/AudioCommon.h"
 
+#include "Common/Assert.h"
 #include "Common/CPUDetect.h"
 #include "Common/CommonPaths.h"
 #include "Common/CommonTypes.h"
@@ -439,6 +440,8 @@ static void FifoPlayerThread(const std::optional<std::string>& savestate_path,
 // See the BootManager.cpp file description for a complete call schedule.
 static void EmuThread(std::unique_ptr<BootParameters> boot, WindowSystemInfo wsi)
 {
+  ASSERT(false);
+  ASSERT_MSG(CORE, false, "Test: %d", 42);
   const SConfig& core_parameter = SConfig::GetInstance();
   CallOnStateChangedCallbacks(State::Starting);
   Common::ScopeGuard flag_guard{[] {

would result in these dialogs:

image
image

and this being logged:

27:16:705 Common\MsgHandler.cpp:112 E[MASTER]: Warning: An error occurred.

  Line: 443
  File: C:\Users\Pokechu22\source\repos\dolphin\Source\Core\Core\Core.cpp

Ignore and continue?
27:20:730 Common\MsgHandler.cpp:112 E[MASTER]: Warning: Test: 42
After
diff --git a/Source/Core/Core/Core.cpp b/Source/Core/Core/Core.cpp
index 622d7f9d90..c447c208e1 100644
--- a/Source/Core/Core/Core.cpp
+++ b/Source/Core/Core/Core.cpp
@@ -20,6 +20,7 @@

 #include "AudioCommon/AudioCommon.h"

+#include "Common/Assert.h"
 #include "Common/CPUDetect.h"
 #include "Common/CommonPaths.h"
 #include "Common/CommonTypes.h"
@@ -439,6 +440,8 @@ static void FifoPlayerThread(const std::optional<std::string>& savestate_path,
 // See the BootManager.cpp file description for a complete call schedule.
 static void EmuThread(std::unique_ptr<BootParameters> boot, WindowSystemInfo wsi)
 {
+  ASSERT(false);
+  ASSERT_MSG(CORE, false, "Test: {}", 42);
   const SConfig& core_parameter = SConfig::GetInstance();
   CallOnStateChangedCallbacks(State::Starting);
   Common::ScopeGuard flag_guard{[] {

would result in these dialogs:

image
image

and this being logged:

23:50:874 Common\MsgHandler.cpp:113 E[MASTER]: Warning: An error occurred.

  Condition: false
  File: C:\Users\Pokechu22\source\repos\dolphin\Source\Core\Core\Core.cpp
  Line: 443
  Function: EmuThread

Ignore and continue?
23:59:503 Common\MsgHandler.cpp:113 E[CORE]: Warning: An error occurred.

Test: 42

  Condition: false
  File: C:\Users\Pokechu22\source\repos\dolphin\Source\Core\Core\Core.cpp
  Line: 444
  Function: EmuThread

Ignore and continue?

@Pokechu22 Pokechu22 changed the title Assert fmt Assertion and panic alert improvements Nov 11, 2021
@Pokechu22 Pokechu22 force-pushed the assert-fmt branch 5 times, most recently from e9fb81c to 37bab59 Compare November 11, 2021 08:17
@Pokechu22 Pokechu22 force-pushed the assert-fmt branch 6 times, most recently from 971cbdd to 3fddbcc Compare November 17, 2021 02:51
@Pokechu22
Copy link
Contributor Author

I've added another commit, which uses the caller's file and line number for panic alerts instead of the line number of the log call in ShowMessageAlert in MsgAlert.cpp.

Example
46:45:173 Core\Core.cpp:443 E[MASTER]: Warning: An error occurred.

  Condition: false
  File: C:\Users\Pokechu22\source\repos\dolphin\Source\Core\Core\Core.cpp
  Line: 443
  Function: EmuThread

Ignore and continue?
46:49:162 Core\Core.cpp:444 E[CORE]: Warning: An error occurred.

Test: 42

  Condition: false
  File: C:\Users\Pokechu22\source\repos\dolphin\Source\Core\Core\Core.cpp
  Line: 444
  Function: EmuThread

Ignore and continue?

Source/Core/Common/MsgHandler.h Outdated Show resolved Hide resolved
Source/Core/Common/x64Emitter.cpp Show resolved Hide resolved
Source/Core/Common/MsgHandler.h Outdated Show resolved Hide resolved
Source/Core/Common/MsgHandler.h Outdated Show resolved Hide resolved
@Pokechu22 Pokechu22 force-pushed the assert-fmt branch 3 times, most recently from e741a81 to 0a8d825 Compare December 12, 2021 17:34
@Pokechu22 Pokechu22 force-pushed the assert-fmt branch 3 times, most recently from 16c2302 to 158616c Compare December 18, 2021 20:56
@Pokechu22 Pokechu22 force-pushed the assert-fmt branch 2 times, most recently from 290f6da to a068548 Compare December 23, 2021 00:35
Copy link
Contributor

@iwubcode iwubcode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wanted to make sure this didn't get lost. Untested but looks like a huge improvement!

Most of these became unneeded when fmt was introduced.
Since it was unused, nonexistent values were used in a few places.  I've replaced them.
Specifically, this meant that __func__ in macros (namely ASSERT) would always be evaluate to "operator ()".
This will assist with finding the source of a panic alert based on logs; before, Common\MsgHandler.cpp:113 (or similar) was always used.
HRWrap now allows HRESULT to be formatted, giving useful information beyond "it failed" or a hex code that isn't obvious to most users.  This commit does not add any uses of it, though.
Note that D3DCommon can't use DX11HRWrap or DX12HRWrap since it's shared between them.
@dolphin-emu-bot
Copy link
Contributor

FifoCI detected that this change impacts graphical rendering. Here are the behavior differences detected by the system:

  • dbz-depth on mvk-osx-m1: diff
  • DKCR-Char on mvk-osx-m1: diff
  • DKCR-fast-depth on mvk-osx-m1: diff
  • ea-pink on mvk-osx-m1: diff
  • lego-star-wars-crane-shadow on mvk-osx-m1: diff
  • pm-hc-jp on mvk-osx-m1: diff
  • sw3-dt on mvk-osx-m1: diff
  • thps4-shadow on mvk-osx-m1: diff

automated-fifoci-reporter

@JosJuice JosJuice merged commit 3da6487 into dolphin-emu:master Jan 9, 2022
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
7 participants