rust (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation56 Commits4 Checks6 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

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

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 (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

@rustbot author: the review is finished, PR author should check the comments and take action accordingly
@rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

rustbot added A-run-make

Area: port run-make Makefiles to rmake.rs

A-testsuite

Area: The testsuite used to check the correctness of rustc

S-waiting-on-review

Status: Awaiting review from the assignee but also interested parties.

T-bootstrap

Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)

labels

Jul 28, 2024

The run-make-support library was changed

cc @jieyouxu

This PR modifies tests/run-make/. If this PR is trying to port a Makefile
run-make test to use rmake.rs, please update the
run-make port tracking issue
so we can track our progress. You can either modify the tracking issue
directly, or you can comment on the tracking issue and link this PR.

cc @jieyouxu

This comment has been minimized.

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

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.

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, symbol_names: &[&str]) -> bool {

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)

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.

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.

rustbot added S-waiting-on-author

Status: This is awaiting some action (such as code changes or more information) from the author.

and removed S-waiting-on-review

Status: Awaiting review from the assignee but also interested parties.

labels

Aug 3, 2024

Comment on lines -6 to -14

NM=nm -D

ifeq ($(UNAME),Darwin)
NM=nm -gU
endif

ifdef IS_WINDOWS
NM=nm -g
endif

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?

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.

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?

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?

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.

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.)

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.

jieyouxu added S-waiting-on-review

Status: Awaiting review from the assignee but also interested parties.

and removed S-waiting-on-author

Status: This is awaiting some action (such as code changes or more information) from the author.

labels

Aug 6, 2024

rustbot added S-waiting-on-author

Status: This is awaiting some action (such as code changes or more information) from the author.

and removed S-waiting-on-review

Status: Awaiting review from the assignee but also interested parties.

labels

Aug 6, 2024

uses helper functions added in rust-lang#128147, must not be merged before that PR.

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

Status: Blocked on something else such as an RFC or other implementation work.

and removed S-waiting-on-author

Status: This is awaiting some action (such as code changes or more information) from the author.

labels

Sep 22, 2024

jieyouxu added S-waiting-on-review

Status: Awaiting review from the assignee but also interested parties.

and removed S-blocked

Status: Blocked on something else such as an RFC or other implementation work.

labels

Sep 22, 2024

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

Operating system: Apple (macOS, iOS, tvOS, visionOS, watchOS)

label

Sep 25, 2024

Dear apple experts, do you know if macOS expects dylib symbol names with an underscore prefix?

Yes. dyld expects anything beyond assembly procedures starts with an underscore prefix.

Dear apple experts, do you know if macOS expects dylib symbol names with an underscore prefix?

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

