set subsections_via_symbols for ld64 helper sections by usamoi · Pull Request #139752 · rust-lang/rust

usamoi · 2025-04-13T15:56:43Z

rustbot · 2025-04-13T15:56:48Z

rustbot has assigned @Nadrieril.
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 · 2025-04-13T15:56:50Z

Some changes occurred in compiler/rustc_codegen_ssa

cc @WaffleLapkin

madsmtm

Thanks for the ping!

@rustbot label O-apple O-linkage

madsmtm · 2025-04-13T18:09:03Z

compiler/rustc_codegen_ssa/src/back/metadata.rs

+    if binary_format == BinaryFormat::MachO {
+        file.set_subsections_via_symbols();
+    }


It's not clear to me that this needs to be in create_object_file? Maybe it would be better to only use it in add_linked_symbol_object?

Also, MH_SUBSECTIONS_VIA_SYMBOLS is vastly under-documented, I'd really like to see a comment here explaining why it's safe for us to use.

Object::set_subsections_via_symbols says it should be called before add_section or add_subsection, so I feel it better to put it here (not error-prone).

I don't know whether it's safe or not, either. I searched on the Internet and found that this is the only way for Mach-O to implement GC.

My problem with having it in create_object_file is that it may negatively affect the other object files we create (which again is hard to tell for sure, since the docs around it are so limited).

Ah! I didn't notice that this function has multiple callers.

I will add a parameter to control this behavior.

Eh, I'd still have preferred to just change add_linked_symbol_object, it'd be unexpected if create_object_file added any sections or subsections, it's literally prefixed "create" as in "only create, do not fill".

But will leave it up to the compiler reviewer to decide, this is a nit anyhow.

tests/run-make/executable-no-mangle-strip/main.rs

madsmtm · 2025-04-13T18:13:43Z

tests/run-make/executable-no-mangle-strip/main.rs

+unsafe extern "C" {
+    unsafe static UNDEFINED: usize;
+}
+
+#[unsafe(no_mangle)]
+pub fn used() {
+    println!("UNDEFINED = {}", unsafe { UNDEFINED });
+}


Honestly, I'm kind of surprised that this worked in the past. Could you describe the use-case?

I'd also be interested, does this pattern work on other platforms?

It's used by https://github.com/pgcentralfoundation/pgrx. It's a framework for developing PostgreSQL extensions. This trick is used for writing hybrid (dylib and SQL generation) code in a library. Code about SQL generation could be compiled to an executable, since dylib-related code are GC-ed.

This should be reasonable usage. Please see #95604 and #95363 (comment).

Yes. It works for Linux, FreeBSD, and Windows, too.

Thanks for the answer. Might make sense to leave a comment in the file about this, and perhaps link to #139744, stating that it is a regression test for that?

Also, is the expected behaviour for the symbol to still be present, or do we expect the linker to completely strip it out? E.g. would dlsym(RTLD_DEFAULT, "used") work?

The symbol is present in fhe dynamic library. It's GC-ed iff the final artifact is the executable.

An idea then would perhaps be to add another test (or use UI-test revisions) with #![crate_type = "dylib"] and //@ dont-check-compiler-stderr that expectedly fails?

I think it's alreay tested in https://github.com/rust-lang/rust/tree/master/tests/run-make/c-link-to-rust-dylib.

No, I meant having test that fails when trying to link undefined symbols in a dylib

It is added.

jieyouxu · 2025-04-14T02:24:56Z

cc @bjorn3 @petrochenkov in case this is problematic

madsmtm · 2025-04-14T04:36:10Z

tests/ui/linking/executable-no-mangle-strip.rs

+#[unsafe(no_mangle)]
+pub unsafe fn used() {
+    println!("THIS_SYMBOL_SHOULD_BE_UNDEFINED = {}", unsafe { THIS_SYMBOL_SHOULD_BE_UNDEFINED });
+}


Nit: I know that #[no_mangle] implies it, but could we also add an explicit test for #[used]?

#[used] emits errors, while #[no_mangle] does not, on MacOS.

Don't know why.

Edit: using pr + lld, nightly + lld, stable + lld, stable + ld64, #[used] emits errors, too.

I know why now.

On MacOS, #[used] emits llvm.used. So #[used(compiler)] is needed here. Is it expected behavior?

TIL that #[used] is an alias for #[used(linker)] on some platforms:

rust/compiler/rustc_codegen_ssa/src/codegen_attrs.rs

Lines 195 to 223 in f836ae4

// Unfortunately, unconditionally using `llvm.used` causes

// issues in handling `.init_array` with the gold linker,

// but using `llvm.compiler.used` caused a nontrivial amount

// of unintentional ecosystem breakage -- particularly on

// Mach-O targets.

//

// As a result, we emit `llvm.compiler.used` only on ELF

// targets. This is somewhat ad-hoc, but actually follows

// our pre-LLVM 13 behavior (prior to the ecosystem

// breakage), and seems to match `clang`'s behavior as well

// (both before and after LLVM 13), possibly because they

// have similar compatibility concerns to us. See

// https://github.com/rust-lang/rust/issues/47384#issuecomment-1019080146

// and following comments for some discussion of this, as

// well as the comments in `rustc_codegen_llvm` where these

// flags are handled.

//

// Anyway, to be clear: this is still up in the air

// somewhat, and is subject to change in the future (which

// is a good thing, because this would ideally be a bit

// more firmed up).

let is_like_elf = !(tcx.sess.target.is_like_darwin

|| tcx.sess.target.is_like_windows

|| tcx.sess.target.is_like_wasm);

codegen_fn_attrs.flags |= if is_like_elf {

CodegenFnAttrFlags::USED

} else {

CodegenFnAttrFlags::USED_LINKER

};

Eventually #[used] should be changed to #[used(linker)] unconditionally anyway. Gold is deprecated upstream and broken with current rustc versions anyway: #139425

Edit: Opened #140872 for making #[used] an alias for #[used(linker)] unconditionally.

Nadrieril · 2025-04-16T18:27:02Z

r? codegen

usamoi · 2025-04-22T15:58:12Z

I hope this gets a beta backport to prevent pgrx from being broken on macOS.

@rustbot label +beta-nominated

madsmtm · 2025-04-22T20:57:39Z

compiler/rustc_codegen_ssa/src/back/metadata.rs

    }
