Migrate `min-global-align` and `no-alloc-shim` `run-make` tests to rmake #128407

Oneirical · 2024-07-30T18:40:28Z

Part of #121876 and the associated Google Summer of Code project.

Please try:

try-job: aarch64-apple
try-job: test-various
try-job: armhf-gnu
try-job: aarch64-gnu
try-job: aarch64-gnu

rustbot · 2024-07-30T18:40:36Z

rustbot has assigned @jieyouxu.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

rustbot · 2024-07-30T18:40:39Z

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

The run-make-support library was changed

cc @jieyouxu

Kobzol · 2024-07-30T18:48:31Z

@bors try

Migrate `min-global-align` and `no-alloc-shim` `run-make` tests to rmake Part of rust-lang#121876 and the associated [Google Summer of Code project](https://blog.rust-lang.org/2024/05/01/gsoc-2024-selected-projects.html). Please try: try-job: x86_64-msvc

bors · 2024-07-30T18:49:42Z

⌛ Trying commit 4c26950 with merge 8e53144...

bors · 2024-07-31T03:56:05Z

☔ The latest upstream changes (presumably #128075) made this pull request unmergeable. Please resolve the merge conflicts.

jieyouxu · 2024-08-03T02:50:06Z

tests/run-make/no-alloc-shim/rmake.rs

+//@ ignore-cross-compile
+// Reason: the compiled binary is executed
+
+//FIXME(Oneirical): ignore-msvc FIXME(bjorn3) can't figure out how to link with the MSVC toolchain


Discussion: I'm fine with ignoring msvc for now and leaving this FIXME in, unless you want to investigate how to make msvc linking succeeds here.

The error message suggests we might be missing a -lstdc++ or something?

I can try to take a look at the msvc stuff.

I suspect that the C runtime library isn't being passed to the linker because the compiler isn't being given any source code to compile, only object files. Adding libcmt.lib to the args (if msvc) would fix this but maybe there should be some more principled way of doing this. Maybe a link() alternative to cc that only does linking? Not sure.

One odd thing I noticed is __udivti3. That suggests to me that compiler-builtins is also needed.

Oh, actually we already have extra_c_flags. Instead of using a hard coded list, maybe we could just use --print native-static-libs to grab all the necessary ones. That should also include the CRT. But that's probably a big enough job to do separately.

I was able to get this test to pass on msvc by adding libcmt.lib as Chris mentioned, but also had to pass compiler-builtins:

use run_make_support::{ cc, cwd, extra_c_flags, has_extension, has_prefix, run, rustc, shallow_find_files, }; fn main() { rustc().input("foo.rs").crate_type("bin").emit("obj").panic("abort").run(); let libdir = rustc().print("target-libdir").run().stdout_utf8(); let libdir = libdir.trim(); let alloc_libs = shallow_find_files(&libdir, |path| { has_prefix(path, "liballoc-") && has_extension(path, "rlib") }); let core_libs = shallow_find_files(libdir, |path| { has_prefix(path, "libcore-") && has_extension(path, "rlib") }); let compiler_builtins_libs = shallow_find_files(libdir, |path| { has_prefix(path, "libcompiler_builtins") && has_extension(path, "rlib") }); cc().args(extra_c_flags()) .input("foo.o") .out_exe("foo") .arg("/link") .args(&alloc_libs) .args(&core_libs) .args(&compiler_builtins_libs) .arg("libcmt.lib") .run(); run("foo"); // Check that linking without __rust_no_alloc_shim_is_unstable defined fails rustc() .input("foo.rs") .crate_type("bin") .emit("obj") .panic("abort") .cfg("check_feature_gate") .run(); cc().args(extra_c_flags()) .input("foo.o") .out_exe("foo") .arg("/link") .args(&alloc_libs) .args(&core_libs) .args(&compiler_builtins_libs) .arg("libcmt.lib") .run_fail(); }

Not super sure about what's the best thing to do w.r.t. having to pass libcmt.lib.

Maybe it's better to accept this PR with ignore-msvc and then later figure the thing to do?

Instead of using a hard coded list, maybe we could just use --print native-static-libs to grab all the necessary ones. That should also include the CRT. But that's probably a big enough job to do separately.

I briefly looked at this and I don't think this is possible (currently), because its docs say:

This may be used when creating a staticlib crate type. If this is the only flag, it will perform a full compilation and include a diagnostic note that indicates the linker flags to use when linking the resulting static library.

Unfortunately, we're building a bin here, and I tried this locally, --print native-static-libs did not emit anything (just a newline).

Maybe it's better to accept this PR with ignore-msvc and then later figure the thing to do?

Yeah, this was what I had in mind in #128407 (comment), and I don't want to block this PR on trying to unignore it on msvc -- I can try to investigate that separately.

@Oneirical let's add a ignore-msvc for this with a FIXME linking to #128602 and call it a day for now 😆 I'll investigate separately.

@rustbot author

Unfortunately, we're building a bin here, and I tried this locally, --print native-static-libs did not emit anything (just a newline).

You'd need to build a fake staticlib then parse stdout. I think this could be done once in the extra_c_flags function and cached instead of hard coding a list of libraries to link, Or maybe it could be something compiletest does.

Very well. I have restored the ignore:

//@ ignore-msvc //FIXME(Oneirical): Getting this to work on MSVC requires passing libcmt.lib to CC, // which is not trivial to do. // Tracking issue: https://github.com/rust-lang/rust/issues/128602 // Discussion: https://github.com/rust-lang/rust/pull/128407#discussion_r1702439172

@rustbot review

jieyouxu

Thanks, LGTM overall, running few more try jobs to be safe.

jieyouxu · 2024-08-05T23:01:23Z

@bors try

jieyouxu · 2024-08-05T23:01:39Z

r=me if the try jobs pass
@bors delegate+

bors · 2024-08-05T23:01:42Z

✌️ @Oneirical, you can now approve this pull request!

If @jieyouxu told you to "r=me" after making some further change, please make that change, then do @bors r=@jieyouxu

jieyouxu · 2024-08-05T23:02:07Z

@bors rollup=iffy (symbol checks)

Migrate `min-global-align` and `no-alloc-shim` `run-make` tests to rmake Part of rust-lang#121876 and the associated [Google Summer of Code project](https://blog.rust-lang.org/2024/05/01/gsoc-2024-selected-projects.html). Please try: try-job: aarch64-linux try-job: test-various try-job: armhf-gnu try-job: x86_64-gnu-llvm-17

…eyouxu Migrate `min-global-align` and `no-alloc-shim` `run-make` tests to rmake Part of rust-lang#121876 and the associated [Google Summer of Code project](https://blog.rust-lang.org/2024/05/01/gsoc-2024-selected-projects.html). Please try: try-job: aarch64-apple try-job: test-various try-job: armhf-gnu try-job: x86_64-gnu-llvm-17

bors · 2024-08-08T14:27:55Z

⌛ Testing commit e1c1ac1 with merge 2d57c94...

bors · 2024-08-08T15:59:13Z

💔 Test failed - checks-actions

jieyouxu · 2024-08-08T16:19:34Z

Wow. I'll take a look at this tomorrow.

ChrisDenton · 2024-08-08T16:26:46Z

__muloti4 seems like another instance of missing compiler-builtins lib.

tests/run-make/min-global-align/rmake.rs

jieyouxu · 2024-08-14T04:27:48Z

tests/run-make/min-global-align/rmake.rs

+    if llvm_components_contain("x86") {
+        rustc().target("i686-unknown-linux-gnu").emit("llvm-ir").input("min_global_align.rs").run();
+        assert_count_is(3, rfs::read_to_string("min_global_align.ll"), "align 1");
+    }


Remark (for myself): I wonder if we could try to support revisions in rmake.rs. In ui tests and codegen tests revisions also define corresponding cfgs, maybe we could use them here as well (this remark is for myself).

Something like

//@ revisions: i686 systemz //@[i686] needs-llvm-components: x86 //@[systemz] needs-llvm-components: systemz fn main() { #[cfg(i686)] { // i686 specific assertions } #[cfg(systemz)] { // systemz specific assertions } }

Just a thought. But handling blessing / properly quarantining revisions would be a pain.

tests/run-make/no-alloc-shim/rmake.rs

bors · 2024-08-14T07:28:31Z

☔ The latest upstream changes (presumably #129076) made this pull request unmergeable. Please resolve the merge conflicts.

Oneirical · 2024-08-14T14:34:14Z

@bors r=@jieyouxu

bors · 2024-08-14T14:34:16Z

📌 Commit b85bedc has been approved by jieyouxu

It is now in the queue for this repository.

bors · 2024-08-14T19:24:21Z

⌛ Testing commit b85bedc with merge 13a5289...

bors · 2024-08-14T21:51:07Z

☀️ Test successful - checks-actions
Approved by: jieyouxu
Pushing 13a5289 to master...

rust-timer · 2024-08-15T00:01:13Z

Finished benchmarking commit (13a5289): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.2%	[-0.2%, -0.2%]	1
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results (secondary 2.9%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	2.9%	[2.3%, 3.4%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 753.65s -> 752.809s (-0.11%)
Artifact size: 341.43 MiB -> 341.47 MiB (0.01%)

rustbot assigned jieyouxu Jul 30, 2024

Kobzol mentioned this pull request Jul 30, 2024

Tracking Issue for porting run-make tests to use Rust #121876

Open

Oneirical force-pushed the feline-dotestication branch from 4c26950 to 2efbc94 Compare July 30, 2024 20:41

This comment has been minimized.

Sign in to view

jieyouxu reviewed Aug 3, 2024

View reviewed changes

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 5, 2024

Oneirical force-pushed the feline-dotestication branch 2 times, most recently from 59abb8e to 2064a76 Compare August 5, 2024 15:04

rustbot 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 5, 2024

Oneirical mentioned this pull request Aug 5, 2024

Migrate staticlib-dylib-linkage run-make test to rmake #128696

Merged

jieyouxu approved these changes Aug 5, 2024

View reviewed changes

This comment was marked as resolved.

Sign in to view

bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Aug 8, 2024

This comment has been minimized.

Sign in to view

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 8, 2024

jieyouxu requested changes Aug 14, 2024

View reviewed changes

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 14, 2024

Oneirical force-pushed the feline-dotestication branch from e1c1ac1 to 204f19b Compare August 14, 2024 14:32

Oneirical added 2 commits August 14, 2024 10:33

rewrite min-global-align to rmake

5e04cef

rewrite no-alloc-shim to rmake

b85bedc

Oneirical force-pushed the feline-dotestication branch from 204f19b to b85bedc Compare August 14, 2024 14:34

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 14, 2024

bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 14, 2024

bors merged commit 13a5289 into rust-lang:master Aug 14, 2024
7 checks passed

rustbot added this to the 1.82.0 milestone Aug 14, 2024

This was referenced Aug 14, 2024

Migrate reproducible-build run-make test to rmake #128456

Merged

Migrate libtest-thread-limit run-make test to rmake #128507

Merged

Migrate rlib-format-packed-bundled-libs and native-link-modifier-bundle run-make tests to rmake #129018

Merged

Migrate min-global-align and no-alloc-shim run-make tests to rmake #128407

Migrate min-global-align and no-alloc-shim run-make tests to rmake #128407

Conversation

Oneirical commented Jul 30, 2024 • edited by jieyouxu Loading

rustbot commented Jul 30, 2024

rustbot commented Jul 30, 2024

Kobzol commented Jul 30, 2024

bors commented Jul 30, 2024

This comment has been minimized.

bors commented Jul 31, 2024

jieyouxu Aug 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jieyouxu Aug 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jieyouxu left a comment

Choose a reason for hiding this comment

jieyouxu commented Aug 5, 2024

jieyouxu commented Aug 5, 2024

bors commented Aug 5, 2024

jieyouxu commented Aug 5, 2024

This comment was marked as resolved.

This comment was marked as resolved.

bors commented Aug 8, 2024

This comment has been minimized.

bors commented Aug 8, 2024

jieyouxu commented Aug 8, 2024

ChrisDenton commented Aug 8, 2024

jieyouxu Aug 14, 2024 • edited Loading

Choose a reason for hiding this comment

bors commented Aug 14, 2024

Oneirical commented Aug 14, 2024

bors commented Aug 14, 2024

bors commented Aug 14, 2024

bors commented Aug 14, 2024

rust-timer commented Aug 15, 2024

Overall result: ✅ improvements - no action needed

Migrate `min-global-align` and `no-alloc-shim` `run-make` tests to rmake #128407

Migrate `min-global-align` and `no-alloc-shim` `run-make` tests to rmake #128407

Oneirical commented Jul 30, 2024 •

edited by jieyouxu

Loading

jieyouxu Aug 3, 2024 •

edited

Loading

jieyouxu Aug 5, 2024 •

edited

Loading

jieyouxu Aug 14, 2024 •

edited

Loading