Symbol and Code Map Improvements + Thread Safety

[Minty-Meeo]

Updated naming scheme of the struct, 'Common::Symbol'.

Added new members to the struct, 'Common::Symbol'

• m_starting_address
• m_file_offset
• m_alignment

Tweaked PPCSymbolDB::SaveSymbolMap to report the new Common::Symbol members instead of filling the starting address and alignment with nonsense.

Added new helper functions to safely run thread-sensitive functions that read from emulated RAM. These helper functions will abort and return false if MSR.DR or MSR.IR are in the wrong state.

• bool PPCAnalyst::HostFindFunctions(u32 startAddr, u32 endAddr, PPCSymbolDB *func_db)
• bool PPCSymbolDB::HostLoadMap(File::IOFile &f, bool bad)
• bool PPCSymbolDB::HostSaveCodeMap(File::IOFile& f) const

PPCAnalyst::FindFunctions now indexes symbols so they are colored correctly in the disassembly view.

Reworked PPCSymbolDB::LoadMap. It was an atrocity when I stumbled upon it, so I fixed up what I could.

• Added tons and tons of comments to explain what is going on and attempt to document CodeWarrior Linker Maps.
• Reordered operations to hopefully speed things up (and for personal preference).
• Symbol Map reading aborts upon encountering Memory Maps or Linker Generated Symbols at the end of the file. Previously, this part of the Symbol Map could sometimes produce garbage symbols.
• Instead of discarding symbols when the current section is unknown, it now changes the current section to .text by default.
• Replaced fundamentally wrong Entry Symbol detection checks with ones that are guaranteed to work.
• Removed code which renamed Entry Symbols based on their parent's name. It produced things like "OSExceptionVector::__OSEVStart". While it was neat, it rarely actually worked right. The code which extracts the parent's name from the line has been abridged and left in.
• Fill Symbols are being tallied, though nothing is done with them at this moment.
• Closure Symbols are now formally being discarded. Previously, this would almost always occur anyway; since they usually have the same virtual address as the following symbol, PowerPC::AddKnownSymbol would overwrite them. Either way, Closure Symbols are problematic currently because their calculated size includes those of Unused Symbols. In the future, it would be more useful if Closure Symbols were included as something akin to Symbol Databases, but only for the symbols it envelopes. I left a TODO in Common::Symbol about this.
• Entry Symbols are being discarded until a better solution comes along. Being 0 bytes large and having virtual addresses in the middle of or at the start of their parent symbol makes them useless and even harmful to code highlighting. Entry Symbols from actual inline ASM are pretty uncommon, anyways. Most are compiler-generated, redundant nonsense. In the future, storing Entry Symbol as children to their parent symbol is what really needs to be done.
• Fixed perfectly fine data symbols with address misalignment being discarded due to misuse of PowerPC::HostIsInstructionRAMAddress instead of PowerPC::HostIsRAMAddress.
• Bad map checks are now much more sophisticated. See code comments for specifics.
  • VerifyVTable and VerifyRTTI helper functions added for basic data symbol checks.
• Symbol tallying has been expanded beyond just "good_count" and "bad_count". A report is given at the end of the function, now including specifics on...
  • Added symbols
  • Updated symbols
  • Discarded symbols
    • Unused Symbols
    • Closure Symbols
    • Entry Symbols
    • Symbols with an invalid virtual address
    • Symbols with no name
    • Symbols discarded by bad map checks
• The names of symbols discarded by bad map checks are recorded and may optionally be written to the log.
• Added 'HLE::Clear()' and 'HLE::PatchFunctions()' to the end of PPCSymbolDB::LoadMap since everything calling it seemed to be doing that anyways.

Rewrote PPCSymbolDB::AddKnownSymbol.

• It now accepts parameters for starting address, file offset, and alignment.
• Un-duplicated some code to make the function easier to read.
• The function is now a bool instead of void. It returns true if an existing symbol was updated, and false if a new symbol was added.

These functions have had their signatures changed:

