Page MenuHomeFreeBSD

lang/rust: restore PORT_LLVM
ClosedPublic

Authored by vishwin on Oct 26 2021, 2:19 AM.
Tags
None
Referenced Files
Unknown Object (File)
Dec 11 2023, 4:18 AM
Unknown Object (File)
Nov 21 2023, 9:19 PM
Unknown Object (File)
Nov 9 2023, 8:40 PM
Unknown Object (File)
Nov 7 2023, 1:32 PM
Unknown Object (File)
Nov 6 2023, 7:12 AM
Unknown Object (File)
Oct 19 2023, 5:08 AM
Unknown Object (File)
Oct 18 2023, 11:47 PM
Unknown Object (File)
Oct 13 2023, 8:53 PM

Details

Summary

Allow Rust to build against ports LLVM.

llvm-config in cargo.toml implies linking to LLVM libraries, -Lnative is passed for linking to proceed.

[1]: https://github.com/rust-lang/rust/issues/46437#issuecomment-426696032
[2]: https://github.com/rust-lang/llvm-project/commits/rustc/14.0-2022-06-20

Test Plan
  • Builds & runs: 13.1/amd64, 14 -CURRENT/amd64.
  • www/firefox on 14-CURRENT/amd64 built by lang/rust with LLVM_PORT=on builds and runs fine.

Diff Detail

Repository
R11 FreeBSD ports repository
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

12.2/amd64 needs -fPIC or -z notext for stage2, update planned.

It'd be absolutely fantastic if Rust would use LLVM from Ports, instead of compiling an own copy. Everyone using Poudriere to build stuff would benefit from it.

This comment was removed by jbeich.

12.2/amd64

Fixed.

llvm-config in config.toml resulted in rustc_codegen_ssa being passed LinkOutputKind::DynamicPicExe, which results in amended "-pie" for LD, failing to link with R_X86_64_32(S) relocations and the rest of non-PIE libraries:

error: can't create dynamic relocation R_X86_64_32 against local symbol in readonly segment; recompile object files with -fPIC or pass '-Wl,-z,notext' to allow text relocations in the output

Cosmetic changes (e.g. pass pre-link -no-pie in config.toml instead of extra patch) left. This now proved to work with more common non-PIE base/ports and might be committed, but I want to check if an extra patch can be avoided. I'll see this weekend.

  • Replace extra-patch with REINPLACE_CMD.

pass pre-link -no-pie in config.toml instead of extra patch

Passing '-no-pie' via config.toml affects the rest of rustc (e.g. stage0 build).
Replace '-pie' to '-no-lie' in the only place that breaks build. Note, this can
be removed once base (>=14) and ports start building as PIE by default.

  • Use ports LLD for WASM.

Don't build bundled LLD ("rust-lld", build enabled via config.toml as "lld=true")
during PORT_LLVM+WASM rustc build.

WASM target also hardcodes "rust-lld". While it can be overriden during runtime via
e.g. rustc -C linker=wasm-ld13, it can't be overriden via config.toml target spec.
Replace 'rust-lld' with wasm-ld from ports LLVM. Build tested via:

$ rustc --target wasm32-unknown-unknown -O hello.rs

Inspired by: Gentoo https://bugs.gentoo.org/715348

Last time I asked about this people mentioned this:
https://github.com/rust-lang/rust/issues/49653

Note, I am not a Rust developer, but this is how I understand this:

  1. "--print target-{cpus,features}": prints LLVM target feature(s).
  2. "-C target-{cpu,feature}": passes target feature(s) to LLVM backend.
  3. "#[target_feature]": guards by supported CPU features, e.g.: https://rust-lang.github.io/rfcs/2045-target-feature.html#unconditional-code-generation-target_feature.

"[rust] Add accessors for MCSubtargetInfo CPU and Feature tables":

