port tests/run-make/extern-fn-reachable to rmake #128314
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @jieyouxu (or someone else) some time within the next two weeks. Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (
|
rustbot
added
A-run-make
|
The run-make-support library was changed cc @jieyouxu This PR modifies cc @jieyouxu |
This comment has been minimized.
This comment has been minimized.
lolbinarycat
force-pushed
the
extern-fn-reachable-rmake
branch
from
ee70647
to
e2335ba
Compare
|
☔ The latest upstream changes (presumably #128147) made this pull request unmergeable. Please resolve the merge conflicts. |
lolbinarycat
force-pushed
the
extern-fn-reachable-rmake
branch
from
e2335ba
to
594e4c0
Compare
This comment has been minimized.
This comment has been minimized.
|
☔ The latest upstream changes (presumably #127926) made this pull request unmergeable. Please resolve the merge conflicts. |
| @@ -193,6 +193,11 @@ impl Rustc { | |||
| self | |||
| } | |||
|
|
|||
| /// Make `rustc` prefere dynamic linking | |||
There was a problem hiding this comment.
Nit:
| /// Make `rustc` prefere dynamic linking | |
| /// Make `rustc` prefer dynamic linking. |
| /// The symbol names must match exactly. | ||
| /// | ||
| /// Panics if `path` is not a valid object file readable by the current user. | ||
| pub fn contains_exact_symbols(path: impl AsRef<Path>, symbol_names: &[&str]) -> bool { |
There was a problem hiding this comment.
Suggestion: having this return a bool but then also have output logging is weird. We probably want to have a read_symbols(path) return something like Vec<OsString> or Vec<Vec<u8>> (pretty sure symbols don't need to be UTF-8), and then have an assertion helper based on that:
fn assert_symbols_match_exactly(expected_symbols: &[&OsStr], found_symbols: &[&OsStr]) {
assert_eq!(expected_symbols, found_symbols, "symbols do not match exactly");
}or something more generic, like assert_byte_strings_are_equal.
(or some customized equality assertion to not show u8 slice debug repr)
There was a problem hiding this comment.
Problem: am I understanding the logic here is that for a given object file, and a given list of expected symbols, we want the object file to contain the symbol at least once? Or does the test actually want exact counts of occurrences of symbols? AFAICT, the logic here only checks that a given expected symbol occurs at least once, whereas the logic in the original Makefile checks that a given expected symbol occurs exactly once, so the logic is not equivalent from what I can tell.
There was a problem hiding this comment.
the previous makefile wasn't checking for exact symbol names though.
"accidentally generating duplicate symbols and somehow not encountering a link error" doesn't seem like a very probable failure mode, and if it is likely, then i would rather approach that with a helper that asserts a file does not contain any duplicate symbols, rather than checking the counts of specific symbols.
i should probably move the debug output into assertion helpers though, the current behavior is based off of the cat-and-grep script, but it's a bit confusing, and we can do better.
rustbot
added
S-waiting-on-author
| NM=nm -D | ||
|
|
||
| ifeq ($(UNAME),Darwin) | ||
| NM=nm -gU | ||
| endif | ||
|
|
||
| ifdef IS_WINDOWS | ||
| NM=nm -g | ||
| endif |
There was a problem hiding this comment.
Question: Hm,
-Dis "display the dynamic symbols rather than the normal symbols. This is only meaningful for dynamic objects, such as certain types of shared libraries"-gis "display only external symbols"-Uis "display only defined symbols for each object file. By default both defined and undefined symbols are displayed"
and these flags are passed differently based on platforms. Are we sure with_symbol_iter based on object preserves these behavior?
There was a problem hiding this comment.
the test is configured to never run on those platforms anyways.
There was a problem hiding this comment.
the test is configured to never run on those platforms anyways.
Hmm? That doesn't seem right, since the Makefile test is only ignore-cross-compile + ignore-windows-msvc, or am I misunderstanding something? This still leaves host linux, host apple and host windows mingw targets?
There was a problem hiding this comment.
It seems you're correct, I must've been thinking of a different rmake test.
Still, it seems strange that it would look for different kinds of symbols on different platforms, shouldn't the symbols have the same linkage regardless of platform?
There was a problem hiding this comment.
Still, it seems strange that it would look for different kinds of symbols on different platforms, shouldn't the symbols have the same linkage regardless of platform?
Yeah, this is exactly why I noticed it. I'll try to do a little digging from the original PR to see why it had to use different nm flags, this seems very cursed.
There was a problem hiding this comment.
I looked at the original PR alongside review comments, and unfortunately it was not enlightening as to the why.
nm flags:
-D: --dynamic
Display the dynamic symbols rather than the normal symbols. This is only meaningful for dynamic objects, such as certain types of shared libraries.
-g: --extern-only
Show only external symbols
-gU: --extern-only --defined-only
Display only defined symbols for each object file. By default both defined and undefined symbols are displayed.
I'm inclined to say ideally we want --dynamic + --extern-only + --defined-only consistently across the platforms (this probably fails, but I'd like to know on which platforms that fails). So maybe something based on Object::exports.
(This is going to take a couple of tries to get right I'm afraid.)
There was a problem hiding this comment.
there's the ObjectSymbol::is_* family of function, but i'm somewhat confused what it would mean to have a non-dynamic global symbol in a dynamic object.
jieyouxu
added
S-waiting-on-review
|
@rustbot author |
rustbot
added
S-waiting-on-author
uses helper functions added in rust-lang#128147, must not be merged before that PR.
…ke, r=<try> port tests/run-make/extern-fn-reachable to rmake uses helper functions added in rust-lang#128147, must not be merged before that PR. try-job: aarch64-apple try-job: armhf-gnu try-job: test-various try-job: x86_64-msvc try-job: x86_64-mingw try-job: i686-msvc try-job: i686-mingw try-job: x86_64-gnu-llvm-17
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
💔 Test failed - checks-actions |
bors
added
S-waiting-on-author
|
Symbol names on OSX are listed as |
do you mean a leading underscore? or do you mean its listed as |
|
Leading, sorry, it's a typo. |
|
@Oneirical any idea why it does that? is there some piece of documentation saying that's how it works? it seems odd that a no_mangle symbol would be subject to a form of mangling. |
@lolbinarycat based on https://developer.apple.com/forums/thread/715385
It might be a macos/apple linker convention that we follow? |
| assert_contains_exact_symbols("dylib.so", &["fun1", "fun2", "fun3", "fun4", "fun5"], |sym| { | ||
| dbg!(dbg!(sym).is_global()) && !dbg!(sym.is_undefined()) | ||
| }); |
There was a problem hiding this comment.
Remark: original Makefile only tested for contains not exact match, I think one of the reasons being the exact naming is different.
There was a problem hiding this comment.
hmm....that still feels pretty fragile to me, maybe we should just add an underscore on macOS
any idea what's going on with the symbol types?
There was a problem hiding this comment.
Can you clarify what you mean by "symbol types"?
There was a problem hiding this comment.
Suggestion: see above, macOS expects dylib symbols to be prefixed with _.
|
From triage. @lolbinarycat Do you need to address something from above or it's ready for review? |
|
@alex-semenyuk this is mostly blocked on "symbols are really weird on other platforms and i don't fully understand what's going on" |
alex-semenyuk
added
S-blocked
jieyouxu
added
S-waiting-on-review
|
I need to revisit this and ask the target maintainers and people who know about the symbol situation. |
|
Dear apple experts, do you know if macOS expects dylib symbol names with an underscore prefix? The closest thing I can find is developer.apple.com/forums/thread/715385 and the sentence "C, and all later languages, add a leading underscore (_) to distinguish their symbols from assembly language symbols." I'm asking as I don't know what is the intended behavior. @rustbot ping apple |
rustbot
added
the
O-apple
|
Hey Apple notification group! This issue or PR could use some Apple-specific (In case it's useful, here are some instructions for tackling these sorts of cc @BlackHoleFox @hkratz @inflation @madsmtm @nvzqz @shepmaster @thomcc |
Yes. dyld expects anything beyond assembly procedures starts with an underscore prefix. |
Thank you, that's what I expected but I couldn't find relevant "official" docs. Do you know of any official docs or references that describes this? I couldn't find anything in docs for dyld (incl. its man pages) that describes this. The closest I got is https://opensource.apple.com/source/dyld/dyld-132.13/src/dyldAPIs.cpp.auto.html and which is almost like "source: I made it up" lol. |
| /// | ||
| /// The symbols must also match `pred`. | ||
| /// | ||
| /// The symbol names must match exactly. |
There was a problem hiding this comment.
Suggestion: apparently dyld expects dylib symbol names to be prefixed with _, so we should describe this difference.
| @@ -193,6 +193,11 @@ impl Rustc { | |||
| self | |||
| } | |||
|
|
|||
| /// Make `rustc` prefere dynamic linking | |||
|
|
||
| /// Get a list of symbols that are in `symbol_names` but not the final binary. | ||
| /// | ||
| /// The symbols must also match `pred`. |
There was a problem hiding this comment.
Nit: match pred -> satisfies pred.
| .collect(); | ||
| } | ||
|
|
||
| /// Assert that the symbol file contains all of the listed symbols and they all match the given predicate |
There was a problem hiding this comment.
Suggestion: ditto above, we should describe on macOS we expect _ prefix in addition to the non-underscore-prefixed symbol name.
| assert_contains_exact_symbols("dylib.so", &["fun1", "fun2", "fun3", "fun4", "fun5"], |sym| { | ||
| dbg!(dbg!(sym).is_global()) && !dbg!(sym.is_undefined()) | ||
| }); |
There was a problem hiding this comment.
Can you clarify what you mean by "symbol types"?
| assert_contains_exact_symbols("dylib.so", &["fun1", "fun2", "fun3", "fun4", "fun5"], |sym| { | ||
| dbg!(dbg!(sym).is_global()) && !dbg!(sym.is_undefined()) | ||
| }); |
There was a problem hiding this comment.
Suggestion: see above, macOS expects dylib symbols to be prefixed with _.
rustbot
added
S-waiting-on-author
|
@bjorn3 said:
|
|
On macOS we should expect that the dylib symbol begins with an underscore @rustbot author |
|
☔ The latest upstream changes (presumably #131387) made this pull request unmergeable. Please resolve the merge conflicts. |
uses helper functions added in #128147, must not be merged before that PR.
try-job: aarch64-apple
try-job: armhf-gnu
try-job: test-various
try-job: x86_64-msvc
try-job: x86_64-mingw
try-job: i686-msvc
try-job: i686-mingw
try-job: x86_64-gnu-llvm-17