-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
port tests/run-make/extern-fn-reachable to rmake #128314
base: master
Are you sure you want to change the base?
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 (
|
The run-make-support library was changed cc @jieyouxu This PR modifies cc @jieyouxu |
This comment has been minimized.
This comment has been minimized.
ee70647
to
e2335ba
Compare
☔ The latest upstream changes (presumably #128147) made this pull request unmergeable. Please resolve the merge conflicts. |
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the PR. I think I noticed a discrepancy between the symbol checking logic between the original Makefile and the rmake.rs version.
@@ -193,6 +193,11 @@ impl Rustc { | |||
self | |||
} | |||
|
|||
/// Make `rustc` prefere dynamic linking |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit:
/// Make `rustc` prefere dynamic linking | |
/// Make `rustc` prefer dynamic linking. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ping
/// 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
NM=nm -D | ||
|
||
ifeq ($(UNAME),Darwin) | ||
NM=nm -gU | ||
endif | ||
|
||
ifdef IS_WINDOWS | ||
NM=nm -g | ||
endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: Hm,
-D
is "display the dynamic symbols rather than the normal symbols. This is only meaningful for dynamic objects, such as certain types of shared libraries"-g
is "display only external symbols"-U
is "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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the test is configured to never run on those platforms anyways.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@rustbot author |
uses helper functions added in rust-lang#128147, must not be merged before that PR.
The job Click to see the possible cause of the failure (guessed by this bot)
|
💔 Test failed - checks-actions |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you clarify what you mean by "symbol types"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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" |
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 |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ping
|
||
/// 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: ditto above, we should describe on macOS we expect _
prefix in addition to the non-underscore-prefixed symbol name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ping
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: see above, macOS expects dylib symbols to be prefixed with _
.
@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. |
ping from triage - can you post your status on this PR? This PR has not received an update in a few months. |
Well, first that was because it was blocked on figuring out what macos is up to, now it's just because i've been busy with other things. I'll try and finish this up, but if anyone else has more time on their hands and wants to take ownership of this PR, let me know. |
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