Skip to content

Implement proper symbol lookup in a collections of versions#972

Merged
marxin merged 6 commits into
wild-linker:mainfrom
marxin:proper-version-script-evaluation
Jul 4, 2025
Merged

Implement proper symbol lookup in a collections of versions#972
marxin merged 6 commits into
wild-linker:mainfrom
marxin:proper-version-script-evaluation

Conversation

@marxin

@marxin marxin commented Jun 30, 2025

Copy link
Copy Markdown
Collaborator

Apparently, the matching logic for symbols in versions in more complex than we've got:
https://maskray.me/blog/2020-11-26-all-about-symbol-versioning#version-script

Comment thread libwild/src/version_script.rs Outdated
Comment thread libwild/src/version_script.rs Outdated
Comment thread libwild/src/version_script.rs
Comment thread libwild/src/version_script.rs Outdated
Comment thread libwild/src/version_script.rs Outdated
@marxin marxin force-pushed the proper-version-script-evaluation branch from 1606e81 to f6c9960 Compare July 2, 2025 20:19
Comment thread libwild/src/version_script.rs
@marxin marxin force-pushed the proper-version-script-evaluation branch from 40c6171 to 0db7271 Compare July 3, 2025 04:29
@marxin marxin marked this pull request as ready for review July 3, 2025 16:54
@marxin

marxin commented Jul 3, 2025

Copy link
Copy Markdown
Collaborator Author

Thanks to the nice integration done by @lapla-cogito, I can say the PR fixes 10 linker script related tests in Mold.

@lapla-cogito

Copy link
Copy Markdown
Member

Then this would allow us to remove those tests from skip_tests.toml, right? (I'm still figuring out how best to maintain the list of skipped tests—I plan to gather feedback on this in Zulip later.)

@mati865

mati865 commented Jul 3, 2025

Copy link
Copy Markdown
Member

Sorry for being late to the party.
In overall, it looks quite good to me. How does it stand on performance side?

Not related to this PR, but maybe we should utf8-parse whole file at once with SIMD? For example, using https://github.com/rusticstuff/simdutf8
I imagine parsing short strings separately isn't the world's fastest thing.

@davidlattimore davidlattimore left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

From a performance perspective, what's most important to me is that we only pay costs if features are used. Validating that all symbols are valid UTF-8 in a single function call could be an interesting thing to experiment, however, we'd only really want to do that for links where we need UTF-8 validity, which I think is only when we have a version script with extern "C++". From some very limited performance testing that I just did, this PR fixes #978 - which makes sense, since we're now skipping demangling and UTF-8 validation unless we have match rules that need them. I'm happy for this to be merged, but will leave it to you.

That's great to have a few more tests passing and to have the builds that originally motivated this change working as well!

@mati865

mati865 commented Jul 4, 2025

Copy link
Copy Markdown
Member

There is also https://github.com/marxin/wild/blob/5d1b3303fc8500a82cea9be51cde7bb76c6116ee/libwild/src/version_script.rs#L474
So the only cases where UTF-8 is not parsed is when names are not mangled and are either an asterisk or plain text - adding globs requires parsing. Naturally, this would be a pessimisation for Rust case.

@davidlattimore

Copy link
Copy Markdown
Member

Good point, I forgot about the globbing. I'm somewhat tempted to make a create that matches glob patterns against bytes

@marxin marxin force-pushed the proper-version-script-evaluation branch from 1f26397 to e5c8987 Compare July 4, 2025 17:10
@marxin marxin merged commit 7e20e22 into wild-linker:main Jul 4, 2025
22 checks passed
@marxin marxin deleted the proper-version-script-evaluation branch July 4, 2025 17:14
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.

4 participants