$ rg 'getFeatureTable'
src/llvm-project/llvm/include/llvm/MC/MCSubtargetInfo.h
236: ArrayRef<SubtargetFeatureKV> getFeatureTable() const {

compiler/rustc_llvm/llvm-wrapper/PassWrapper.cpp
416: const ArrayRef<SubtargetFeatureKV> FeatTable = MCInfo->getFeatureTable();
424: const ArrayRef<SubtargetFeatureKV> FeatTable = MCInfo->getFeatureTable();

PassWrapper.cpp, guarded by #ifdef LLVM_RUSTLLVM e.g.:

extern "C" void LLVMRustGetTargetFeature(LLVMTargetMachineRef TM, size_t Index,

                                       const char** Feature, const char** Desc) {
[...]
const ArrayRef<SubtargetFeatureKV> FeatTable = MCInfo->getFeatureTable();
const SubtargetFeatureKV Feat = FeatTable[Index];
*Feature = Feat.Key;
*Desc = Feat.Desc;

}

$ rg 'LLVMRustGetTargetFeature'

compiler/rustc_codegen_llvm/src/llvm_util.rs
229: let len = unsafe { llvm::LLVMRustGetTargetFeaturesCount(tm) };
235: llvm::LLVMRustGetTargetFeature(tm, i, &mut feature, &mut desc);

compiler/rustc_codegen_llvm/src/llvm/ffi.rs (FFI interface)
2136: pub fn LLVMRustGetTargetFeaturesCount(T: &TargetMachine) -> size_t;
2137: pub fn LLVMRustGetTargetFeature(

llvm_util.rs, e.g.:

llvm_target_features(tm: &llvm::TargetMachine) -> Vec<(&str, &str)>
[...]

llvm::LLVMRustGetTargetFeature(tm, i, &mut feature, &mut desc);
if feature.is_null() || desc.is_null() {
    bug!("LLVM returned a `null` target feature string");
}
let feature = CStr::from_ptr(feature).to_str().unwrap_or_else(|e| {
    bug!("LLVM returned a non-utf8 feature string: {}", e);
});
let desc = CStr::from_ptr(desc).to_str().unwrap_or_else(|e| {
    bug!("LLVM returned a non-utf8 feature string: {}", e);
});

[...]

$ rg 'llvm_target_features'

compiler/rustc_codegen_llvm/src/llvm_util.rs
228:fn llvm_target_features(tm: &llvm::TargetMachine) -> Vec<(&str, &str)> {
252: let mut target_features = llvm_target_features(tm);

llvm_util.rs:

fn print_target_features(sess: &Session, tm: &llvm::TargetMachine) {

let mut target_features = llvm_target_features(tm);

[...]

println!("Features supported by rustc for this target:");
for (feature, desc) in &rustc_target_features {
    println!("    {1:0$} - {2}.", max_feature_len, feature, desc);
}

[...]

The sole user of getFeatureTable() of Rust's LLVM fork is rustc target
feature printer (help). Repeating the same to trace getCPUTable() use
is not necessary because the only getCPUTable() call is guarded
with LLVM_RUSTLLVM.

  1. and 3. is tedious to trace, but likely not needed since Rust's LLVM fork

does not maintain related patches, and target_feature was switched from getFeatureTable
and LLVM_RUSTLLVM on LLVM 6+ [1]. Note, upstream has issues with non-stable rustc/LLVM
mapping [2], but SIMD for amd64 is stabilized [3][4].

[1]: https://github.com/rust-lang/rust/pull/49428
[2]: https://github.com/rust-lang/rust/pull/84004#issue-853651627
[3]: https://github.com/rust-lang/rust/blob/69e1d22ddbc6/compiler/rustc_codegen_ssa/src/target_features.rs#L54
[4]: https://github.com/rust-lang/rust/pull/49664

Also, -C target-cpu=skylake SIMD codegen here:
https://gist.github.com/mmatyas/639fc151dda34bb6dd07ee28120677a5

$ rustc -C target-cpu=skylake --emit asm mat4.rs
$ rg 'vmovss' mat4.s
[...]
vmovss %xmm0, 8(%rsp)
[...]

What I am concerned with:

If LLVM doesn't maintain ABI compatibility per major release (I don't know),
then lang/rust with PORT_LLVM will need a rebuild for each ABI breakage in a
minor release. LLVM soname versioning depends on LLVM_MAJOR (e.g. so.12 and so.13),
so I guess ABI compatibility during a major release is more likely to be than not.

Technical issues aside: Is there a committer who is going to own/support this?

lang/rust/Makefile
68

PORT_LLVM_MAKE_ENV

155

missing blank line

Technical issues aside: Is there a committer who is going to own/support this?

Not me.

Is there a committer who is going to own/support this?

Is being a committer a hard requirement?

I am interested to use this as long as I use FreeBSD, and plan to fix possible
breakage as I can. And I'm not the only one that can do this, FreeBSD is likely a community project.

PORT_LLVM was a non-default option for years before removal, and it wasn't always
QA'ed according to git log --grep='PORT_LLVM'.

Besides, I marked this as "experimental" to avoid implying maintainance burden.

Also, why aren't you interested? PORT_LLVM benefits are:

  • Faster patch testing and bisection.
  • Tier-2/3 machines with no package builders e.g. RISC-V can benefit from faster lang/rust builds, including the initial bring-up work.
  • Binary packages aren't always an option, and PORT_LLVM helps when:
    • poudriere-devel pkg seeding doesn't fetch on dependency mismatch e.g. after ca_root_nss or curl updates when project builders can't catch up for days.
    • The above also applies for port maintainers building ports which dependency graph includes lang/rust, resulting in a lang/rust build for each jail, e.g. every supported release sometimes combined with i386.

One would rather avoid building bundled LLVM when ports LLVM was already built.

I get faster lang/rust build time with PORT_LLVM=on (~36 min. vs ~20 min. with on).
The impact might be more noticeable on slower machines like riscv64 boards.

Just put "unsupported" after the option description. Let's hope somebody supports it once it's in. Some of the interested committers in here can commit it.

This revision is now accepted and ready to land.Nov 8 2021, 8:29 AM

Just put "unsupported" after the option description. Let's hope somebody supports it once it's in. Some of the interested committers in here can commit it.

Unsupported might be understood as "not working". At least, that's what I would thought. Maybe "experimental"?

I mean just use "community supported" then?

Anyway use your own judgement. I'm not insisting on it.

In D32654#742309, @evgeniy_khramtsov.org wrote:

Is there a committer who is going to own/support this?

I am interested to use this as long as I use FreeBSD, and plan to fix possible
breakage as I can. And I'm not the only one that can do this, FreeBSD is likely a community project.

I need to warn that now I have no time for FOSS at all until somewhere in 2022Q2-2022Q3. I stopped
updating beyond security fixes, and I no longer do local QA against unmodified base and ports. If anyone
interested commits this, you likely can't expect help from me until somewhere in 2022Q2-2022Q3.

2khramtsov_gmail.com edited the summary of this revision. (Show Details)
2khramtsov_gmail.com edited the test plan for this revision. (Show Details)
  • Rebase.
  • Change LIB_DEPENDS to BUILD_DEPENDS (and RUN_DEPENDS if also WASM=on)

External LLVM is static by default in Rust's LLVM wrapper, e.g.:

[...]
[rustc_llvm 0.0.0] cargo:rustc-link-lib=static=LLVMX86CodeGen
[rustc_llvm 0.0.0] cargo:rustc-link-lib=static=LLVMDemangle CodeGen
[rustc_llvm 0.0.0] cargo:rustc-link-lib=dylib=rt
[...]
[rustc_llvm 0.0.0] cargo:rustc-link-search=native=/usr/local/llvm14/lib
[rustc_llvm 0.0.0] cargo:rerun-if-env-changed=LLVM_LINKER_FLAGS
[rustc_llvm 0.0.0] cargo:rerun-if-env-changed=LLVM_STATIC_STDCPP
[rustc_llvm 0.0.0] cargo:rerun-if-env-changed=LLVM_USE_LIBCXX
[rustc_llvm 0.0.0] cargo:rustc-link-lib=c++
[...]
-l static=llvm-wrapper -l static=LLVMX86TargetMCA -l static=LLVMX86Disassembler -l static=LLVMX86AsmParser -l static=LLVMX86CodeGen
[...]

  • Rename PORT_LLVM to LLVM_PORT. Dynamic linking of external LLVM is possible, and base LLVM with base modification in theory could also be used, so leave room for different names for possible future radio menu, e.g. LLVM_BUNDLED, LLVM_PORT, LLVM_PORT_DYN, LLVM_BASE. Dynamic and base LLVM can reduce LLVM proliferation and have their use for runtime patching without rebuilding (e.g. LD_PRELOAD).
This revision now requires review to proceed.Jul 5 2022, 10:16 AM

Commit message was lost and I have yet to learn to use Phabricator beyond Web frontend:

From: Evgeniy Khramtsov <evgeniy@khramtsov.org>
Date: Tue, 5 Jul 2022 09:32:14 +0000
Subject: [PATCH] lang/rust: allow experimental static linking against
LLVM from ports

Rust bundles vendored LLVM for rarely essential changes,
or features guarded behind LLVM_RUSTLLVM, that may not
be required for a user.

Upstream also provides a way to link against external LLVM,
which can save build time when devel/llvmN was already built.

Provide experimental tunable "LLVM_PORT" to allow building
against LLVM from the FreeBSD ports collection. This tunable
is unsupported and its use is discouraged.

Inspired by:            Gentoo
Reviewed by:            tobik
Differential Revision:  https://reviews.freebsd.org/D32654

---

Intermediate commit messages are not kept even when using arcinator (phab's command line interface), you have to explicitly put them in the comment/summary section

In D32654#810083, @evgeniy_khramtsov.org wrote:

Commit message was lost and I have yet to learn to use Phabricator beyond Web frontend:

From: Evgeniy Khramtsov <evgeniy@khramtsov.org>
Date: Tue, 5 Jul 2022 09:32:14 +0000
Subject: [PATCH] lang/rust: allow experimental static linking against
LLVM from ports

Rust bundles vendored LLVM for rarely essential changes,
or features guarded behind LLVM_RUSTLLVM, that may not
be required for a user.
---

does these statements come from the rust project or is it your personal opinion?

Rust bundles vendored LLVM for rarely essential changes,
or features guarded behind LLVM_RUSTLLVM, that may not
be required for a user.
---

does these statements come from the rust project or is it your personal opinion?

Obviously, not from www.rust-lang.org. Feel free to reword.
I don't see anything essential in Rust's fork https://github.com/rust-lang/llvm-project/commits/rustc/14.0-2022-06-20 to build ports on FreeBSD.

This is Rust's documentation regarding updating and fixing bugs in their LLVM: https://rustc-dev-guide.rust-lang.org/backend/updating-llvm.html

Enthusiasm to upstream lost.

In FreeBSD, it seems that the separation between contributors and committers is too strong, as the latter
would prefer to avoid landing a change that could be problematic (even for not that complicated
non-default option that is also used in Gentoo), the burden is on the committer, not the author
of change, so no one wants to land. pkg.FreeBSD.org-centric culture also doesn't help.

...I am interested to use this as long as I use FreeBSD, and plan to fix possible
breakage as I can. And I'm not the only one that can do this, FreeBSD is likely a community project...

I also revoke this as I lost enthusiasm to fire poudriere for nothing, as there is no perspective of upstreaming.

Without QA against baseline vanilla ports, I abandon this revision. Anyone interested is free to commandeer revision,
but don't expect me to maintain it. I did enough work to explain and QA this change, and I have other >100 local changes that may have different fate.

I may reevaluate in future.

got it to work (consumers included), incoming

rename back to PORT_LLVM, not least because USES=llvm claims the variable name. Per upstream, minimum external LLVM supported is 14; next major Rust version (1.73) bumps to 15. For this version (1.72), two upstream commits included to fix LLVM 17 API compatibility.

On -CURRENT with LLVM 17, at least x11/alacritty and www/firefox (with LTO enabled) build and run as intended; the previous issues no longer exist due to tracking LLVM releases directly instead of snapshotting trunk.

Note: DOCS option in selected LLVM port must be disabled else a circular dependency arises.Fixed after R11:761dc8a9f4dd

I will support and maintain this bit.

fwiw, this is still approved.

Some optional suggestions below.

lang/rust/Makefile
101–105

These are small patches. IMHO it's clearer if you just copy them to files/

169

Can you match the existing style (dedent comment and silence ${REINPLACE_CMD})?

207–209

I don't think _RUST_BUILD_WASM is needed anymore. Why not make the conditional clearer with something like this

.if ${PORT_OPTIONS:MWASM} && !${PORT_OPTIONS:MPORT_LLVM}
        @${ECHO_CMD} 'lld=true' >> ${WRKSRC}/config.toml
.else
        @${ECHO_CMD} 'lld=false' >> ${WRKSRC}/config.toml
.endif
This revision is now accepted and ready to land.Oct 7 2023, 12:08 PM

What's the real life approach with this change? Are we going to stick to using default version in ports tree of LLVM? If doable is that supported by upstream and if not who is going to take care of fallouts? If not, are we going to track upstream instead and is it worth the effort of unbundling LLVM as there like will be a handful of ports depending on the same (recent) version of LLVM?

What's the real life approach with this change?

Saving cycles by using an external LLVM rather than compiling the entire bundled copy.

Are we going to stick to using default version in ports tree of LLVM?

The bundled LLVM remains the default option, this is merely a use-at-own-risk convenience for those who want/have to build themselves.

If doable is that supported by upstream and if not who is going to take care of fallouts? If not, are we going to track upstream instead and is it worth the effort of unbundling LLVM as there like will be a handful of ports depending on the same (recent) version of LLVM?

The situation has changed drastically when this option was removed. Back then, which was during the LLVM 6.0/7.0 era, rust tracked LLVM trunk and was thus more sensitive to interactions between them, as evidenced with the small API breakage that eventually lead to the option removal. Starting with LLVM 8.0 and the advent of llvm-wrapper, rust switched to tracking releases with cherry-picking existing bug fix commits, much like we do with our base LLVM. Concurrently and consequently, external LLVM support has improved, with upstream tracking which LLVM version ranges work with each rust release.

lang/rust/Makefile
101–105

Not only do these need to be applied before anything in ${PATCHDIR}, but these are incorporated in more recent nightlies and the just-released 1.73.0.

  • silence REINPLACE_CMD and clean up _RUST_BUILD_WASM
This revision now requires review to proceed.Oct 8 2023, 6:08 PM

add comment to upstream patches noting LLVM 17 compatibility fixes

This revision was not accepted when it landed; it landed in state Needs Review.Oct 9 2023, 3:31 PM
This revision was automatically updated to reflect the committed changes.