• bool PPCSymbolDB::LoadMap(const std::string& filename, bool bad) ---> void PPCSymbolDB::LoadMap(File::IOFile& f, bool bad)
• bool PPCSymbolDB::SaveSymbolMap(const std::string& filename) const ---> void PPCSymbolDB::SaveSymbolMap(File::IOFile& f) const
• bool PPCSymbolDB::SaveCodeMap(const std::string& filename) const ---> void PPCSymbolDB::SaveCodeMap(File::IOFile& f) const
• void MenuBar::LoadSymbolMap() ---> void MenuBar::LoadDefaultSymbolMap()
• void MenuBar::SaveSymbolMap() ---> void MenuBar::SaveDefaultSymbolMap()
• void MenuBar::SaveSymbolMapAs() ---> void MenuBar::SaveOtherSymbolMap()
• bool MenuBar::TryLoadMapFile(const QString& path, const bool bad) ---> void MenuBar::TryLoadMapFile(const QString& path, const bool bad)

Split MenuBar::SaveCode into two new functions:

• MenuBar::SaveDefaultCodeMap()
• MenuBar::TrySaveCodeMap()

Added new MenuBar::SaveOtherCodeMap function to save codemaps to arbitrary filepaths.

Added ModalMessageBox in the following functions for when they are aborted due to virtual address translation being disabled.

• void MenuBar::GenerateSymbolsFromAddress
• void MenuBar::GenerateSymbolsFromSignatureDB
• void MenuBar::TryLoadMapFile(const QString& path, const bool bad)
• void MenuBar::TrySaveCodeMap(const QString& path);

MenuBar::LoadDefaultSymbolMap no longer falls back on behavior identical to that of MenuBar::GenerateSymbolsFromSignatureDB when the default symbol map is not found.

MenuBar text and layout has been slightly altered.

• "Save Code Map As..." Action added to MenuBar.

[degasus]

This flag disables the address lookup, so it must stay where it is. We emulate a CPU with vitrual and physical addresses. Please update the symbol map in a way that it is able to handle those states. Through virtual (DS=1) is by far the more common one, it is also the tricky one. It changes quite often in some games (the remapping to physical addresses).

[AdmiralCurtiss]

Yeah this is an issue I've run across several times before in various contexts as well, it's a complicated one though with no easy solution. I think the cleanest I've come up with is letting users of HostRead_* and HostWrite_* specify whether they want hardware address or MMU'd address and then returning std::nullopt or false or something when that request mismatches the current emulated hardware state and handing that appropriately in the calling code, but I've never been happy with any actual implementation I ended up with for it...

[merryhime]

One cannot assume in general that translation lookup is set-up correctly when MSR[DR]=0.

[Minty-Meeo]

Can I be directed to games that would have issues with how I've changed this? The games I've used to test this never do.

[Minty-Meeo]

I gave a lot of thought to this today and realized there is one question I never bothered to ask. Why are we reading instructions from emulated memory for hash checks? With some effort, I'm willing to bet the hash check could gather its instructions from the DOL / ELF instead, permitting usage of the Physical Address of symbols and eliminating dependency on the state of the emulated CPU.

[sepalani]

To answer your question, (unless I'm mistaken and missed something really obvious), we are reading instruction from memory (RAM) because that's how it works. You usually don't execute instruction directly from disc/disk (or whatever the physical support is) because it's very slow. You fetch the instructions first in memory, then do the necessary steps to execute it.

Plus, checking directly the signature hash from DOL/ELF is wrong as it won't take into account relocated functions such as RSO/REL modules and won't work for programs rewriting themselves.

IMHO, the only decent way to address your issue would be to pause the emulation first, handle the symbol map processing and resume the emulation. It doesn't make sense to handle symbols in a dynamic context when they can be wrong or not there anymore while analysing them because the game is running.

[Minty-Meeo]

Pausing emulation to handle symbol map processing was literally the first idea I proposed in the IRC, but it was dismissed as a hack and I was sent on a goose chase to somehow fix translating virtual addresses while emulation is still running. Bruh.

Okay, how about this for a solution everyone can be happy with:

1. If emulation is running, the "Load Symbol Map" menu option will be grayed out.
2. If the MSR.DR field is in the wrong state, the LoadMap function will produce an appropriate error message box and abort loading the given Symbol Map.

[Minty-Meeo]

Unrelated, I've grown kinda fond of the changes I've made in MMU.cpp. Would they be worth keeping in a seperate PR for future usage, or do they miss the point of the HostRead/HostWrite functions?

[sepalani]

@Minty-Meeo Pretty sure this hack was used a lot at some point, especially in the debugger. IIRC, some parts where it was used were replaced with Core::RunAsCPUThread. Does this function address your issue when loading a map?

Your solution 1. is fine by me. I'm not fond of 2.

Regarding your MMU changes I don't mind as long as it doesn't break anything. Probably a duplicate of #8682 as @AdmiralCurtiss mentioned. I don't know which approach is better, though.

[AdmiralCurtiss]

As has been pointed out, AlwaysTranslate is just a bad idea because you can't be certain the MMU is setup correctly when the DR/IR flags are disabled. I'd prefer an approach where you let all the HostRead functions return an std::optional, give an enum of PhysicalAddress or VirtualAddress as parameter, and then in the combination of VirtualAddress but MMU disabled return std::nullopt (Physical + MMU enabled is fine); or something along those lines anyway.

[Minty-Meeo]

This PR has been three completely different things. To wrap it up, here is the final description:

While loading a Symbol Map, the emulated CPU's Machine State Register's Data Address Translation (DR) field can be set to 0 by the emulated software. This causes:

1. 'PPCSymbolDB::LoadMap' to believe a symbol's Virtual Address is not in valid emulated memory, discarding it.
2. If the former does not fail, 'PPCSymbolDB::AddKnownSymbol' will try to update the symbol's information, including a checksum. This produces a number of "Unable to resolve read address" PanicAlerts as instructions are read one at a time from emulated memory.

This issue is resolved with two fixes.

1. 'Core::RunAsCPUThread' is now used liberally in code where loading a symbol map is done. This ensures the emulated software pauses while the Symbol Map is being loaded.
2. In 'PPCSymbolDB::LoadMap, the state of the MSR.DR field is checked. If (MSR.DR == 0), the Symbol Map loading will abort. This is necessary, for as unlikely as it is, emulation could be paused whilst the MSR.DR field is set to 0 by the emulated software.

[leoetlino]

Should we add a // i18n comment to explain how this should be translated? I feel like technical terms like MSR are going to be completely translated and possibly end up incomprehensible.

I would also "Failed to load symbol map." or something like that at the beginning because this is supposed to be an error message.

[Minty-Meeo]

The functions that call this throw that very error message when false is returned.

[Minty-Meeo]

Or would you rather the error message be moved inside LoadMap?

[Minty-Meeo]

nvm that would not work for the usage in MIOS.cpp

[leoetlino]

In this case, one of the alerts should be removed because having two panic alerts in a row doesn't make for a very good UX. It's also inconsistent with the other failure paths and adds yet another panic alert into Core code.

I'd get rid of the PanicAlert here and move it into the UI (MenuBar::TryLoadMapFile) -- ModalMessageBox has the additional advantage of not being suppressed even if panic handlers are disabled. This means you would have to return some kind of error code to show the actual failure reason though.

For the other usages of this function, a failure isn't really critical.

[Minty-Meeo]

Is there ever a case where the MSR.DR field will be 0 when booting a game?

[leoetlino]

After emulated BS2, no. Unless the apploader sets MSR.DR to 0, but I doubt any official title would do that.

[Minty-Meeo]

Your suggestions have brought all changes down to just MenuBar.cpp. I like that :).

[leoetlino]

IMO this check for DR should still be in LoadMap. Accessing MSR directly in UI code is kind of... ugly? Make LoadMap return an enum so you can detect if loading has failed because of translation being disabled if you want to show a different error message.

Also, this check (written as is) introduces another race condition.

[Minty-Meeo]

