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.
lolbinarycat
force-pushed
the
extern-fn-reachable-rmake
branch
from
3d7de9f
to
8663bbe
Compare
This comment has been minimized.
This comment has been minimized.
|
@rustbot review |
rustbot
added
S-waiting-on-review
There was a problem hiding this comment.
Suggestion: could we add a brief comment on what this test is trying to check? I think it's trying to check that #[no_mangle] functions have symbols that are present in the dylib?
There was a problem hiding this comment.
Also, can you please squash the commits into one after adding a comment? There's some formatting commits.
|
@bors try |
…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?
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