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

(old) PPCSymbolDB: Refactor SymbolMap Save/Load #13195

Closed
wants to merge 4 commits into from

Conversation

sepalani
Copy link
Contributor

@sepalani sepalani commented Nov 24, 2024

This PR refactors parts of the loading/saving process of symbol map. I'm putting it as a draft for now as I'd like to wait for https://dolp.in/pr13113 to be merged first.

This PR fixes issues I had where some symbols weren't loaded properly (which might get saved later). Here are some examples on how they can alter existing symbol maps.

The alignment wasn't properly detected and 0 was detected as name by sscanf.

This issue was also caused by the namepos/strstr logic which is flawed when the name is small and equals to part of the symbol address, size or vaddress. Moreover, checking for a space doesn't work when alignment is 16. I've seen this alignment value on some *fill* symbols.

-80468370 00000008 80468370 0 TRKPositionFile (entry of .text)  TRK_Hollywood_Revolution.a C:\products\RVL\runtime_libs\debugger\embedded\MetroTRK\Processor\
+80468360 00000008 80468360 0 0468360 00000008 80468360 0 TRKPositionFile (entry of .text)      TRK_Hollywood_Revolution.a C:\products\RVL\runtime_libs\debugger\embedded\MetroTRK\Processor\

savegpr / loadgpr entry wasn't properly detected

This issue also trimmed the object names after the "entry of" part.

-80456e54 000018 80456e54 0 _restgpr_27 (entry of __restore_gpr)        Runtime.PPCEABI.H.a runtime.o
+80456e54 000018 80456e54 0 __restore_gpr::

This PR is ready to be reviewed and tested. I also advise people testing this PR to backup their symbol maps just in case or test this PR in a dedicated portable build.

@sepalani sepalani marked this pull request as ready for review December 9, 2024 17:51
@sepalani
Copy link
Contributor Author

sepalani commented Dec 9, 2024

As #13113 was merged, this PR is ready to be reviewed and tested.

@dreamsyntax
Copy link
Member

I assume I should be able to just load an existing symbol map, then re-save and this should apply?

@sepalani
Copy link
Contributor Author

I assume I should be able to just load an existing symbol map, then re-save and this should apply?

Indeed that should work. It won't restore symbols which have already damaged (e.g. _restgpr_27 becoming __restore_gpr::), though.

@dreamsyntax
Copy link
Member

dreamsyntax commented Dec 18, 2024

If I have my map file in the \maps folder and use this build, Dolphin crashes on game boot (flatpak x64).

/app/bin/dolphin-emu-wrapper: line 6:    13 Aborted                 (core dumped) dolphin-emu "$@"

If I have no map file, boot the game, place the map file in \maps folder, and then choose Load Map
Dolphin hangs until I try to do another action, then crashes.

This is my map incase it's useful:
https://raw.githubusercontent.com/ShadowTheHedgehogHacking/shadow-symbols-and-dmw/refs/heads/main/GUPE8P.map

I verified the latest dev flatpak works as expected with the map.

@dreamsyntax
Copy link
Member

An update on this, interestingly compiling your branch on Windows I can't reproduce the crash.

@sepalani
Copy link
Contributor Author

I was trying to reproduce it on Windows and couldn't as well. I'll try to compile it on Linux and check if it gives similar results to the flatpak build. Otherwise, I'll try to rebase the PR to see if it fixes the issue.

@dreamsyntax
Copy link
Member

I was trying to reproduce it on Windows and couldn't as well. I'll try to compile it on Linux and check if it gives similar results to the flatpak build. Otherwise, I'll try to rebase the PR to see if it fixes the issue.

Just tried DolphinCI flatpak artifact on a fresh user folder on my desktop, it worked fine.

I think maybe the other install had some config issue, ill see if I can reproduce on the laptop again.

@dreamsyntax
Copy link
Member

dreamsyntax commented Dec 22, 2024

I think maybe the other install had some config issue, ill see if I can reproduce on the laptop again.

This is not the case.
I completely destoyed the .var/app/org.DolphinEmu.dolphin-emu folder, and am able to reproduce the crash.
After which I:

1. Enable Config -> Interface -> Debugging UI
2. Disable Dual Core
3. Place .map in \Maps
4. Launch game
5. Crash

I compiled your branch on Linux and do not experience the crash.
Its strange I can only reproduce this issue on the laptop, and only with the flatpak.
I verified the dGPU bug scenario is not related, as in either state it would not affect the Load Other Map function.

If you have any suggestions on how I can hook a debugger to the flatpak build or otherwise, let me know. I don't really see any useful logs.

--

Adding my discord post here too:

I'm trying to figure out why I get a crash when loading a symbol map on sepalani's PR.

But only on a laptop and the flatpak.
Native builds work fine. On a Desktop with one GPU, the flatpak also works fine.
This is expecially odd to me because it should have nothing to do with the other dGPU flatpak bug.
I can invoke the crash by doing the Load Other Symbol Map option long after the dGPU is successfully initialized and in-use.
I'll add it doesn't matter if I use the iGPU instead - the same crash occurs.
I just want to see a more useful trace other than
/app/bin/dolphin-emu-wrapper: line 6: 13 Aborted (core dumped) dolphin-emu "$@" which tells us nothing
Both the desktop and laptop are running the same kernel/OS (cachyOS/arch based), and same nvidia driver (565.77)
Both have the same flatseal flatpak restrictions (I override and allow 'All system files')
Desktop
-- CPU: AMD 5900X
-- GPU: NVIDIA RTX 3080
Laptop
-- CPU: Intel 13th Gen i9-13900HX
-- GPUs: iGPU RaptorLake UHD | dGPU NVIDIA RTX 4080 Mobile

@sepalani
Copy link
Contributor Author

I managed to trigger a crash on Windows when loading Shadow The Hedgehog. However, I can't find a way to reproduce it consistently and I'm unsure that's related to the flatpak issue:
image

D3D12Core.dll!00007ffe691950ce()
D3D12Core.dll!00007ffe6919198f()
D3D12Core.dll!00007ffe69192f01()
D3D12.dll!00007ffea0b16d40()
D3D12.dll!00007ffea0b168ec()
Dolphin.exe!DX12::DXContext::GetAAModes(unsigned int adapter_index) Line 65
	at d:\Source\Core\VideoBackends\D3D12\DX12Context.cpp(65)
Dolphin.exe!DX12::VideoBackend::FillBackendInfo() Line 83
	at d:\Source\Core\VideoBackends\D3D12\VideoBackend.cpp(83)
Dolphin.exe!DX12::VideoBackend::InitBackendInfo(const WindowSystemInfo & wsi) Line 46
	at d:\Source\Core\VideoBackends\D3D12\VideoBackend.cpp(46)
Dolphin.exe!VideoBackendBase::PopulateBackendInfo(const WindowSystemInfo & wsi) Line 307
	at d:\Source\Core\VideoCommon\VideoBackendBase.cpp(307)
Dolphin.exe!Core::Init(Core::System & system, std::unique_ptr<BootParameters,std::default_delete<BootParameters>> boot, const WindowSystemInfo & wsi) Line 267
	at d:\Source\Core\Core\Core.cpp(267)
Dolphin.exe!BootManager::BootCore(Core::System & system, std::unique_ptr<BootParameters,std::default_delete<BootParameters>> boot, const WindowSystemInfo & wsi) Line 183
	at d:\Source\Core\Core\BootManager.cpp(183)
Dolphin.exe!MainWindow::StartGame(std::unique_ptr<BootParameters,std::default_delete<BootParameters>> && parameters) Line 1142
	at d:\Source\Core\DolphinQt\MainWindow.cpp(1142)
Dolphin.exe!MainWindow::StartGame(const std::vector<std::string,std::allocator<std::string>> & paths, std::unique_ptr<BootSessionData,std::default_delete<BootSessionData>> boot_session_data) Line 1112
	at d:\Source\Core\DolphinQt\MainWindow.cpp(1112)
Dolphin.exe!MainWindow::ScanForSecondDiscAndStartGame(const UICommon::GameFile & game, std::unique_ptr<BootSessionData,std::default_delete<BootSessionData>> boot_session_data) Line 1083
	at d:\Source\Core\DolphinQt\MainWindow.cpp(1083)
