Skip to content

viking: Check metadata of symbols in .dynsym#42

Open
MonsterDruide1 wants to merge 3 commits into
open-ead:masterfrom
MonsterDruide1:upstream-check-symbol-metadata
Open

viking: Check metadata of symbols in .dynsym#42
MonsterDruide1 wants to merge 3 commits into
open-ead:masterfrom
MonsterDruide1:upstream-check-symbol-metadata

Conversation

@MonsterDruide1

@MonsterDruide1 MonsterDruide1 commented May 20, 2026

Copy link
Copy Markdown
Contributor

requires #38 to be merged first.

In this PR, a new check is added that ensures that symbols listed in the .dynsym section of the executable match in metadata between the original and decompiled binary.

This includes the defined symbol size, info (type and binding) and other (visibility) fields included in the ELF. Any mismatch is reported similar to mismatches in function contents, requesting the user to adjust their code to fix attributes, visibility and similar.

We iterate over all symbols in the original elf, then try looking up the corresponding symbol in the decomp elf. This also means that additional symbols in the decomp elf are ignored (introduced/kept due to -g or more exports), and ensures that games without symbols do not show errors here either (no symbols = no mismatches possible).


This change is Reviewable

@MonsterDruide1 MonsterDruide1 self-assigned this May 20, 2026
@LynxDev2

LynxDev2 commented May 24, 2026

Copy link
Copy Markdown
Contributor

IMO these symbol checks should be under an opt-in config flag that defaults to not checking them. This is so that for projects where the original ELF doesn't have proper symbols, the stuff in that ELF doesn't need to be unnecessarily parsed. Even for projects that do have symbols (like SMO), this would also cause updating viking for them to cause check errors in CI before the errors are fixed for every library used.

I also feel like check_symbol and the symbol mismatch struct should go into src/check.rs (while check_symbols can stay in src/tools/check.rs), but that's just a styling nit

@LynxDev2

LynxDev2 commented May 24, 2026

Copy link
Copy Markdown
Contributor

If this is put under an opt-in flag that would basically make it so that no new code is ran for botw, so I think having this reviewed just by me (when this becomes ready for review) would be good enough in that case, otherwise I think we should wait for leo to also review this

@MonsterDruide1 MonsterDruide1 force-pushed the upstream-check-symbol-metadata branch from e05b938 to a7cc335 Compare May 24, 2026 11:45
@MonsterDruide1 MonsterDruide1 marked this pull request as ready for review May 24, 2026 11:46
@MonsterDruide1

Copy link
Copy Markdown
Contributor Author

Adjusted according to both comments, although I don't agree to the reasoning on the config file.

If you check the diff again (now that #38 has been merged), you'll notice that it doesn't read any additional data from the elf which blocks other stuff. There's more stuff in the rayon::scope, but as these are executed in parallel, it doesn't really matter for performance, so I'm also not putting this part behind a config flag.

For other games such as BotW, this code will just have no effect, as the original binary provides no symbols at all, so there is no base truth to compare to (for (name, symbol) in orig_dynsym.iter() is empty). It is indeed a very decoupled check, so in case disabling that is desired for whatever reason, it's still possible now.

@MonsterDruide1 MonsterDruide1 force-pushed the upstream-check-symbol-metadata branch from 060b4fb to c07be50 Compare June 19, 2026 09:15

@LynxDev2 LynxDev2 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@LynxDev2 partially reviewed 5 files and made 1 comment.
Reviewable status: 1 of 5 files reviewed, 1 unresolved discussion (waiting on MonsterDruide1).


viking/src/checks.rs line 591 at r3 (raw file):

            "incorrect {name}; expected to see {orig}\n\
            --> decomp contains {decomp} instead",
            name = name,

Nit: Setting these equal to the same name is unnecessary. I guess this might be to match the styling of the other mismatch write!s, but those ones set them to longer experssions.

@LynxDev2 LynxDev2 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@LynxDev2 reviewed 4 files and all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on MonsterDruide1).

@LynxDev2 LynxDev2 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@LynxDev2 made 1 comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on MonsterDruide1).


a discussion (no related file):
I tested this by combining it with my file-list-tools branch and everything seems to be working fine.

The main issue though is that atm, there's no way to make it not complain about a symbol mismatch without fixing that mismatch (other than disabling the symbol checking entirely). There are some edge-cases where it can be pretty challenging to for example get a symbol's binding to match without hacky workarounds. I think it should at least only complain about function symbols when the specific function is marked as matching, but I'm not sure what we should do with data symbols.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants