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

PPCSymbolDB: Handle data symbols in SymbolMap properly #6101

Merged
merged 2 commits into from Oct 11, 2017

Conversation

3 participants
@sepalani
Contributor

sepalani commented Oct 4, 2017

There are some issues regarding data symbols and symbol maps.

The first one is that data symbols from games' symbol map aren't loaded. However, if they get loaded somehow, they'll get saved as function symbol if Dolphin saves them and load them later on which is the second issue. This issue is almost invisible when dealing with ELF files since the symbols are loaded from the ELF file itself unless you save the symbol map, clear them and reload the symbol map. I won't be surprised that symbols loaded from RSO files are impacted.

This PR loads symbols from all sections in a given map file. I compared the result with the one I get with the ELF games I have and ctors/dtors symbols are present in my ELF files. I also removed the skip when symbol names begin with ".init" and ".text" (ex: ".text JSystemV.a J3DUDL.cpp") because these symbols aren't meaningless and can be seen in the Memory View. Other symbols beginning with .sbss, .bss, .data, etc. aren't skipped, so it doesn't make much sense anyway.

Then, I fixed the data symbols save by adding a data section in the Dolphin generated map.

I tested symbol maps from the following games and they still load properly:

  • Mario Kart: Double Dash
  • MySims Wii
  • Super Mario Sunshine
  • Zelda: Twilight Princess

Regardless, I'm open to suggestions if you have any.

Ready to be reviewed & merged.

@lioncash

This comment has been minimized.

Show comment
Hide comment
@lioncash

lioncash Oct 4, 2017

Member

Your commit messages should probably include part(s) (or just the applicable parts) from your main PR explanation to put context in the commit history as well.

Member

lioncash commented Oct 4, 2017

Your commit messages should probably include part(s) (or just the applicable parts) from your main PR explanation to put context in the commit history as well.

@sepalani

This comment has been minimized.

Show comment
Hide comment
@sepalani

sepalani Oct 4, 2017

Contributor

@lioncash Is it better?

Contributor

sepalani commented Oct 4, 2017

@lioncash Is it better?

sepalani added some commits Oct 4, 2017

PPCSymbolDB: Load more SymbolMap symbols
Allows to load data symbols from symbol map files.

Symbols from all sections are loaded.

The data/function symbol type is set accordingly.
PPCSymbolDB: Save data symbols properly
Data symbols were previously saved as function symbols.
// 3] _stack_addr found as linker generated symbol
// ...
// 10] EXILock(func, global) found in exi.a EXIBios.c
if (StringEndsWith(temp, "]"))

This comment has been minimized.

@leoetlino

leoetlino Oct 11, 2017

Member

"EndsWith", rather than "contains"?

@leoetlino

leoetlino Oct 11, 2017

Member

"EndsWith", rather than "contains"?

This comment has been minimized.

@sepalani

sepalani Oct 11, 2017

Contributor

If I'm not mistaken the temp variable is the first word that doesn't contain any whitespace which is parsed with sscanf.

man (3) sscanf
%s
Matches a sequence of non-white-space characters; the next pointer must be a pointer to character array that is long enough to hold the input sequence and the terminating null byte ('\0'), which is added automatically. The input string stops at white space or at the maximum field width, whichever occurs first.

@sepalani

sepalani Oct 11, 2017

Contributor

If I'm not mistaken the temp variable is the first word that doesn't contain any whitespace which is parsed with sscanf.

man (3) sscanf
%s
Matches a sequence of non-white-space characters; the next pointer must be a pointer to character array that is long enough to hold the input sequence and the terminating null byte ('\0'), which is added automatically. The input string stops at white space or at the maximum field width, whichever occurs first.

This comment has been minimized.

@leoetlino

leoetlino Oct 11, 2017

Member

Ah, my bad, I had missed the sscanf call. LGTM then.

@leoetlino

leoetlino Oct 11, 2017

Member

Ah, my bad, I had missed the sscanf call. LGTM then.

@leoetlino leoetlino merged commit ceed4d6 into dolphin-emu:master Oct 11, 2017

10 checks passed

default Very basic checks passed, handed off to Buildbot.
Details
lint Build succeeded on builder lint
Details
pr-android Build succeeded on builder pr-android
Details
pr-deb-dbg-x64 Build succeeded on builder pr-deb-dbg-x64
Details
pr-deb-x64 Build succeeded on builder pr-deb-x64
Details
pr-freebsd-x64 Build succeeded on builder pr-freebsd-x64
Details
pr-osx-x64 Build succeeded on builder pr-osx-x64
Details
pr-ubu-x64 Build succeeded on builder pr-ubu-x64
Details
pr-win-dbg-x64 Build succeeded on builder pr-win-dbg-x64
Details
pr-win-x64 Build succeeded on builder pr-win-x64
Details

@sepalani sepalani deleted the sepalani:symbol-map-data branch Oct 11, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment