Skip to content
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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions src/tools/run-make-support/src/external_deps/rustc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -194,6 +194,11 @@ impl Rustc {
self
}

/// Make `rustc` prefere dynamic linking
Copy link
Member

Choose a reason for hiding this comment

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

Nit:

Suggested change
/// Make `rustc` prefere dynamic linking
/// Make `rustc` prefer dynamic linking.

Copy link
Member

Choose a reason for hiding this comment

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

Ping

pub fn prefer_dynamic(&mut self) -> &mut Self {
self.arg("-Cprefer-dynamic")
}

/// Specify directory path used for profile generation
pub fn profile_generate<P: AsRef<Path>>(&mut self, path: P) -> &mut Self {
let mut arg = OsString::new();
Expand Down
45 changes: 44 additions & 1 deletion src/tools/run-make-support/src/symbols.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use std::path::Path;

use object::{self, Object, ObjectSymbol, SymbolIterator};
use object::{self, Object, ObjectSymbol, Symbol, SymbolIterator};

/// Iterate through the symbols in an object file.
///
Expand Down Expand Up @@ -42,3 +42,46 @@ pub fn any_symbol_contains(path: impl AsRef<Path>, substrings: &[&str]) -> bool
false
})
}

/// Get a list of symbols that are in `symbol_names` but not the final binary.
///
/// The symbols must also match `pred`.
Copy link
Member

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.

///
/// The symbol names must match exactly.
Copy link
Member

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.

///
/// Panics if `path` is not a valid object file readable by the current user.
pub fn missing_exact_symbols<'a>(
path: impl AsRef<Path>,
symbol_names: &[&'a str],
pred: impl Fn(&Symbol<'_, '_>) -> bool,
) -> Vec<&'a str> {
let mut found = vec![false; symbol_names.len()];
with_symbol_iter(path, |syms| {
for sym in syms.filter(&pred) {
for (i, symbol_name) in symbol_names.iter().enumerate() {
found[i] |= sym.name_bytes().unwrap() == symbol_name.as_bytes();
}
}
});
return found
.iter()
.enumerate()
.filter_map(|(i, found)| if !*found { Some(symbol_names[i]) } else { None })
.collect();
}

/// Assert that the symbol file contains all of the listed symbols and they all match the given predicate
Copy link
Member

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.

pub fn assert_contains_exact_symbols(
path: impl AsRef<Path>,
symbol_names: &[&str],
pred: impl Fn(&Symbol<'_, '_>) -> bool,
) {
let missing = missing_exact_symbols(path.as_ref(), symbol_names, pred);
if missing.len() > 0 {
eprintln!("{} does not contain symbol(s): ", path.as_ref().display());
for sn in missing {
eprintln!("* {}", sn);
}
panic!("missing symbols");
}
}
1 change: 0 additions & 1 deletion src/tools/tidy/src/allowed_run_make_makefiles.txt
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ run-make/dep-info-doesnt-run-much/Makefile
run-make/dep-info-spaces/Makefile
run-make/dep-info/Makefile
run-make/emit-to-stdout/Makefile
run-make/extern-fn-reachable/Makefile
run-make/incr-add-rust-src-component/Makefile
run-make/issue-84395-lto-embed-bitcode/Makefile
run-make/jobserver-error/Makefile
Expand Down
26 changes: 0 additions & 26 deletions tests/run-make/extern-fn-reachable/Makefile

This file was deleted.

10 changes: 10 additions & 0 deletions tests/run-make/extern-fn-reachable/rmake.rs
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

Also, can you please squash the commits into one after adding a comment? There's some formatting commits.

Copy link
Member

Choose a reason for hiding this comment

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

Ping

Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
//@ ignore-cross-compile
use run_make_support::object::ObjectSymbol;
use run_make_support::rustc;
use run_make_support::symbols::assert_contains_exact_symbols;
fn main() {
rustc().input("dylib.rs").output("dylib.so").prefer_dynamic().run();
assert_contains_exact_symbols("dylib.so", &["fun1", "fun2", "fun3", "fun4", "fun5"], |sym| {
dbg!(dbg!(sym).is_global()) && !dbg!(sym.is_undefined())
});
Comment on lines +7 to +9
Copy link
Member

@jieyouxu jieyouxu Aug 26, 2024

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.

Copy link
Contributor Author

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?

Copy link
Member

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

Copy link
Member

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

}
Loading