Dolphin.exe!MainWindow::StartGame(const std::string & path, MainWindow::ScanForSecondDisc scan, std::unique_ptr<BootSessionData,std::default_delete<BootSessionData>> boot_session_data) Line 1100
	at d:\Source\Core\DolphinQt\MainWindow.cpp(1100)
Dolphin.exe!MainWindow::Play(const std::optional<std::string> & savestate_path) Line 854
	at d:\Source\Core\DolphinQt\MainWindow.cpp(854)
[Inline Frame] Dolphin.exe!MainWindow::ConnectGameList::__l2::<lambda_1>::operator()() Line 713
	at d:\Source\Core\DolphinQt\MainWindow.cpp(713)
[Inline Frame] Dolphin.exe!QtPrivate::FunctorCall<QtPrivate::IndexesList<>,QtPrivate::List<>,void,`MainWindow::ConnectGameList'::`2'::<lambda_1>>::call(MainWindow::ConnectGameList::__l2::<lambda_1> &) Line 127
	at d:\Externals\Qt\Qt6.5.1\x64\include\QtCore\qobjectdefs_impl.h(127)
[Inline Frame] Dolphin.exe!QtPrivate::Functor<`MainWindow::ConnectGameList'::`2'::<lambda_1>,0>::call(MainWindow::ConnectGameList::__l2::<lambda_1> &) Line 241
	at d:\Externals\Qt\Qt6.5.1\x64\include\QtCore\qobjectdefs_impl.h(241)
Dolphin.exe!QtPrivate::QFunctorSlotObject<`MainWindow::ConnectGameList'::`2'::<lambda_1>,0,QtPrivate::List<>,void>::impl(int which, QtPrivate::QSlotObjectBase * this_, QObject * r, void * * a, bool * ret) Line 409
	at d:\Externals\Qt\Qt6.5.1\x64\include\QtCore\qobjectdefs_impl.h(409)
[External Code]
Dolphin.exe!app_main(int argc, char * * argv) Line 293
	at d:\Source\Core\DolphinQt\Main.cpp(293)
Dolphin.exe!wWinMain(HINSTANCE__ * __formal, HINSTANCE__ * __formal, wchar_t * __formal, int __formal) Line 312
	at d:\Source\Core\DolphinQt\Main.cpp(312)
[External Code]

I'm going to rebase the PR and hopefully, it will fix the flatpak issue.

@dreamsyntax
Copy link
Member

dreamsyntax commented Dec 23, 2024

I managed to trigger a crash on Windows when loading Shadow The Hedgehog. However, I can't find a way to reproduce it consistently and I'm unsure that's related to the flatpak issue:

D3D12Core.dll!00007ffe691950ce()
....

Oh that crash is unrelated.

This game is unstable with DX12 backend. Use DX11 or Vulkan when testing any thing relating to this game. DX11 is most stable for Windows.

@dreamsyntax
Copy link
Member

dreamsyntax commented Dec 26, 2024

I'm going to rebase the PR and hopefully, it will fix the flatpak issue.

Bad news, your rebased PR still has the issue.
It is not reproducible on a random other PR's flatpak artifact - I confirmed using 13420 / feat(linux): allow configuring real wiimotes with known bluetooth addresses.
It also does not happen in the release artifact, so the issue is somewhere in this code.

As for why its exclusive to two+ GPU hosts and with flatpaks only, I have no idea.

Comment on lines 393 to 394
// Should never happen
Common::Unreachable();
Copy link
Member

Choose a reason for hiding this comment

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

Can you also throw in something that outputs to Dolphin logs, just to see if I am hitting this somehow in the flatpak?

@dreamsyntax
Copy link
Member

dreamsyntax commented Dec 28, 2024

I don't see any Unreachable/Bad columns in the logs, and nothing sticks out when the program crashes
logs-two-dgpus-flatpak-dolphin-commit-1abc4a4.zip

@sepalani sepalani changed the title PPCSymbolDB: Refactor SymbolMap Save/Load (old) PPCSymbolDB: Refactor SymbolMap Save/Load Jan 4, 2025
@sepalani
Copy link
Contributor Author

sepalani commented Jan 4, 2025

@dreamsyntax I moved the changed to the new PR and also addressed @mitaclaw's comment about the SplitString function: #13247

@sepalani sepalani closed this Jan 4, 2025
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.

2 participants