Keep codegen units unmerged when building compiler builtins

[tmiasko]

Make it possible to control how mono items are partitioned into code generation
units, when compiling the compiler builtins, by retaining the original partitioning.

Helps with #48625, #61063, #67960, #70489.

r? @alexcrichton

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[alexcrichton]

Seems plausible to me! I think though this isn't exactly what we want, but we can still work with it as-is. Ideally we want one-symbol-per-object file in compiler-builtins, but this is only preventing merging codegen units. In the ideal world the compiler's partitioning algorithm would place all #[no_mangle] symbols in compiler-builtins in their own CGU.

That being said I think this is by far the easiest way to implement this in the compiler, and I'd probably say we should keep this. To fully fix the referenced issues, though, we'll need to make changes to compiler-builtins too. I think we'll want to ensure that we have a module-per-symbol in compiler-builtins to ensure that we have a CGU-per-symbol.

Does that make sense?

[alexcrichton]

Could this comment be updated to basically indicate that for the compiler-builtins crate specifically we want one-symbol-per-object-file like most C implementations do? And comment as well about how that appeases linkers where the goal is that we're trying to avoid duplicate symbol errors when other compiler-rt libraries probably have the same symbols but highly likely the same API too

[tmiasko]

The goal would be to obtain similar partitioning as other libraries we are likely to link-in (libgcc primarily). Usually an individual object file for a builtin, but not always. The approach here provides enough flexibility for that, although it does require additional changes in compiler builtins crate.

Placing each symbol individually would create similar issues in other direction.

[alexcrichton]

Ah yeah that's a good point! All-in-all I'm ok with this change as-is for rustc, but I think we'll likely need to edit some things on the compiler-builtins side of things after this lands as well (or in parallel, either way!)

[petrochenkov]

@tmiasko
Could you explain the background here a bit, why does keeping symbols in compiler-builtins in separate object files help with the linked issues?

I've seen these conflicts between compiler-builtins and libgcc myself and mitigated them with --allow-multiple-definition, and I've also seen this this code

rust/src/librustc_target/spec/android_base.rs

Lines 5 to 10 in bd18bc9

	// Many of the symbols defined in compiler-rt are also defined in libgcc.
	// Android's linker doesn't like that by default.
	base.pre_link_args
	.get_mut(&LinkerFlavor::Gcc)
	.unwrap()
	.push("-Wl,--allow-multiple-definition".to_string());

applying that workaround globally on Android, so I kind of thought the conflicts are unavoidable and "by design".

[alexcrichton]

Oh wow I had no idea we did that. Turns out the inclusion of that flag dates back quite awhile... (and I even r+'d it)

In any case the reason for this change has to do with how linkers work with conflicting symbols. The compiler-builtins crate defines symbols that are highly likely to clash with other implementations (think memcpy). One way the clash can arise is if you generate a staticlib (which includes compiler-builtins) and then you link that with your native compiler which brings in something like libgcc/libcompiler-rt. This basically sets up a situation for a symbol clash.

The way the linker works is that it only loads an object file from an archive if it actually needs a symbol in the object file. The linker then generates an error (by default) if the same symbol is defined twice. The reason we don't hit this error all over the place today is mostly by luck rather than anything else. One common way this can arise is a scenario like this:

• First the native code you're compiling with references an intrinsic Rust doesn't need, so it's loaded from the system libgcc. This system libgcc object file which defines the symbol may define another symbol as well.
• Next you load an unrelated intrinsic which is defined with Rust (say, for example, a 128-bit arithmetic intrinsic).
• An error happens if the Rust object file happens to define one of the same symbols of the original C object file. Even if you don't use the symbol it still generates an error.

The goal of this PR is to allow compiler-builtins to work similarly to other libgcc/libcompiler-rt implementations which is to isolate symbols to their own object file (most of the time, not always). This means that if the linker loads a symbol from compiler-builtins in Rust it'll only get that one symbol, no extras. That way it's guaranteed to not clash with other objects.

I believe some "uber portable" C libraries often do this where they'll define a function-per-object file. Some of this is for symbol visibility, but historically some of it has also been for smaller linked binaries when you aren't otherwise passing -ffunction-sections and -Wl,--gc-sections.

tl;dr; this'll give fine-grained control over object file-to-symbol allocation for compiler-builtins to allow isolating symbols to their own object file as necessary to avoid clashes with system libraries which define the same intrinsics.

---

My personal preference is to not pass flags like --allow-multiple-definition since it requires all consumers to pass it. If we can solve this ourselves without that flag then no one has to pass it, including rustc. (we might even be able to remove that ancient block for android)

[petrochenkov]

@alexcrichton
Thanks!
This part

linker works is that it only loads an object file from an archive if it actually needs a symbol in the object file.

was new to me, I assumed that if you link libfoo.a and libbar.a together they will conflict if they have symbols with the same name in any of their object files.

I agree that --allow-multiple-definition is rather a workaround than a proper solution and should be avoided if it's possible.

[alexcrichton]

Yeah if all it took was symbols in an archive to overlap with another archive, we would have had major problems long ago with this issue (compiler-builtins basically always duplicates at least one symbol in libgcc). I sort of suspect that "feature" was actually an optimization to linkers at some point which ended up becoming a defining feature, but hey it's what we've got to work with.

[alexcrichton]

@bors: r+

[bors]

📌 Commit c8b83ba has been approved by alexcrichton