Making an enum for just that seems uglier and less clear.
How could there be a race condition if MSR.DR never changes? All three functions calling it that were experiencing race conditions are now using Core::RunAsCPUThread.

[leoetlino]

How is it less clear if the check looks like if (error == LoadMapError::TranslationDisabled)? I'd argue that's actually clearer than checking for the relevant bit in MSR directly and it doesn't expose things that callers shouldn't have to know about.

Another reason for putting the check for MSR.DR is that it's still needed in case LoadMap is called when translation is disabled. This is unlikely to happen during a game boot but it's possible so I think it makes more sense to check it there.

I don't feel that strongly about this though, so maybe let's wait for another opinion.

How could there be a race condition if MSR.DR never changes? All three functions calling it that were experiencing race conditions are now using Core::RunAsCPUThread.

Oh right, this is called from the CPU thread. Forget what I said about the race then.

[Minty-Meeo]

I have some ideas to further improve Symbol Map loading in Dolphin that I want to try wrapping into this PR. I'm marking this as a draft for now.

[Minty-Meeo]

This PR has grown really big and features a lot more than the title suggests. I have cut down my ideas for improvement to what I believe to be reasonable without seriously overhauling the SymbolDB and its derivatives.

[Rumi-Larry]

I suggest that you rewrite the description of this pr to match what it is now, and specify what the next steps should be once it gets merged.

[Minty-Meeo]

I suggest that you rewrite the description of this pr to match what it is now, and specify what the next steps should be once it gets merged.

Done.

[iwubcode]

In places where applicable, Core::RunAsCPUThread has replaced Core::SetState workarounds to pause and resume emulation for thread-sensitive operations.
void Core::SaveScreenShot()
void Core::SaveScreenShot(std::string_view name)
void JitInterface::GetProfileResults(Profiler::ProfileStats *prof_stats)
void MainWindow::OnExportRecording()

I haven't looked at the code yet but these seem unrelated to the other logic. Maybe it could go in a new PR to be reviewed / merged more quickly?

[Minty-Meeo]

Maybe it could go in a new PR to be reviewed / merged more quickly?

Done: #9596

[sepalani]

I'm not fond of these CW specific overloads to the current multi-purpose map loader which supports not only CW symbol map but also other formats. If you really want to fully support CW map, I strongly suggest you that you write a proper class that loads the link map, section layout and memory map separately. You won't loose information that way and you'll be able to insert them appropriately into the Dolphin existing classes.

As suggested, you really should split this PR into several ones, at the very least: one for the PPCAnalyst improvement, another one for the symbol map modification, one for the style/name change. At its current's state this PR adds more complexity to a currently not clean/complex code and doesn't provide much benefit, IMHO.

[sepalani]

The m_ prefix should be used on class members not struct. Moreover, I'm strongly opposed to the modification of this struct if there isn't any real benefit from it such as adding fields like file_offset or virtual_address. These are CodeWarriors specific, you're also missing the object file and the linked/library file for said CW symbol.

[Minty-Meeo]

I was not aware of the distinction between classes and structs before. I reverted the name changes.
I disagree about the CW specific stuff being bad, but I don't have to agree.

I just have to get this PR accepted.

[sepalani]

start_addr / end_addr

[Minty-Meeo]

Done.

[sepalani]

These additional parameters make the function overly complicated, IMHO. Some of them are also used with a hardcoded 0 parameter and doesn't make more sense.

[sepalani]

These should probably be based on the section name and not hardcoded.

[Minty-Meeo]

Done. This will adversely affect any symbol maps that were loaded into Dolphin and then resaved (section names won't match anymore), but honestly those symbol maps aren't worth worrying about anyways.

Edit: Actually, what am I saying. 99% of closure symbols were being erased before anyhow.

[sepalani]

These shouldn't be hardcoded and can probably be merged with the other check bellow using a function.

[Minty-Meeo]

Done. It is now being done with lambdas.

[leoetlino]

This PR is too monolithic especially given the size of the diff. As @sepalani said, please split it into several PRs or several commits at the very least.