+    if binary_format == BinaryFormat::MachO {
+        if set_subsections_via_symbols {
+            file.set_subsections_via_symbols();


Still needs a documentation comment IMO explaining why we do this.

saethlin · 2025-04-23T19:14:22Z

Looks good! Thanks @madsmtm for also reviewing ❤️

@bors r=saethlin,madsmtm

bors · 2025-04-23T19:14:24Z

📌 Commit e18e599 has been approved by saethlin,madsmtm

It is now in the queue for this repository.

usamoi · 2025-04-24T17:11:16Z

On windows-gnu, the linker removes statics marked with #[used], but does not functions marked with #[used] or referenced by statics marked with #[used]. Not interested in this target, so I mark the test as ignore-windows-gnu for now.

saethlin · 2025-04-24T17:36:20Z

Can you add the reason windows-gnu is special for this test to the magic comment?

saethlin · 2025-04-24T19:29:43Z

@bors r=saethlin,madsmtm

bors · 2025-04-24T19:29:45Z

📌 Commit b1a3831 has been approved by saethlin,madsmtm

It is now in the queue for this repository.

bors · 2025-04-24T22:59:04Z

⌛ Testing commit b1a3831 with merge 847e3ee...

bors · 2025-04-25T02:18:41Z

☀️ Test successful - checks-actions
Approved by: saethlin,madsmtm
Pushing 847e3ee to master...

github-actions · 2025-04-25T02:21:02Z

What is this?

This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.

Comparing d7ea436 (parent) -> 847e3ee (this PR)

Test differences

Show 6 test diffs

Stage 1

[ui] tests/ui/linking/cdylib-no-mangle.rs: [missing] -> ignore (only executed when the target vendor is Apple) (J1)
[ui] tests/ui/linking/executable-no-mangle-strip.rs: [missing] -> pass (J1)

Stage 2

[ui] tests/ui/linking/executable-no-mangle-strip.rs: [missing] -> pass (J0)
[ui] tests/ui/linking/cdylib-no-mangle.rs: [missing] -> pass (J2)
[ui] tests/ui/linking/executable-no-mangle-strip.rs: [missing] -> ignore (ignored when the operating system and target environment are windows-gnu (only statics marked with used can be GC-ed on windows-gnu)) (J3)
[ui] tests/ui/linking/cdylib-no-mangle.rs: [missing] -> ignore (only executed when the target vendor is Apple) (J4)

Job group index

Test dashboard

Run

cargo run --manifest-path src/ci/citool/Cargo.toml -- \
    test-dashboard 847e3ee6b0e614937eee4e6d8f61094411eadcc0 --output-dir test-dashboard

And then open test-dashboard/index.html in your browser to see an overview of all executed tests.

Job duration changes

dist-aarch64-linux: 5203.0s -> 7914.2s (52.1%)
dist-x86_64-apple: 9270.3s -> 11170.0s (20.5%)
dist-loongarch64-linux: 6264.6s -> 7264.5s (16.0%)
aarch64-apple: 4275.1s -> 3670.3s (-14.1%)
dist-x86_64-linux: 4945.4s -> 5538.0s (12.0%)
x86_64-apple-2: 4757.0s -> 5185.7s (9.0%)
x86_64-gnu-aux: 5920.6s -> 6338.0s (7.1%)
dist-x86_64-mingw: 7686.2s -> 8201.0s (6.7%)
x86_64-gnu-distcheck: 4707.8s -> 4422.4s (-6.1%)
dist-x86_64-netbsd: 4991.9s -> 5292.1s (6.0%)

How to interpret the job duration changes?

Job durations can vary a lot, based on the actual runner instance
that executed the job, system noise, invalidated caches, etc. The table above is provided
mostly for t-infra members, for simpler debugging of potential CI slow-downs.

rust-timer · 2025-04-25T03:48:58Z

Finished benchmarking commit (847e3ee): comparison URL.

Overall result: ❌✅ regressions and improvements - please read the text below

Our benchmarks found a performance regression caused by this PR.
This might be an actual regression, but it can also be just noise.

Next Steps:

If the regression was expected or you think it can be justified,
please write a comment with sufficient written justification, and add
@rustbot label: +perf-regression-triaged to it, to mark the regression as triaged.
If you think that you know of a way to resolve the regression, try to create
a new PR with a fix for the regression.
If you do not understand the regression or you think that it is just noise,
you can ask the @rust-lang/wg-compiler-performance working group for help (members of this group
were already notified of this PR).

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

Instruction count

This is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.

	mean	range	count
Regressions ❌ (primary)	0.3%	[0.2%, 0.3%]	2
Regressions ❌ (secondary)	0.3%	[0.2%, 0.6%]	30
Improvements ✅ (primary)	-0.2%	[-0.2%, -0.2%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.1%	[-0.2%, 0.3%]	3

Max RSS (memory usage)

Results (primary -0.4%, secondary -1.7%)

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)	2.1%	[1.7%, 2.4%]	2
Regressions ❌ (secondary)	2.1%	[1.6%, 2.3%]	3
Improvements ✅ (primary)	-1.2%	[-3.6%, -0.4%]	6
Improvements ✅ (secondary)	-3.9%	[-7.8%, -2.0%]	5
All ❌✅ (primary)	-0.4%	[-3.6%, 2.4%]	8

Cycles

Results (primary 0.7%)

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.7%	[0.5%, 0.9%]	4
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.7%	[0.5%, 0.9%]	4

Binary size

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

Bootstrap: 777.966s -> 776.153s (-0.23%)
Artifact size: 365.11 MiB -> 365.17 MiB (0.02%)

rylev · 2025-04-29T12:00:09Z

I don't really understand how this change could have the performance impact seen here. Some of the regressions do seem like pure noise (with the next run returning back to the previous baseline), but not all of them.

@usamoi @saethlin would you agree that the regressions here are unlikely to be actually caused by this change?

madsmtm · 2025-04-29T12:14:46Z

This should only affect macOS/Apple platforms, and I don't think the benchmarks are run on those?

rylev · 2025-04-29T12:20:26Z

That is correct. I'll mark this as triaged

@rustbot label: +perf-regression-triaged

jieyouxu · 2025-05-01T14:09:37Z

This was discussed in today's compiler triage meeting. Beta backport was approved, thanks!

@rustbot label: +beta-accepted

[beta] backports - Don't allow flattened format_args in const. rust-lang#139624 - set subsections_via_symbols for ld64 helper sections rust-lang#139752 - Fix detection of `main` function if there are expressions around it rust-lang#140220 - rustdoc: Fix doctest heuristic for main fn wrapping rust-lang#140420 - extend the list of registered dylibs on `test::prepare_cargo_test` rust-lang#140563 r? cuviper

rustbot assigned Nadrieril Apr 13, 2025

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Apr 13, 2025

usamoi force-pushed the macos-used branch from 73a7a59 to 782d196 Compare April 13, 2025 16:22

rustbot added the A-run-make Area: port run-make Makefiles to rmake.rs label Apr 13, 2025

This comment was marked as outdated.

Sign in to view

madsmtm requested changes Apr 13, 2025

View reviewed changes

madsmtm reviewed Apr 13, 2025

View reviewed changes

rustbot added the O-apple Operating system: Apple / Darwin (macOS, iOS, tvOS, visionOS, watchOS) label Apr 13, 2025

usamoi force-pushed the macos-used branch from 782d196 to 7a7548a Compare April 14, 2025 04:21

madsmtm reviewed Apr 14, 2025

View reviewed changes

usamoi force-pushed the macos-used branch 2 times, most recently from 6e175fc to 7d10cf7 Compare April 14, 2025 05:34

jieyouxu added the A-linkage Area: linking into static, shared libraries and binaries label Apr 14, 2025

usamoi force-pushed the macos-used branch from 7d10cf7 to e1088d5 Compare April 14, 2025 10:17

jieyouxu removed the A-run-make Area: port run-make Makefiles to rmake.rs label Apr 14, 2025

usamoi force-pushed the macos-used branch from e1088d5 to 3d43603 Compare April 14, 2025 14:07

rustbot assigned saethlin and unassigned Nadrieril Apr 16, 2025

rustbot added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Apr 22, 2025

madsmtm reviewed Apr 22, 2025

View reviewed changes

usamoi force-pushed the macos-used branch from 3d43603 to e18e599 Compare April 23, 2025 01:16

madsmtm approved these changes Apr 23, 2025

View reviewed changes

bors added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 24, 2025

usamoi force-pushed the macos-used branch from 64ba44f to 69bfbf9 Compare April 24, 2025 17:07

set subsections_via_symbols for ld64 helper sections

b1a3831

usamoi force-pushed the macos-used branch from 69bfbf9 to b1a3831 Compare April 24, 2025 17:41

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-review Status: Awaiting review from the assignee but also interested parties. labels Apr 24, 2025

bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 25, 2025

bors merged commit 847e3ee into rust-lang:master Apr 25, 2025
7 checks passed

rustbot added this to the 1.88.0 milestone Apr 25, 2025

rustbot added the perf-regression Performance regression. label Apr 25, 2025

rustbot added the perf-regression-triaged The performance regression has been triaged. label Apr 29, 2025

rustbot added the beta-accepted Accepted for backporting to the compiler in the beta channel. label May 1, 2025

cuviper mentioned this pull request May 2, 2025

[beta] backports #140592

Merged

cuviper modified the milestones: 1.88.0, 1.87.0 May 2, 2025

cuviper removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label May 2, 2025

usamoi mentioned this pull request May 25, 2025

fix: build error on Apple Silicon due to ambiguous call to svwhilelt functions tensorchord/VectorChord#260

Closed

	// Unfortunately, unconditionally using `llvm.used` causes
	// issues in handling `.init_array` with the gold linker,
	// but using `llvm.compiler.used` caused a nontrivial amount
	// of unintentional ecosystem breakage -- particularly on
	// Mach-O targets.
	//
	// As a result, we emit `llvm.compiler.used` only on ELF
	// targets. This is somewhat ad-hoc, but actually follows
	// our pre-LLVM 13 behavior (prior to the ecosystem
	// breakage), and seems to match `clang`'s behavior as well
	// (both before and after LLVM 13), possibly because they
	// have similar compatibility concerns to us. See
	// https://github.com/rust-lang/rust/issues/47384#issuecomment-1019080146
	// and following comments for some discussion of this, as
	// well as the comments in `rustc_codegen_llvm` where these
	// flags are handled.
	//
	// Anyway, to be clear: this is still up in the air
	// somewhat, and is subject to change in the future (which
	// is a good thing, because this would ideally be a bit
	// more firmed up).
	let is_like_elf = !(tcx.sess.target.is_like_darwin
	\|\| tcx.sess.target.is_like_windows
	\|\| tcx.sess.target.is_like_wasm);
	codegen_fn_attrs.flags \|= if is_like_elf {
	CodegenFnAttrFlags::USED
	} else {
	CodegenFnAttrFlags::USED_LINKER
	};

Uh oh!

Conversation

usamoi commented Apr 13, 2025 • edited by saethlin Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Apr 13, 2025

Uh oh!

rustbot commented Apr 13, 2025

Uh oh!

This comment was marked as outdated.

madsmtm left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

madsmtm Apr 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

usamoi Apr 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jieyouxu commented Apr 14, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

usamoi Apr 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

usamoi Apr 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bjorn3 Apr 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Nadrieril commented Apr 16, 2025

Uh oh!

usamoi commented Apr 22, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

saethlin commented Apr 23, 2025

Uh oh!

bors commented Apr 23, 2025

Uh oh!

usamoi commented Apr 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

saethlin commented Apr 24, 2025

Uh oh!

usamoi commented Apr 13, 2025 •

edited by saethlin

Loading

madsmtm left a comment •

edited

Loading

madsmtm Apr 22, 2025 •

edited

Loading

usamoi Apr 13, 2025 •

edited

Loading

usamoi Apr 14, 2025 •

edited

Loading

usamoi Apr 14, 2025 •

edited

Loading

bjorn3 Apr 14, 2025 •

edited

Loading

usamoi commented Apr 24, 2025 •

edited

Loading