void* dlsym(void* handle, const char* symbolName)
{
    if ( dyld::gLogAPIs )
        dyld::log("%s(%p, %s)\n", __func__, handle, symbolName);

    dlerrorClear();

    const ImageLoader* image;
    const ImageLoader::Symbol* sym;

    // dlsym() assumes symbolName passed in is same as in C source code
    // dyld assumes all symbol names have an underscore prefix

which is almost like "source: I made it up" lol.

///
/// The symbols must also match `pred`.
///
/// The symbol names must match exactly.

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

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`.

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

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.

Choose a reason for hiding this comment

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

Ping

Comment on lines +7 to +9

| assert_contains_exact_symbols("dylib.so", &["fun1", "fun2", "fun3", "fun4", "fun5"], |sym| { | | -------------------------------------------------------------------------------------------------- | | dbg!(dbg!(sym).is_global()) && !dbg!(sym.is_undefined()) | | }); |

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"?

Comment on lines +7 to +9

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 _.

rustbot added S-waiting-on-author

Status: This is awaiting some action (such as code changes or more information) from the author.

and removed S-waiting-on-review

Status: Awaiting review from the assignee but also interested parties.

labels

Sep 25, 2024

@bjorn3 said:

Mach-O states that all exported symbols should have an underscore as prefix. At the same time dlsym will implicitly add it, so outside of compilers, linkers and people writing assembly, nobody needs to be aware of this.

On macOS we should expect that the dylib symbol begins with an underscore _.

@rustbot author

@lolbinarycat

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.

@lolbinarycat do you mind if I take over this PR and #128567? I'd like to get rid of the remaining Makefiles hopefully this Month.

@jieyouxu sure, go ahead, i've mostly been focusing on rustdoc and my own projects.

jieyouxu added S-waiting-on-review

Status: Awaiting review from the assignee but also interested parties.

and removed S-waiting-on-author

Status: This is awaiting some action (such as code changes or more information) from the author.

labels

Jan 13, 2025

bors added a commit to rust-lang-ci/rust that referenced this pull request

Jan 13, 2025

…, r=

tests: Port extern-fn-reachable to rmake.rs

Part of rust-lang#121876.

Summary

This PR ports tests/run-make/extern-fn-reachable to use rmake.rs. Notable changes:

We now use the object crate and look at the exported symbols specifically.
This test's coverage regressed against windows-msvc back in replace dynamic library module with libloading rust-lang#90716, but since we use object now, we're able to claw the test coverage back.
The checks are now stricter:
1. It no longer looks for substring symbol matches in nm textual outputs, it inspects the symbol names precisely.
2. We now also explicitly check for the presence of leading underscore in exported symbol names on apple vs non-apple targets.

History

Test was initially introduced as a run-pass[^run-pass] test as part of Don't mark reachable extern fns as internal rust-lang#10539.
Test re-introduced as a run-make test in rust-lang#13741.
Later, the test coverage regressed in rust-lang#90716.

[^run-pass]: no longer a thing nowadays

Supersedes rust-lang#128314. Co-authored with @lolbinarycat.

r? @ghost

try-job: x86_64-msvc try-job: i686-msvc try-job: i686-mingw try-job: x86_64-mingw-1 try-job: x86_64-apple-1 try-job: aarch64-apple try-job: test-various

Closing in favor of #135458, thanks for the PR!

bors added a commit to rust-lang-ci/rust that referenced this pull request

Jan 14, 2025

…, r=

tests: Port extern-fn-reachable to rmake.rs

Part of rust-lang#121876.

Summary

This PR ports tests/run-make/extern-fn-reachable to use rmake.rs. Notable changes:

We now use the object crate and look at the exported symbols specifically.
This test's coverage regressed against windows-msvc back in replace dynamic library module with libloading rust-lang#90716, but since we use object now, we're able to claw the test coverage back.
The checks are now stricter:
1. It no longer looks for substring symbol matches in nm textual outputs, it inspects the symbol names precisely.
2. We now also explicitly check for the presence of leading underscore in exported symbol names on apple vs non-apple targets.
Added another case of #[no_mangle] fn fun6() {} (note the lack of pub) to check that Rust nameres visibility is orthogonal to symbol visiblity in dylib.

History

Test was initially introduced as a run-pass[^run-pass] test as part of Don't mark reachable extern fns as internal rust-lang#10539.
Test re-introduced as a run-make test in rust-lang#13741.
Later, the test coverage regressed in rust-lang#90716.

[^run-pass]: no longer a thing nowadays

Supersedes rust-lang#128314. Co-authored with @lolbinarycat.

r? @ghost

try-job: x86_64-msvc try-job: i686-msvc try-job: i686-mingw try-job: x86_64-mingw-1 try-job: x86_64-apple-1 try-job: aarch64-apple try-job: test-various

bors added a commit to rust-lang-ci/rust that referenced this pull request

Jan 16, 2025

…, r=lqd

tests: Port extern-fn-reachable to rmake.rs

Part of rust-lang#121876.

Summary

This PR ports tests/run-make/extern-fn-reachable to use rmake.rs. Notable changes:

We now use the object crate and look at the exported symbols specifically.
This test's coverage regressed against windows-msvc back in replace dynamic library module with libloading rust-lang#90716, but since we use object now, we're able to claw the test coverage back.
The checks are now stricter:
1. It no longer looks for substring symbol matches in nm textual outputs, it inspects the symbol names precisely.
2. We now also explicitly check for the presence of leading underscore in exported symbol names on apple vs non-apple targets.
Added another case of #[no_mangle] fn fun6() {} (note the lack of pub) to check that Rust nameres visibility is orthogonal to symbol visibility in dylib.

History

Test was initially introduced as a run-pass[^run-pass] test as part of Don't mark reachable extern fns as internal rust-lang#10539.
Test re-introduced as a run-make test in rust-lang#13741.
Later, the test coverage regressed in rust-lang#90716.

[^run-pass]: no longer a thing nowadays

Supersedes rust-lang#128314. Co-authored with @lolbinarycat.

try-job: x86_64-msvc try-job: i686-msvc try-job: i686-mingw try-job: x86_64-mingw-1 try-job: x86_64-apple-1 try-job: aarch64-apple try-job: test-various

Labels

A-run-make

Area: port run-make Makefiles to rmake.rs

A-testsuite

Area: The testsuite used to check the correctness of rustc

O-apple

Operating system: Apple (macOS, iOS, tvOS, visionOS, watchOS)

S-waiting-on-review

Status: Awaiting review from the assignee but also interested parties.

T-bootstrap

Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)